search

smol-rs / async-broadcast

Async broadcast channels
Apache License 2.0
151 stars 26 forks source link

Change Receiver::clone to truly duplicate a Receiver's state #24

Closed danieldg closed 2 years ago

danieldg commented 2 years ago

This changes Receiver::clone to do what you probably expect it to do: produce a Receiver with the same messages queued.

yoshuawuyts commented 2 years ago

@danieldg Can you elaborate on what the new behavior you're introducing is, and why you have a need for it? If the existing Clone behavior is incorrect, we should replace it and publish a new major version of async-broadcast. Having two subtly different ways to clone a Receiver is unlikely something we would want.

Right now I don't understand what problem this PR is trying to address, which makes it hard to review in any meaningful way. If you could elaborate, that'd be much appreciated!

danieldg commented 2 years ago

@danieldg Can you elaborate on what the new behavior you're introducing is, and why you have a need for it? If the existing Clone behavior is incorrect, we should replace it and publish a new major version of async-broadcast. Having two subtly different ways to clone a Receiver is unlikely something we would want.

Right now I don't understand what problem this PR is trying to address, which makes it hard to review in any meaningful way. If you could elaborate, that'd be much appreciated!

@yoshuawuyts Look at the two tests that the first patch in this series introduces: they do the same thing, but use a different clone function and produce different behavior.

Quick description: cloning a Receiver that currently has items pending should (in my intuition) also clone the items that are waiting. Otherwise, the clone isn't really a clone, it's just a new receiver attached to the same channel.

The use case that drives this is allowing a receiver to, at any point without coordination with senders in other threads, take some of its internal state and hand it off to someone else along with a channel that will get (broadcast) state updates. If only the current "new receiver" clone is available, the receiver needs to clone and then be sure that it has drained the original receiver first (applying any pending updates) before sending it to avoid having a missed update. This is strictly worse because there is still a race when you get updates that were sent between the clone and the drain (which are applied twice instead of zero times).

yoshuawuyts commented 2 years ago

@danieldg ah, I see. Thanks for explaining! Imo we should just update the existing Clone impl to also clone the contents of the receiver it's cloned on, for the exact reasons you mention.

Imo this shouldn't need any further changes to the public API, and could probably pass as a semver patch.

mmstick commented 2 years ago

Is this blocked on anything that I can help push through?

zeenix commented 2 years ago

Is this blocked on anything that I can help push through?

IIRC @danieldg did this work for zbus and since we ended up not making use of this there, we both forgot about this PR. I've merged this work now. Thanks for reminding. :)

mmstick commented 2 years ago

Awesome. I have use for this as a component for a graceful shutdown mechanism