Fix incorrect Windows process waiting #3559
Merged
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
I think there are a couple of problems with recent changes to `ponyint_win_process_wait`. Changing the timeout value to 0 means that `WaitForSingleObject` won't block, so we can't close the handle in that case. Also 0x80 is the return code `WAIT_ABANDONED` which doesn't apply to processes. We should be checking for `WAIT_TIMEOUT` (0x102) which means the process hasn't exited yet. Also we need to make sure that retval is not 0 or 1 in an error case where `GetLastError()` returns 0 for some reason.
|
@kulibali did you verify that this fixes the corral issue: |
SeanTAllen
reviewed
May 13, 2020
src/libponyrt/lang/process.c
Outdated
| if (retval == 0) retval = -1; | ||
| } | ||
| break; | ||
| case WAIT_ABANDONED: // shouldn't happen to a process |
There was a problem hiding this comment.
if this shouldn't happen, i feel like we should be erroring out or something here so that it straight up dies.
There was a problem hiding this comment.
OK, I have moved this so it fails.
Closed
|
Yes, from initial testing this seems to fix the Corral issue. |
|
@kulibali I'm wary of that "this can't happen" fall through. I'd prefer to not have that. Better to have it complain, die or something. Other than that, I think this is good. |
SeanTAllen
changed the title
SeanTAllen
added
the
changelog - fixed
SeanTAllen
approved these changes
May 13, 2020
|
My bad! :( Me trying windows things... |
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.
I think there are a couple of problems with recent changes to
ponyint_win_process_wait. I should have caught them in review.Changing the timeout value to 0 means that
WaitForSingleObjectwon't block, so we can't close the handle in that case. Also 0x80 is the return codeWAIT_ABANDONEDwhich doesn't apply to processes. We should be checking forWAIT_TIMEOUT(0x102) which means the process hasn't exited yet. Also we need to make sure that retval is not 0 or 1 in an error case whereGetLastError()returns 0 for some reason.