-
Notifications
You must be signed in to change notification settings - Fork 14k
oneshot Channel
#143741
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
base: main
Are you sure you want to change the base?
oneshot Channel
#143741
Conversation
|
Maybe want to ask the unresolved questions at the ACP thread? To make sure it's seen by those already involved. (I'll take a closer look later) |
Because But mpmc is multiple producer multiple consumer, so it doesn't matter how many threads For this one, they can both be unconditionally |
|
Thank you for the answer! Please let me know if I'm interpreting this right: If all methods where synchronization must occur consume |
c7e668b to
1a515fd
Compare
|
I haven't gotten a chance to look into this unfortunately r? libs |
|
r? libs (based on this comment) |
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #147928) made this pull request unmergeable. Please resolve the merge conflicts. |
|
It's been a little while so Feel free to reroll if you want but you've reviewed a lot of sync module things. |
This comment has been minimized.
This comment has been minimized.
|
Using the full |
|
@joboet I think I'm in agreement, though the main reason I did it like so was because of this comment from the original ACP: rust-lang/libs-team#610 (comment) I believe that the goal was to reuse as much code as possible? I do think a |
|
Cc @the8472 from the above-linked comment |
|
As I wrote in the comment we didn't discuss implementation too much, we just assumed crossbeam's code would be suitable. If that's inefficient for a oneshot channel and all the desired API surface can be implemented in a better way then we can replace the internals. But landing things incrementally is fine, so that can happen in a separate PR. |
5e564f5 to
d2185fc
Compare
This comment has been minimized.
This comment has been minimized.
|
Sorry for the delay, I completely forgot about this! |
library/std/src/sync/oneshot.rs
Outdated
| #[unstable(feature = "oneshot_channel", issue = "143674")] | ||
| impl<T> fmt::Debug for Sender<T> { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.pad("Sender { .. }") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the builder methods for this – that also makes the alternative formatting work.
| f.pad("Sender { .. }") | |
| f.debug_struct("Sender").finish_non_exhaustive() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, sorry I was just copying the mpmc impl (that should probably be updated too)
library/std/src/sync/oneshot.rs
Outdated
| /// # #![feature(oneshot_channel)] | ||
| /// # use std::sync::oneshot; | ||
| /// # use std::thread; | ||
| /// # |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally don't hide imports and feature declarations in doc-examples – it's useful for users to know what a specific type is.
library/std/src/sync/oneshot.rs
Outdated
| /// # Safety | ||
| /// | ||
| /// Since the only methods in which synchronization must occur take full ownership of the | ||
| /// [`Sender`], it is perfectly safe to share a `&Sender` between threads (as it is effectively | ||
| /// useless without ownership). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your motivation for making this a doc-comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops
d2185fc to
8aa907f
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
8aa907f to
2cc6ca8
Compare
The `oneshot` channel is gated under the `oneshot_channel` feature.
Tests inspired by tests in the `oneshot` third-party crate.
Signed-off-by: Connor Tsui <[email protected]>
2cc6ca8 to
2606400
Compare
Tracking Issue: #143674
This PR adds an experimental
oneshotmodule.Before talking about the API itself, I would prefer to get some of these questions below out of the way first. And as discussed in the ACP it would be
Unresolved Questions
Why exactly is it okay forSenderto beSync? Or basically, how do we boil down the discussion in ImplementSyncformpsc::Sender#111087 into a comment for theunsafe impl<T: Send> Sync for Sender<T> {}?Why ismpsc::Receiver!Syncbutmpmc::ReceiverisSync? Shouldoneshot::ReceiverbeSyncor not?is_readymethod as proposed in the tracking issue? If so, then the surface of this PR would likely need to increase to add apub(crate) fn is_disconnectedmethod tompmc(might even be a good idea to add that to all 3 channel flavors).mpmc, or should it be a wrapper around the internal crossbeam implementation?SenderandReceiveroperations be methods or associated methods? Sosender.send(msg)orSender::send(sender, msg)? The method syntax is more consistent with the rest of the ecosystem (namelytokio)