Ensure non-blocking process wait and correctly report process exit status #3419
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
|
This still needs to be tested on windows, CI doesnt run the process tests on windows. |
mfelsche
force-pushed
the
polling-process-wait
branch
from
33fe254 to
9348421
Compare
|
Ok, here we go! Tests run fine on windows! 🎉 |
This was referenced Dec 19, 2019
|
@cquinn can you take a look at this? |
cquinn
approved these changes
Jan 13, 2020
packages/process/_process.pony
Outdated
| @@ -34,13 +34,95 @@ primitive _EAGAIN | |||
| primitive _EINVAL | |||
| fun apply(): I32 => 22 | |||
|
|
|||
| primitive WNOHANG | |||
There was a problem hiding this comment.
Maybe a Comment here to describe what WNOHANG represents. Also, would it be possible to make it _WNOHANG?
There was a problem hiding this comment.
The _ is a good catch. That should be changed.
SeanTAllen
requested changes
Jan 13, 2020
mfelsche
force-pushed
the
polling-process-wait
branch
from
9348421 to
6eaf1a3
Compare
mfelsche
force-pushed
the
polling-process-wait
branch
3 times, most recently
from
d88817c to
989b563
Compare
|
@SeanTAllen all good to go from my side. How about you? |
|
@mfelsche 0.34.1 has been released. you can merge this whenever. can you add the release notes to the 0.34.2 issue? |
by calling waitpid with WNOHANG on posix and doing the equivalent on windows and if the process didnt exit yet, start a timer and poll the process. But only create timers etc. if the process didnt finish in the first try to save resources in the normal case.
So users of ProcessNotify are properly notified of how their process exited. If it was killed by a signal, child_exit_status will be a Signaled instance containing the signal number that terminated the process (only on POSIX).
It was depending on being executed in the directory of its source, which was not true. THe test accidentally created an empty executable version of that file, while ensuring it to be executable. This was polluting the test directory. Now everything is put inside a temp dir, which is cleaned afterwards.
mfelsche
force-pushed
the
polling-process-wait
branch
from
02ba6a1 to
10011c0
Compare
mfelsche
force-pushed
the
polling-process-wait
branch
from
10011c0 to
a6f5a36
Compare
SeanTAllen
added
the
changelog - fixed
|
@mfelsche don't forget to SQUASH when you merge this! 😉 I forget sometimes, thought I would remind you just in case. |
|
Thanks, much appreciated! |
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.
Waiting or a process to finish was potentially blocking both on windows and posix systems. It was only ever called when the child process was killed beforehand or has closed both stdout and stderr, so the actual risk to block was low, but still. Now it calls
waitpid(and the windows equivalent) withWNOHANG(on windows with timeout of 0) to ensure it does not block. If the process is still running at that point, a timer is scheduled to poll the process using the non-blockingwaitpid.During testing I discovered that processes terminated by signals were usually reported as exited with exit-code 0, which is wrong. I changed the
ProcessNotify.disposesignature to receive aSignaledorExitedclass containing signal number or exit code.This works pretty well, the only case this does not catch is, when a process exits and sets an exit-code in a signal handler.
ProcessNotify.disposewill be called withSignaled, not withExitedand the exit code is basically lost. But I still prefer this.As this is a breaking change, I thought to keep it as draft at first, in order to discuss it.
I would consider the non-blocking waitpid call as internal and the improved exit status reporting as actually fixing a bug, that is why I didn't create an RFC for this.