Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close vs disconnect in futures-channel #1346

Closed
ghost opened this issue Nov 22, 2018 · 3 comments
Closed

Close vs disconnect in futures-channel #1346

ghost opened this issue Nov 22, 2018 · 3 comments
Labels

Comments

@ghost
Copy link

ghost commented Nov 22, 2018

I find the terminology around channel closing/disconnection in futures-channel confusing:

  • The documentation says a channel becomes disconnected when all its receivers or senders get dropped. However, a channel can also be manually closed. Those are two different operations, but the end result is the same: nothing can be sent into the channel anymore.

  • I'd prefer Receiver::close() to be named disconnect().

  • There's a similar method Sender::close_channel(). Why close_channel() and not just close() or disconnect()?

  • Why do we have Receiver::close(), but then also SendError::is_disconnected()? So if we call Receiver::close(), then we may get a SendError that reports the channel as being disconnected?

  • Similarly, if we drop all receivers and disconnect the channel, then Sender::is_closed() will return true.

I think we should pick either closed or disconnected and just stick with it. Between those two, disconnected is used in std::sync::mpsc, but closed is more common elsewhere (channels in Go get closed, Unix pipes get closed, etc.).

Finally, two more related questions:

  • Why is there Sender::is_closed() but not Receiver::is_closed()?

  • Why is SendError an opaque struct with is_* methods rather than an enum like std::sync::mpsc::TrySendError? Do we expect to add more error cases in the future?

Personally, I'm leaning towards universal terminology/API within the Rust ecosystem - it'd be great if we could make channel libraries (std::sync::mpsc, futures-channel, tokio-channel, crossbeam-channel) as similar and consistent as possible.

cc @cramertj

@ebkalderon
Copy link
Contributor

Not sure how accurate this information is now that #1443 has been merged. The PR introduces a Sender::disconnect() method alongside the existing Sender::close_channel() method, among other things, and the doc comments for both methods now explain what the difference is between the two operations.

@najamelan
Copy link
Contributor

I would be in favor of using close and closed everywhere and nothing else. It is the briefest, and it conveys meaning clearly. The PR above does go the opposite direction. It introduces a feature that makes it possible to close all senders from a single sender.

If we don't want to distinguish between disconnect and close, I don't think this feature can work. However, personally I find it questionable. I wonder if anyone really uses this. I would suggest that one drops all senders in order to close a channel. Otherwise only a specific sender is closed. I even wonder about the usefulness of the close method as opposed to just dropping (on the sender side that is). It enlarges API surface for something that can always be solved with an Option.

@cramertj cramertj added the docs label Oct 31, 2019
@cramertj
Copy link
Member

I'd love to see docs improved to clarify the situation if any of y'all want to send a PR! Otherwise, there hasn't been much discussion on this issue so I'm going to close for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants