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

Consider adding impl Sync for Sender? #46

Open
silvanshade opened this issue Jun 14, 2024 · 1 comment
Open

Consider adding impl Sync for Sender? #46

silvanshade opened this issue Jun 14, 2024 · 1 comment

Comments

@silvanshade
Copy link

Is there any particular reason why Sender doesn't impl Sync?

I have a use case where I need this impl and it seems like Sender should trivially satisfy its definition so I'm wondering if I'm overlooking something.

The reason why I think it should be okay is because all of the Sender methods take self by-value, so &Sender<T>: Send (trivially) because you can't actually do anything with the Sender in that case, and &Sender<T>: Send implies Sender<T>: Sync.

As for the use case, I have an algorithm where I am using a DashMap<T, Option<Vec<oneshot::Sender<()>>>> to solve an ordering problem, where the map is used as a ledger to determine whether certain tasks have already completed, and to notify pending tasks when ready using the oneshot::Sender<()>.

The only way I can make this type-check (without rolling my own unsafe impl Sync wrapper) is if I wrap the channels in a Mutex like this: DashMap<T, Option<Vec<Mutex<oneshot::Sender<()>>>>>.

But this seems kind of pointless because I never use the Mutex for locking (in fact it's not possible because it doesn't return the channel by-value): instead I have to immediately consume the Mutex with .into_inner and then call send on the channels. So the Mutex is only used to satisfy the Sync requirement.

I suppose maybe the reason the Sync impl wasn't already added is because it seems useless/trivial, and indeed the constraint for use here with DashMap is a little misleading: The reason the algorithm I describe works is because I remove the entry from the map (by-value), not using a typical lookup (which wouldn't allow me to send on the channel).

So this is sort of a case of the the DashMap API being overly conservative, but in any case, if there's no reason why Sync is problematic, it would be useful to have.

I just noticed #26 (comment). The above at least provides a real-world use.

@madsmtm
Copy link

madsmtm commented Oct 29, 2024

See also rust-lang/rust#111087.

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

No branches or pull requests

2 participants