Tracking issue for channel selection

[alexcrichton]

This is a tracking issue for the unstable mpsc_select feature in the standard library. Known points:

• The API is unsafe, and should probably never be stabilized.
• Servo apparently makes very heavy use of this API, so it can't be removed without replacement currently.
• The correctness of the implementation is somewhat dubious, it's incredibly complicated.
• The macro-based approach for selection is dubious.
• This is not a scalable implementation of selection (e.g. "poll everything each time wait() is called")
• This drags down some other channel-related code with a few mutexes in otherwise lock-free code.

[jdm]

cc me

[steveklabnik]

Is it worth investigating @BurntSushi 's https://github.com/BurntSushi/chan ?

[steveklabnik]

I also think @carllerche was supporting the idea of removing channels from the stdlib

[alexcrichton]

Note that I'm not advocating removing channels from the standard library (plus they're stable). Removing selection is also probably not an option as it's so closely tied to the implementation details of channels.

One of the major points of complexity of the current implementation is that it's all basically 99% lock free (giving us a nice performance boost in theory). I believe @BurntSushi's implementation (please correct me if I'm wrong!) is largely based on mutexes and condition variables.

[BurntSushi]

@alexcrichton Correct. I doubt very much that chan will ever be lock free because I'm not convinced the semantics I want can be done with them. It's also much easier to implement. :-)

[nagisa]

I personally prefer the epoll-like design as implemented in comm.

[carllerche]

Streams in Eventual are lock free :) There is also stream selection, which is lock free (and definitely is a pain to implement).

Re: removing channels from stdlib, as @alexcrichton points out, they are stable. I would not worry about adding select to them. They are good as a starting point. If somebody requires more complex concurrency semantics, they can use a lib. I'm not sure why @alexcrichton thinks that removing select from std is hard though...

[tripped]

Just hopping on to mention #12902 as one of the current weaknesses of the macro approach to select!.

[softprops]

Fwiw, channel selection with golang channels is part of what makes golang concurrency story so alluring to newcomers to go. For a newcomer to a safe concurrent language like rust, needing to mull over which 3rd party concurrency crate is popular this week to pick for actually writing concurrent code feels onerous. Having primatives like channels and channel selection in the std lib make rust more attractive for concurrent applications than alternatives like go and make crates a little more interoperable than resolving conflicting implementation issues betwwen 3rd party options.

[BurntSushi]

Fwiw, channel selection with golang channels is part of what makes golang concurrency story so alluring to newcomers to go.

@softprops This doesn't address your primary concerns, but FYI, chan_select! in the chan crate was built to specifically match the semantics of Go's select.

[softprops]

@BurntSushi nice!

[jonhoo]

Correct me if I'm wrong, but doesn't the macro approach also have the drawback that selecting over a dynamic set of channels isn't easily expressible?

[szagi3891]

Best to get rid of the macro select.
I made a working prototype that shows that it is possible:
https://github.com/szagi3891/channels_async-rs/blob/master/examples/select.rs#L76
Without any section unsafe.

[canndrew]

Here's a wacky idea: add support for generically sized tuples and anonymous enums. Then it will be possible to select across an arbitrary number of channels of different types without macros:

let ch0: Receiver<i32> = ...;
let ch1: Receiver<String> = ...;
match select((ch0, ch1)) {
    (x|!) => println!("Got an i32: {}", x),
    (!|y) => println!("Got a String: {}", y),
}

[BurntSushi]

Is there anyone still using this API? I don't see any obvious path to stabilization. It would be nice to just remove it.

[alexcrichton]

@jdm is this still being used in Servo?

[SimonSapin]

grep finds 4 instances of select! in servo master.

[mattgreen]

I'd like to remove this from watchexec, but I'm unsure what to move to. One of my crates already streams data to me via a channel, so I hopped onto that bandwagon.

I'm usually watching for either control-c or file system inputs at all times, since I have cleanup to do when the user hits control-c. select! might be unstable but I can at least accomplish this.

[softprops]

I've had only good experiences with https://github.com/BurntSushi/chan

[szagi3891]

@mattgreen :
I think you should be able to replace this selecta! macoro of one channel.

This channel type will be :

enum Message {
    CtrlC,
    NormalMessage(....),
}

