Prevent race conditions in process teardown sequences and use of Execution in the body closure #88
Conversation
This file contains hidden or 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
…hrows If the body closure given to withAsyncTaskCleanupHandler throws, its cleanup handler is called twice: once in the catch block where body is called, and then again in the task group task once the Task.sleep throws due to cancellation, which swallows the error and then continues to call the handler as well. That results in the teardown sequence being invoked twice, as well as the teardown sequence being invoked for non-error cases. This patch ensures the cleanup handler is invoked in failure cases only, and only once. Closes #80
Process termination monitoring is started asynchronously with the closure which receives the Execution, meaning any use of Execution in the body to send signals, etc., may send them to a pid which no longer refers to the original process. The cleanup handler may also run after the process has already terminated.
jakepetroules
requested review from
iCharlesHu
and removed request for
iCharlesHu
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.
This combines #81 and #87 into a single PR as it may be more difficult to understand the value of these fixes independently.
This patch ensures process termination monitoring is started after the body closure (which receives the Execution) called as part of the public API. This ensures that any use of Execution to send signals, etc., will send them to the right pid as we are guaranteed not to reap it until body returns. Additionally, we ensure the cleanup handler which runs the termination sequence is run ONLY once, and only run when the body closure throws or when the parent task is cancelled. In combination with the aforementioned process termination monitoring change, this ensures the termination sequence is similarly always sent to the right pid since it will always occur before the pid is reaped.
Closes #80
Closes #86