Fix failed initialization bug in Process Monitor #4043
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
ponylang-main
added
the
discuss during sync
Previously, when a process failed to start up, it was possible to get a report that the process had started up and exited with the error code 0 which normally signals success. The problem comes from some rather poor state handling in the `process` package. This commit doesn't fix the larger issue of all kinds of weirdness around state handling but it does address the specific bug that was most likely to be triggered in our unit tests. The flow in its most extreme form was: - ProcessMonitor is created in test runner - Long test is setup - Dispose when done is setup to send dispose to ProcessMonitor - ProcessMonitor never successfully starts a Process so its child is _ProcessNone - notify gets called with `failed` - `dispose` is sent to the ProcessMonitor - ProcessMonitor extracts exit code from _ProcessNone which was 0 <-- o boy - "Success exit code" of 0 is passed as argument to `dispose`
SeanTAllen
force-pushed
the
invalid-initialization-not-handled
branch
from
e64941c to
a358524
Compare
SeanTAllen
added
the
changelog - fixed
mfelsche
reviewed
Feb 24, 2022
Co-authored-by: Matthias Wahl <[email protected]>
I forgot to add to commit
|
This is ready for another review @mfelsche. The Windows Docker Image failures are entirely a GH issue which amusingly, from looking at the error message, I understand intimately. |
mfelsche
approved these changes
Feb 24, 2022
ponylang-main
removed
the
discuss during sync
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.
Previously, when a process failed to start up, it was possible to get a report
that the process had started up and exited with the error code 0 which
normally signals success.
The problem comes from some rather poor state handling in the
processpackage.This commit doesn't fix the larger issue of all kinds of weirdness around state
handling but it does address the specific bug that was most likely to be
triggered in our unit tests.
The flow in its most extreme form was:
faileddisposeis sent to the ProcessMonitordispose