Implement Sync for mpsc::Sender

[ibraheemdev]

mpsc::Sender is currently !Sync because the previous implementation contained an optimization where the channel started out as single-producer and was dynamically upgraded on the first clone, which relied on a unique reference to the sender. This optimization is one of the main reasons the old implementation was so complex and was removed in #93563. mpsc::Sender can now soundly implement Sync.

Note for any potential confusion, this chance does not add MPMC behavior. This only affects the already Send + Clone sender, not receiver.

It's technically possible to rely on the !Sync behavior in the same way as a PhantomData<*mut T>, but that seems very unlikely in practice. Either way, this change is insta-stable and needs an FCP.

@rustbot label +T-libs-api -T-libs

[rustbot]

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[ibraheemdev]

@rustbot label -T-libs

[BurntSushi]

This PR makes mpsc::Sender implement Sync.

@rfcbot fcp merge

[rfcbot]

Team member @BurntSushi has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

Concerns:

• failing-tests resolved by Implement Sync for mpsc::Sender #111087 (comment)
• why-not-sync resolved by Implement Sync for mpsc::Sender #111087 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[BurntSushi]

Are we certain that the original reason for a !Sync impl was because of an internal optimization? Can we go back and find where the original API was sketched out and see if there were any other reasons for the !Sync impl?

@rfcbot concern why-not-sync

[BurntSushi]

Right now there are some failing tests. We should check to make sure those themselves don't raise any other concerns.

@rfcbot concern failing-tests

[goffrie]

Are we certain that the original reason for a !Sync impl was because of an internal optimization? Can we go back and find where the original API was sketched out and see if there were any other reasons for the !Sync impl?

I dug up #11111 - which made senders (and other channel types) !Freeze (because Sync hadn't been invented yet). The only comment was:

#[no_freeze] // technically this implementation is shareable, but it shouldn't
             // be required to be shareable in an arc
pub struct SharedChan<T> {

I doubt that the decision was considered in-depth - my guess is that it was done just to keep options open for future implementation changes. But maybe @alexcrichton remembers ;)

[goffrie]

Ah, I found #42397 as well which made SyncSender Sync; there's some interesting discussion in there.

[BurntSushi]

@goffrie Thanks for digging up #42397. That gives me a lot of confidence that these types weren't Sync primarily due to implementation details. So I can at least resolve that concern.

@rfcbot resolve why-not-sync

[dtolnay]

the previous implementation contained an optimization where the channel started out as single-producer and was dynamically upgraded on the first clone, which relied on a unique reference to the sender. This optimization is one of the main reasons the old implementation was so complex and was removed in #75194.

This isn't the right PR. Is it #93563?

[ibraheemdev]

@dtolnay yeah that's the one, not sure how #75194 ended up in there.

[ibraheemdev]

I guess the big question is why implement Sync if it's already Send and Clone, and the only real reason is ergonomics. For example, it makes patterns with thread::scope possible without extra cloning clutter:

let (tx, rx) = std::sync::mpsc::channel();
 
 // std::thread::scope(|s| {
//     let tx1 = tx.clone();
//     s.spawn(move || {
//         tx1.send(1);
//     });
//
//     let tx2 = tx.clone();
//     s.spawn(move || {
//         tx2.send(2);
//     });
// })

std::thread::scope(|s| {
    s.spawn(|| {
        tx.send(1);
    });
    
    s.spawn(|| {
        tx.send(2);
    });
})

[BurntSushi]

@rfcbot resolve failing-tests

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[dtolnay]

@bors r+

[bors]

📌 Commit 4ceca09 has been approved by dtolnay

It is now in the queue for this repository.

[ibraheemdev]

cc @thomcc because they expressed thoughts on this.

[thomcc]

My thoughts (I'm assuming you're referring to this tweet) are just that it's a useful optimization sometimes to be able to do things like that (even the lower-complexity version where we just check an Arc's reference count somewhere).

If we don't want to do it in the future, then IMO there's no harm in accepting this. My tweet was definitely not about this stabilization in particular -- which I'm not opposed to (std should be willing to prioritize ergonomics over having the best possible implementation¹, as the best possible implementation for a given situation will almost always depend on that situation's specifics anyway)

Footnotes

1. Besides, nobody has come forth with an optimization here that would leverage this anyway. ↩

[bors]

⌛ Testing commit 4ceca09 with merge e0ba2d0...