Ensure actors de-register with arbiter when cancelled during infitinite streaming.#143
Merged
Ensure actors de-register with arbiter when cancelled during infitinite streaming.#143
Conversation
This truly reproduces #141. It turns out the problem only occurs when we're cancelled in the middle of consuming "infinite streams". Good news is this tests a lot of edge cases :)
Owner
Author
|
Alright there's the expected failures. |
Trio will kill subprocesses via `Process.__aexit__()` using a `finally:` block (which, yes, will get triggered on cancellation) so we avoid that until true process "tear down" since subactors do many things during graceful shutdown (such as de-registering from the name discovery system). Oddly this only seems to be an issue during cancellation of infinite stream consumption. Resolves #141
Owner
Author
|
@guilledk just waiting for your "ok" I think 👍 |
guilledk
reviewed
Aug 4, 2020
Collaborator
guilledk
left a comment
guilledk
left a comment
There was a problem hiding this comment.
Just a small nitpick hehe
Owner
Author
|
@ryanhiebert @guilledk appreciate the review when ready :) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 is a (finally) reproduction of the issue I discovered manually in #141; it was likely a regression (caused by #128) but it's hard to know since we didn't have tests for this until now 😄
It turns out the issue only appears if you are in the middle of consuming and infinite stream and are cancelled.
The cancellation causes the
trio.Processbackend to kill the process before it can cleanup resulting in stale entries in the arbiter registry.This first set of commits will demonstrate the issue (that I want to see also in CI) and then I will push up the solution commit 🏄