std::sync::mpsc::SharedSender

[seanmonstar]

Add a SharedSender to std::sync::mpsc that implements Sync.

Rendered

[sfackler]

I would personally prefer this to a second channel constructor function, but discoverability might be hard?

[seanmonstar]

Er, I forgot to add description as to its downside: if you at the beginning you want a shared sender, this is just wasted operations going from oneshot -> shared, and bumping the internal counter by one since the Sender will be dropped and drop the counter.

[BurntSushi]

I'm not terribly familiar with the internals of std::sync, but what is the specific reason why the existing channels can't be made Sync? (It looks like you explained it in the opening paragraph, but I didn't quite grok it.)

[seanmonstar]

@BurntSushi a Sender contains a Flavor internally, starting as Oneshot, then going to Stream if used more than once. Also, the Flavor will change to Shared if the Sender is cloned.

Reasons: Flavor::Oneshot and Flavor::Stream are safe as an spsc queue, but not mpsc. So once cloned, they upgrade themselves to Flavor::Shared which uses a safe mpsc queue. If you were to do Arc::new(tx), and then clone the Arc and pass it to different threads, you'd have an spsc queue being sent to from multiple threads.

[alexcrichton]

I think that it's probably worth exploring adding Sync to the existing sending half of the channel before resorting to expanding the API surface. The addition won't be quite as much of a "drop in replacement" as this currently is, but it's in theory possible for a sender to detect concurrent access on a oneshot or stream channel and perform the upgrade in a CAS-like fashion, avoiding any extra types in play.

[seanmonstar]

@alexcrichton huh, I didn't consider that it'd be possible to detect concurrent access. I suppose with a cas, if false, then another thread sent at the exact same time, and it's time to upgrade to shared?

[alexcrichton]

Yeah I imagine something along those lines could be done. Some extra synchronization will have to happen no matter what (and it may be quite difficult to bolt on the already large amount going on), but in theory if concurrent access could be detected in a lightweight manner a CAS-like thing could be done to upgrade the channel, and the first thread to succeed ends up creating the new shared channel while all other threads fail the CAS and then start re-using the shared channel from the initial thread.

Again, though, this implementation would be significantly more complicated than the outline you've got in this RFC. The benefit, however, are that we don't need another type of sender which would be nice!

[seanmonstar]

@alexcrichton it might be nice not having an additional Sender, but at the cost of paying extra when you really only need a spsc-like queue. I'd be content with an auto-upgrade here if there were also an option pick the flavor at the beginning, so that we can pay for what you use and all that.

[BurntSushi]

@seanmonstar Thanks for the explanation! That was helpful. Two more questions (again, please keep in mind that my familiarity with the std::sync implementation is pretty low, so please slap me if I misspeak!)

1. How much impact does a single CAS operation have on the spsc queue?
2. What does the API look like if we let the caller choose the type of channel before hand and allow it to be auto upgradeable? Can the additional type be removed? (I'm not sure I see how to remove the type, which makes this option less attractive IMO.)

[seanmonstar]

@BurntSushi I tried to make a couple queues with as few operations as possible, to make a benchmark. I can't say if the spsc_plus_cas version correctly uses the cas, so it'd be great if someone could sanity check that.

---

If sane, then this benchmark gave me these results on my crappy machine:

running 2 tests
test bench_spsc          ... bench:          71 ns/iter (+/- 45)
test bench_spsc_plus_cas ... bench:          87 ns/iter (+/- 43)

test result: ok. 0 passed; 0 failed; 0 ignored; 2 measured

[TyOverby]

I think that this will be really confusing for people because of the sync_channel that is already in the module.

If you asked me which channel implements Sync: "sync_channel" or "shared_channel", I'd be confused, but pick "sync_channel" because it has "sync" right in the name.

[seanmonstar]

@TyOverby I agree that the naming of sync_channel is unfortunate. Even without this change, it's still confusing to tell people "You need a Sync Sender. No no, not SyncSender..."

[BurntSushi]

OK, so let me see if I've got the situation right. I think we have some variation of the following four choices:

1. Leave the API as is. The fact that SyncSender is not Sync is a little confusing and channels cannot actually be Sync. Which is a little sad.
2. Make existing channels Sync with an unavoidable performance penalty.
3. Increase the API surface with at least one additional type and constructor. (And the API is still confusing as in (1).)
4. Choose (1) or (3) and use deprecation to rename things that are unclear.

Is there a choice I've left out? None of them look particularly appealing to me.

I guess the next thing to look at are use cases for making channels Sync. For me personally, the big one I can think of is better ergonomics when using scoped threads. Are there other prominent use cases?

[seanmonstar]

The original motivation I had for writing this RFC was because it's a common stumbling block when people try to use hyper's Server. The Server takes a Handler, where trait Handler: Send + Sync {}

So the following situation happens a lot:

let (tx, rx) = channel();
Server::http(addr).listen(move |req, res| {
    tx.send("request!"); // tx does not implement Sync
});

With the unfortunate solution being:

let (tx, rx) = channel();
let tx = Mutex::new(tx);
Server::http(addr).listen(move |req, res| {
    tx.lock().unwrap().send("request!"); 
});

[BurntSushi]

I am personally pretty ambivalent here, and as such, I'm not sure exactly how to move it forward. Part of me thinks a tiny perf hit might be acceptable, especially if we get a more sane API, because wrapping channels in a mutex is a bummer. I will say though that @alexcrichton thinks this may be tricky to implement!

[seanmonstar]

Likewise, I don't strongly prefer either option over the other, I just want to stop wrapping senders in mutexs.

---

• SharedSender
  • easier to implement
  • pay for what you use (also, I haven't heard anyone say anything about my implementation benchmark, so still unsure if that single CAS is correct)
• add Sync to Sender
  • doesn't require stabilizing a new name
  • perhaps less confusing

[alexcrichton]

🔔 This RFC is now entering its week-long final comment period 🔔

[tikue]

I ran into this issue the other day with a client that spins up a thread that recv()s messages and writes them to a stream in a loop. I had wanted to make the client sync, but I couldn't because of Sender. Cloning wasn't ideal either, because I ended up having to wrap a bunch of Sync fields in Arcs.

I don't feel strongly about the particular implementation; any Sync Sender is better than none.

[alexcrichton]

The libs team discussed this during the triage meeting today and the decision was to close. This unfortunately doesn't interact well with code that already takes a Sender (as this is essentially just another Sender), and we were thinking that we probably don't want to expand channels too much in the standard library as-is (e.g. feeling a little wary about them being stable as-is). We thought that this would make a good external crate, but perhaps not in the standard library itself.

Thanks for the RFC, though, @seanmonstar!