`oneshot` Channel

[connortsui20]

Tracking Issue: https://github.com/rust-lang/rust/issues/143674

This PR adds an experimental oneshot module.

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

• [x] ~~Why exactly is it okay for Sender to be Sync? Or basically, how do we boil down the discussion in https://github.com/rust-lang/rust/pull/111087 into a comment for the unsafe impl<T: Send> Sync for Sender<T> {}?~~
• [x] ~~Why is mpsc::Receiver !Sync but mpmc::Receiver is Sync? Should oneshot::Receiver be Sync or not?~~
• [ ] Should this PR try to add an is_ready method as proposed in the tracking issue? If so, then the surface of this PR would likely need to increase to add a pub(crate) fn is_disconnected method to mpmc (might even be a good idea to add that to all 3 channel flavors).
• [ ] In a similar vein to the previous question, should the first internal implementation simply be a wrapper around mpmc, or should it be a wrapper around the internal crossbeam implementation?
• [ ] Should the Sender and Receiver operations be methods or associated methods? So sender.send(msg) or Sender::send(sender, msg)? The method syntax is more consistent with the rest of the ecosystem (namely tokio)

[rustbot]

r? @tgross35

rustbot has assigned @tgross35. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[tgross35]

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)

[RustyYato]

Why exactly is it okay for Sender to be Sync? Or basically, how do we boil down the discussion in https://github.com/rust-lang/rust/pull/111087 into a comment for the unsafe impl<T: Send> Sync for Sender<T> {}?

Why is mpsc::Receiver !Sync but mpmc::Receiver is Sync? Should oneshot::Receiver be Sync or not?

Because mpsc is multiple producer single consumer, and does the receiver operation via &self, so it would be incorrect to allow recv to be shared across threads because that would allow two threads to recv at the same time, which isn't allowed for a mpsc.

But mpmc is multiple producer multiple consumer, so it doesn't matter how many threads recv at the same time.

For this one, they can both be unconditionally Sync, since there is no API to go from &Sender<T> or &Reciever<T> to &T. (even via traits like Debug or Clone). In fact it looks like &Sender<T> and &Receiver<T> are entirely useless (only a Debug impl, which doesn't provide any information), which is just like Exclusive.. This is more evidence that both Sender<T> and Receiver<T> can be unconditionally Sync

[connortsui20]

Thank you for the answer!

Please let me know if I'm interpreting this right: If all methods where synchronization must occur consume self, we can say that both Sender and Receiver are Sync. For example, if threads share a &oneshot::Receiver, then none of them can call recv anyways. And this would still be compatible with the proposed is_ready method discussed in the tracking issue.

[tgross35]

I haven't gotten a chance to look into this unfortunately

r? libs

[connortsui20]

r? libs (based on this comment)

[rustbot]

This PR was rebased onto a different master 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.

[bors]

:umbrella: The latest upstream changes (presumably #147928) made this pull request unmergeable. Please resolve the merge conflicts.

[tgross35]

It's been a little while so r? @joboet

Feel free to reroll if you want but you've reviewed a lot of sync module things.

[rustbot]

This PR was rebased onto a different master 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.

[joboet]

Using the full mpmc implementation is a bit overkill here in my opinion. I think we could do better by using a OnceLock wrapped by an Rc here and adding a wait_timeout_force method to Once next to the existing wait_force.

[connortsui20]

@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: https://github.com/rust-lang/libs-team/issues/610#issuecomment-3051625179

I believe that the goal was to reuse as much code as possible? I do think a OnceLock impl would probably be better in the long-term but this at least is a lot easier to reason about w.r.t the API

[tgross35]

Cc @the8472 from the above-linked comment

[the8472]

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.

[rustbot]

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.

[connortsui20]

Sorry for the delay, I completely forgot about this!

[rustbot]

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.

[rustbot]

:warning: Warning :warning:

• Some commits in this PR modify submodules.

  If this was not intentional, see I changed a submodule on accident in the rustc dev guide.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

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.

[connortsui20]

@joboet Do you think we should we namespace the other name conflicting Debug implementations? I think that either we should namespace everything that conflicts among mpsc, mpmc, and oneshot, or we maybe shouldn't for any of them...

I've taken out the oneshot:: in the Debug impl of the error types in the latest commit but I can easily add it back.

[rust-log-analyzer]

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

.................................F
======== tests/rustdoc-gui/headers-color.goml ========

[ERROR] `tests/rustdoc-gui/headers-color.goml`: headers-color output:
Waiting failed: 30000ms exceeded
stack: TimeoutError: Waiting failed: 30000ms exceeded
    at new WaitTask (/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/cjs/puppeteer/common/WaitTask.js:50:34)
    at IsolatedWorld.waitForFunction (/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/cjs/puppeteer/api/Realm.js:25:26)
    at CdpFrame.waitForFunction (/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/cjs/puppeteer/api/Frame.js:561:43)
    at CdpFrame.<anonymous> (/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/cjs/puppeteer/util/decorators.js:98:27)
    at CdpPage.waitForFunction (/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/puppeteer-core/lib/cjs/puppeteer/api/Page.js:1366:37)
    at runAllCommands (/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/browser-ui-test/src/index.js:417:28)
    at async innerRunTestCode (/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/browser-ui-test/src/index.js:692:21)
    at async innerRunTests (/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/browser-ui-test/src/index.js:630:17)
    at async runTests (/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/node_modules/browser-ui-test/src/index.js:820:27)



<= doc-ui tests done: 33 succeeded, 111 failed, 0 filtered out

[connortsui20]

@bors retry

[bors]

@connortsui20: :key: Insufficient privileges: not in try users

[the8472]

Restarted PR CI.

[connortsui20]

@rustbot ready

Though https://github.com/rust-lang/rust/pull/143741#issuecomment-3620825013 is still unresolved

[tbu-]

Sorry for the delay. Apart from the formatting nits, this looks fine. In addition to the comments, may I suggest using f.write_str("...") instead of "...".fmt(f) in general?

This makes Display implementations worse, they'll ignore formatting specifiers:

use std::fmt;

struct A;
struct B;

impl fmt::Display for A {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        "a".fmt(f)
    }
}

impl fmt::Display for B {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.write_str("b")
    }
}

fn main() {
    println!("{:>10}", A);
    println!("{:>10}", B);
}

outputs

         a
b

[connortsui20]

@tbu This is the Debug implementation though? And I believe @joboet said that specifically to distinguish between the Debug and Display impls.

[joboet]

Sorry for the delay. Apart from the formatting nits, this looks fine. In addition to the comments, may I suggest using f.write_str("...") instead of "...".fmt(f) in general?

This makes Display implementations worse, they'll ignore formatting specifiers:

use std::fmt;

struct A;
struct B;

impl fmt::Display for A {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        "a".fmt(f)
    }
}

impl fmt::Display for B {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.write_str("b")
    }
}

fn main() {
    println!("{:>10}", A);
    println!("{:>10}", B);
}

outputs

         a
b

Mmmh, that's right. pad is correct, though.