Implement `Sync` for `mpsc::Sender` by ibraheemdev · Pull Request #111087 · rust...

r? @joshtriplett

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

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

@rustbot label -T-libs

This PR makes mpsc::Sender implement Sync.

@rfcbot fcp merge

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

Concerns:

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.

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

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

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 ;)

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

@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

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?

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

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);
    });
})

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

@rfcbot resolve failing-tests

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