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

Add support for stream cloning #203

Closed
wants to merge 1 commit into from
Closed

Add support for stream cloning #203

wants to merge 1 commit into from

Conversation

goodboy
Copy link
Owner

@goodboy goodboy commented Mar 2, 2021

Leverage trio's built-in rx channel cloning with our ReceiveStream to enable multi-consumer usage.

if self._shielded:
log.warning(f"{self} is shielded, portal channel being kept alive")
return

# close the local mem chan
rx_chan.close()
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This likely needs to be put above.

Thanks to @richardsheridan for catching 🏄🏼

@goodboy
Copy link
Owner Author

goodboy commented Apr 5, 2021

Heh, so this doesn't even work remotely as planned 😂

Turns out receive mem chans do not support broadcasting in trio:

The clones all share the same underlying channel. Whenever a clone receive()s a value, it is removed from the channel and the other clones do not receive that value. If you want to send multiple copies of the same stream of values to multiple destinations, like itertools.tee(), then you need to find some other solution; this method does not do that.

This actually ended up being a source of major confusion in pikers/piker#158 where this PR's change to ReceiveStream.aclose() was actually preventing far end producer cancellation and then a very hard to debug back-pressure instigated lockup 😂

So basically I think we need to drop this idea wholesale (at least if we're sticking with clone as it has meaning in trio) and instead venture into what we're actually after: broadcast channels as in python-trio/trio#987.

@goodboy
Copy link
Owner Author

goodboy commented Apr 30, 2021

See also #53 for further discussion on how we don't want trio's .clone() semantics (since it isn't useful cross-process).

@goodboy goodboy closed this Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant