Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

End of channel fixes for streams and broadcasting #278

Merged
merged 8 commits into from
Dec 16, 2021

Conversation

goodboy
Copy link
Owner

@goodboy goodboy commented Dec 16, 2021

This patch set is factored out of #257 and corrects a bunch of previously unknown bugs to do with stream closure and broadcast receiver usage around streams as provided by MsgStream.subscribe().

I decided to pull the work out of the other PR because this is notably different work despite discovering it via the original cached-inter-actor stream test that found it.

Summary:

  • add a MsgStream._closed: bool for internal closure tracking and raise trio.ClosedResourceError in .send() if set
  • always set MsgStream._eoc: bool inside .aclose()
  • always set ._eoc when a 'stop' message is received
  • add .eoc: bool and .cancelled: bool tracking inside BroadcastState, set these flags on handling of the appropriate trio exceptions as well as wake all broadcast consumers on each signal to avoid hangs when a wrapped recv channel has closed via trio.EndOfChannel
  • add a full task-broadcast inter-actor streaming test that verifies all of the above is now correct and graceful closure of the stream results in closure of all consumer tasks which entered .subscribe()

Without this wakeup you can have tasks which re-enter `.receive()`
and get stuck waiting on the wakeup event indefinitely. Whenever
a ``trio.EndOfChannel`` arrives we want to make sure all consumers
at least know about it and don't block. This previous behaviour was
basically a bug.

Add some state flags for tracking if the broadcaster was either
cancelled or terminated via EOC mostly for testing and debugging
purposes though this info might be useful if we decide to offer
a `.statistics()` like API in the future.
This actually catches a lot of bugs to do with stream termination and
``MsgStream.subscribe()`` usage where the underlying stream closes from
the producer side. When this passes the broadcaster logic will have to
ensure non-lossy fan out semantics and closure tracking.
Avoids the issue noted in
python-trio/trio-typing#50
to keep CI green.
@goodboy goodboy added bug Something isn't working streaming testing labels Dec 16, 2021
@goodboy goodboy force-pushed the end_of_channel_fixes branch from f83d6c4 to 325e550 Compare December 16, 2021 22:30
@goodboy goodboy merged commit 2d6fbd5 into master Dec 16, 2021
@goodboy goodboy deleted the end_of_channel_fixes branch December 16, 2021 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working streaming testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant