New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Process] Process Manager #23596

Closed

johnnickell wants to merge 12 commits into symfony:master from johnnickell:feature/process/process-manager

johnnickell commented Jul 20, 2017

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#8454
License	MIT
Doc PR	todo

Simple configurable process manager. Processes are added to the manager, then they can be run in parallel.

Example Usage

// will run up to 3 processes in parallel
$processManager = new ProcessManager(3);

foreach ($this->getProcessesToRun() as $process) {
    $processManager->add($process);
}

$processManager->run();

By default the process manager will throw an exception if any of the child processes fail. If you would like to get around this, you can specify that when calling the run method:

$processManager->run(ProcessManager::IGNORE_ON_ERROR);

johnnickell added 5 commits

July 19, 2017 14:53


          [Process] added process manager

8ee5d26


          [Process] fix and simplification of process manager

01c12e1


          [Process] renamed variable for consistency

dba981e


          [Process] adjusted api to match conventions

27d9217


          [Process] added tests for process manager

ce42b1a

carsonbot added Status: Needs Review Process Feature labels

johnnickell added 5 commits

July 19, 2017 19:11


          [Process] updated to match code standards

23a76cc


          [Process] updated changelog

5c8c587


          [Process] adjusted test to be more reliable

4c58a70


          [Process] fixed test against race condition

8079b78


          [Process] adjusted test to be more reliable

93c8c36

nicolas-grekas added this to the 3.4 milestone

nicolas-grekas self-requested a review

July 20, 2017 07:10

linaori suggested changes

View reviewed changes

Contributor

linaori left a comment

While this solution as is, might not be suitable for enterprise companies, it's a very nice addition to the process component imo.

I'd like to see some architectural changes to this class as I mentioned in my comments, but implementation wise, it looks similar to something I've written and used before.

src/Symfony/Component/Process/ProcessManager.php Outdated

+               *
+               * @author John Nickell <[email protected]>
+               */
+              class ProcessManager

Contributor

linaori Jul 20, 2017

I would like to see a different name here, what about something towards ParallelProcessRunner or ProcessRunnerQueue? Naming Classes - How to avoid calling everything a "Manager"?

Also from an architectural point of view, I'd like to see an interface implemented here and the constants either in that interface or a dedicated final (with private constructor) class. This will avoid difference references and gives a proper decoration possibility.

src/Symfony/Component/Process/ProcessManager.php Outdated