The main thread would send in theright time message Message::CtrlC.
The remainder of the program would send messages Message::NormalMessage

It makes sense ?

[antrik]

Servo's ipc-channel library ( https://github.com/servo/ipc-channel ) also uses mpsc_select (for its inprocess back-end) -- which is a bit of a pain, since platforms relying on this back-end can't use ipc-channel with stable Rust. (See servo/ipc-channel#118 )

It should be noted though that this back-end is not a real implementation, but rather just a partial workaround for platforms that don't have a "real" inter-process implementation yet. While it was originally created for Windows, most likely Windows will be getting a proper implementation soonish. ( servo/ipc-channel#108 )

Right now the inprocess back-end is also being used for Android -- though I'm not sure why really...

So while this back-end might continue to be useful during bring-up of new platforms (depending on whether Servo keeps the option of running in a single process), it's not exactly a first-class citizen I'd say.

[antrik]

I'd like to point out that https://github.com/BurntSushi/chan -- which is being touted as a replacement for mpsc here -- doesn't really seem like a valid alternative to me: unlike mpsc, it has clonable and sharable receivers.

This might seem convenient for some use cases; but I'm not convinced it's a real win over just wrapping the receiver in a Mutex<> or Arc<Mutex<>> yourself when you know it's save and necessary for your application. (As mutexes are safe and easy to use in Rust -- unlike in Go -- I don't see any motivation for channels to double as a locking substitute...)

More importantly though, for the vast majority of use cases, sharing the receiver is actually not desirable: rather, it often indicates bugs -- and thus not rejecting this by default is indeed more error prone!

(Not even considering implementation overhead for something that's not needed at all in most cases...)

[BurntSushi]

@antrik Please see the README for chan. Notably:

The purpose of this crate is to provide a safe concurrent abstraction for communicating sequential processes. Performance takes a second seat to semantics and ergonomics. If you're looking for high performance synchronization, chan is probably not the right fit.

There are a number of reasons why it doesn't necessarily replace std::sync::mpsc. Nevertheless, if you want to write in a CSP style in Rust, then I do think it's your only option on stable Rust because it gives you chan_select!. I suspect that's why people are bringing it up (which seems reasonable to me).

[antrik]

@BurntSushi as long as there is a general understanding that chan is a special-purpose library rather than a general replacement, that's perfectly fine -- I'm just concerned that there seems to be an air here of "we don't need to fix mpsc since there is chan", which I find rather disconcerting...

[SimonSapin]

With servo/servo#21325 merged (thanks @stjepang and @gterzian!), Servo does not use this feature anymore.

Do we know of other projects that do? Would it be useful to do a crater run (with something like renaming the feature flag) to find out?

@rust-lang/libs How do you feel we should proceed with this feature?

[alexcrichton]

FWIW #54228 seems like a good way to proceed. I think if that's small enough we can probably just outright delete the feature, and if it's large we can do a round of messaging with a schedule to delete.

[SimonSapin]

I’m in favor of deleting eventually, but since that part of the code isn’t being actively worked on there isn’t much to gain by deleting sooner rather than later. We can keep it with a deprecation warning for a few release cycles, just in case there are users not covered by Crater to leave them some more time to migrate. (This migration took a while to land in Servo.)

I my opinion #54228 can help figure out if we want to keep delaying even the deprecation warning like we did so far.

[SimonSapin]

Crater has found three regressions. One spurious, one that I submitted a PR to fix, and one in a benchmark measuring specifically the performance of the select! macro: #54228 (comment)

So let’s formally propose:

• Emit a deprecation warning for usage of this feature
• Wait for a few release cycles for remaining users to have an opportunity to see the warning and potentially comment here
• Then, unless new information or arguments have come up, remove the feature.
• According to this issue’s original message, this would allow removing some mutexes in the rest of the channels code to make it lock-free. Would someone be interested in working on that?

@rfcbot fcp close

[rfcbot]

Team member @SimonSapin has proposed to close this. The next step is review by the rest of the tagged teams:

• @Kimundi
• @SimonSapin
• @alexcrichton
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

[rfcbot]

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

[rfcbot]

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

[SimonSapin]

Next is landing a deprecation warning.

[SimonSapin]

Triage: this unstable feature was deprecated in 1.32, so it is fine to remove any time.