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

Implement Clone for watch::Sender to make it multi-producer #4809

Closed
madadam opened this issue Jul 6, 2022 · 9 comments · Fixed by #6388
Closed

Implement Clone for watch::Sender to make it multi-producer #4809

madadam opened this issue Jul 6, 2022 · 9 comments · Fixed by #6388
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-sync Module: tokio/sync

Comments

@madadam
Copy link
Contributor

madadam commented Jul 6, 2022

Currently watch is single-producer but looking at the source of Sender, it seems it would be trivial to change it to multi-producer - just slap a #[derive(Clone)] on it. I think this would make it more useful as it would be possible to watch multiple sources. It's already possible to put the Sender into Arc, but it's inefficient because it's essentially an Arc in an Arc.

Would the tokio maintainers be interested in a PR to add this?

@madadam madadam added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Jul 6, 2022
@Darksonn Darksonn added the M-sync Module: tokio/sync label Jul 6, 2022
@Darksonn
Copy link
Contributor

Darksonn commented Jul 6, 2022

Sounds good to me, but the impl Drop for Sender will need to be updated.

@madadam
Copy link
Contributor Author

madadam commented Jul 6, 2022

Good point, I haven't considered it. Guess that makes it not quite as trivial as I though...

@madadam
Copy link
Contributor Author

madadam commented Jul 6, 2022

I think to properly implement this, we would have to track the number of senders instead of just a single bit indicating closed channel. Thus we won't be able to pack the information about whether the channel is closed into the same atomic as the version and we would need a separate atomic. That would make the state loading operation no longer atomic (because it would require two separate atomic loads). I'm not sure what implication that would have. I think it should be fine because the two values are independent, but I might be missing something.

@hawkw
Copy link
Member

hawkw commented Jul 6, 2022

FWIW, the number of Receivers is already tracked in a separate atomic...

/// Tracks the number of `Receiver` instances.
ref_count_rx: AtomicUsize,
but, that isn't read in operations that also care about the version.

Another potential option, which would maintain the atomicity of operations on the version/sender closedness state, would be to use half of a a word to store the sender refcount, and the other half to store the version. I think 32 bits of ref-count would be plenty (nobody's gonna clone a sender 4,294,967,295 times, right...?), but we might want to be able to handle the version wrapping around. And, we would probably need to change the state to be an explicitly-64-bit atomic --- 65,535 clones/versions on 32-bit platforms seems kinda cramped...

@madadam
Copy link
Contributor Author

madadam commented Jul 12, 2022

Yeah, that might work although I'm not sure 32 bits is enough for the version (but maybe some wrap-around tricks can be used). But I wonder, do they actually need to be packed into a single atomic? Is there some other reason for it apart from space saving?

@Darksonn
Copy link
Contributor

If the counter is only used to know when all senders are gone, then it does not need to be packed together.

@madadam
Copy link
Contributor Author

madadam commented Jul 12, 2022

But currently the flag indicating whether the channel is closed is packed together with the version. So I wonder whether that's just to save some space or whether there is a deeper reason.

@Darksonn
Copy link
Contributor

Packing the closed flag together with the version is indeed done for space reasons.

@bouk
Copy link

bouk commented Aug 16, 2023

I've started work on this feature here: #5936

Comments welcome, especially if there's other features/methods that need to be added while I'm at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-sync Module: tokio/sync
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants