Don't wait for a timer when the process already died #15
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
On first sight it looks good to me but I'm slightly worried that when dealing with a lot of data between parent and child process CPU usage will be elevated. Have to test it to be sure but this is the first thing that came to mind. Aside from that 👍 on the PR, 0.1 second is a really long time to wait for something. |
src/Process.php
Outdated
| @@ -110,13 +110,24 @@ public function start(LoopInterface $loop, $interval = 0.1) | |||
| stream_set_blocking($pipe, 0); | |||
| } | |||
|
|
|||
| $loop->addPeriodicTimer($interval, function (Timer $timer) { | |||
| $periodicMonitor = function (Timer $timer) { | |||
There was a problem hiding this comment.
How about moving the above anonymous function to a (public) checkRunning() (better names welcome) instead?
This way, the user would have more control over when to check the process status. After all, the consumer of this API will probably have a better understanding on when to consider a process dead.
|
Updated |
nicolas-grekas
changed the title
|
I'd like to finally get this in, can you add a test case for this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the 'exit' event occurs only after some time defined by a periodic timer.
This means a react child-process can't be faster than this timer.
Using this patch, the exit even is triggered as soon as the process exits, which can be way earlier than what is done today.