+                  protected function init()
+                  {
+                      if ($this->maxConcurrent === 0 || count($this->procs) < $this->maxConcurrent) {
+                          $next = $this->queue->dequeue();

Contributor

linaori Jul 20, 2017

Could make a guard clause here, prevents nesting while not needed

src/Symfony/Component/Process/ProcessManager.php Outdated

+                          /** @var Process $process */
+                          $process = $next[0];
+                          /** @var callable|null $callback */
+                          $callback = $next[1];

Contributor

linaori Jul 20, 2017

Could use list($process, $callback) = $this->queue->dequeue(); instead. Don't think typehints are needed here as it's all internal anyway.

src/Symfony/Component/Process/ProcessManager.php Outdated

+                      usleep($this->usleepDelay);
+                      foreach ($this->procs as $pid => $process) {
+                          $process->checkTimeout();
+                          if (!$process->isRunning()) {

Contributor

linaori Jul 20, 2017

Could be turned into a guard clause as well by using continue;

src/Symfony/Component/Process/ProcessManager.php Outdated

+                  /**
+                   * Starts a process if possible.
+                   */
+                  protected function init()

Contributor

linaori Jul 20, 2017

Should either be private or public

src/Symfony/Component/Process/ProcessManager.php Outdated

+                   *
+                   * @throws ProcessFailedException When a process fails
+                   */
+                  protected function tick($errorBehavior)

Contributor

linaori Jul 20, 2017

Should either be private or public

src/Symfony/Component/Process/ProcessManager.php Outdated

+                  /**
+                   * Stop running processes.
+                   */
+                  protected function stop()

Contributor

linaori Jul 20, 2017

Should either be private or public

src/Symfony/Component/Process/ProcessManager.php Outdated


		namespace Symfony\Component\Process;

		use SplQueue;

Contributor

linaori Jul 20, 2017

I believe this should always be an \SplQueue usage and global namespaces are never important in Symfony (correct me if I'm wrong here)

ro0NL reviewed

View reviewed changes

src/Symfony/Component/Process/ProcessManager.php Outdated

+                   * @param callable|null $callback A PHP callback to run whenever there is some
+                   *                                output available on STDOUT or STDERR
+                   */
+                  public function add(Process $process, callable $callback = null)

Contributor

ro0NL Jul 20, 2017

what about error behavior per process? add($proces, $callback, $errorBehavior)

ro0NL reviewed

View reviewed changes

src/Symfony/Component/Process/ProcessManager.php Outdated

+                   */
+                  public function clear()
+                  {
+                      $this->queue = new SplQueue();

Contributor

ro0NL Jul 20, 2017

new \SplQueue() indeed

ro0NL reviewed

View reviewed changes

src/Symfony/Component/Process/ProcessManager.php Outdated

+                   *
+                   * @var int
+                   */
+                  protected $maxConcurrent;

Contributor

ro0NL Jul 20, 2017

private; doblock seems redundant. Same for other constructor args.

Member

nicolas-grekas commented Jul 20, 2017 •

edited

Loading

Thank you @johnnickell.
I spent some time to review and think about this. There are code style comments related to the exact convention we are used to (as already spotted by others.)

But more importantly, I think this approach is not good enough for ending up in Symfony.
I also think that this approach is the best we can have with the current design of the Process component, so that it's perfectly legitimate to propose this implementation.
Yet, the fact that this is basically a while+usleep loop triggers a red flag in my mind.

The core issue is that the Process component hides a stream_select, and because this stream_select is "hidden", we cannot efficiently manage 3 processes (ie 3 stream_select).

The proper design would need a single stream_select for all processes. Put another way, we'd need an event loop - even a light one (not to recreate React & co.).

So, my stand here is that we should refactor the Process class to inject it a small event loop (eg as an optional constructor argument so that we keep BC). Then, once that refacto is done, a process manager will be trivial to write - correctly. :)

Thank you again for your PR, sorry for the negative (but hopefully constructive) comment :)

nicolas-grekas removed their request for review

July 20, 2017 14:08

Author

johnnickell commented Jul 20, 2017

@nicolas-grekas Thank you very much for taking the time to look at this and provide feedback.

I don't have experience using an event loop with PHP, so the refactor might be beyond my current skill. I would be glad to add the requested changes as an exercise, even though this pull request will be closed.

I'll continue to dig into the process class and experiment. If I come up with something useful, I will definitely share it.

Thank you again!


          [Process] renamed classes and adjusted per peer review

95e0499

Member

nicolas-grekas commented Jul 20, 2017

@johnnickell, that event loop idea is not a trivial one, I may give it a try in the next weeks... Meanwhile, I think we can close this PR. As you want of course.


          [Process] moved use statements

4da8563

johnnickell closed this

nicolas-grekas mentioned this pull request

[Process] add a process manager allowing to run commands in parallel (queueing up if needed) #8454

Closed

Contributor

TomasVotruba commented Feb 2, 2018

What state is this in? I'd love this feature build in Symfony. Gotta hack it around the whole process now :(

TheTechsTech added a commit to symplely/processor that referenced this pull request


          update documentation, code clean up, add shortcut call

d720082

In reference to:
symfony/symfony#23596
symfony/symfony#8454

TheTechsTech mentioned this pull request

The serialized output may only be 10240 bytes long. - Is there a reason for this limit? spatie/async#76

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Process Status: Needs Review