Transport cleaning #215
Merged
Transport cleaning #215
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 change some super old (and bad) code from the project's very early days. For some redic reason i must have thought masking `trio`'s internal stream / transport errors and a TCP EOF as `StopAsyncIteration` somehow a good idea. The reality is you probably want to know the difference between an unexpected transport error and a simple EOF lol. This begins to resolve that by adding our own special `TransportClosed` error to signal the "graceful" termination of a channel's underlying transport. Oh, and this builds on the `msgspec` integration which helped shed light on the core issues here B)
Since we currently have no real "discovery protocol" between process trees, the current naive approach is to check via a connect and drop to see if a TCP server is bound to a particular address during root actor startup. This was a historical decision and had no real grounding beyond taking a simple approach to get something working when the project was first started. This is obviously problematic from an error handling perspective since we need to be able to avoid such quick connect-and-drops from cancelling an "arbiter"'s (registry actor's) channel-msg loop machinery (which would propagate and cancel the actor). For now we map this particular TCP error, which gets remapped by `trio` as a `trio.BrokenResourceError` to our own internal `TransportClosed` which is swallowed by channel message loop processing and indicates a graceful teardown of the far end actor.
goodboy
force-pushed
the
transport_cleaning
branch
from
df9e2b3 to
55760b3
Compare
|
Omg finally 😂 |
guilledk
approved these changes
Jul 6, 2021
|
Gonna rerun the suite a few more times. Pretty sure it's just debugger tests again (which have a bunch of non-determinism) until #220 lands. |
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.
Pulls out some transport related finessing into a new small patchset for other PRs (like #209) to build on.
Mostly trying to get these lower level changes in and see it work in CI.