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

Make reason for try_send errors clearer #864

Merged
merged 2 commits into from
Jan 23, 2019

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Jan 23, 2019

When you call try_send on a channel and the call fails, it often matters to the caller why the call failed. For unbounded senders, we know that it must be because the receiver went away, whereas for bounded channels it may either be because the receiver went away or because there was no available capacity. This PR explicitly calls out in the docs that errors on UnboundedSender::try_send can only occur if the channel receiver has gone away, and adds was_closed and was_full methods to the error returned by BoundedSender::try_send so the caller can figure out which of the two error cases they're in.

@carllerche carllerche merged commit 0ec8986 into tokio-rs:master Jan 23, 2019
@jonhoo jonhoo deleted the try-send-error-reason branch January 23, 2019 23:06
@jonhoo
Copy link
Contributor Author

jonhoo commented Jan 28, 2019

I just noticed that is_closed is called is_disconnected in futures::sync::mpsc. Not sure if it's worth renaming + deprecating to match the code people are coming from?

@LucioFranco
Copy link
Member

@jonhoo I saw that too, I am in favor of keeping the api roughly the consistent, and it seems this api is still the case in futures 0.3 as well.

@jonhoo
Copy link
Contributor Author

jonhoo commented Jan 28, 2019

Along the same lines, it may be that we want oneshot::Sender::poll_close to poll_cancel (and similarly rename is_close to is_cancelled). That might be a more controversial change though, as poll_close aligns better with things like Sink.. :/

@carllerche ^

@ghost
Copy link

ghost commented Jan 28, 2019

Related issues on "close" vs "disconnect":

I wish the whole community just chooses one of those two words and sticks to it. :)

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

Successfully merging this pull request may close these issues.

3 participants