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

Don't close the whole mpsc channel when one sender is closed #1443

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Jan 29, 2019

I've confirmed locally that this fixes #1440, all senders are given a chance to send before the channel is closed.

There's probably a nicer way to do this with less unwrapping, suggestions welcome otherwise I might take another look whether I can clean this up a bit tomorrow.

@carllerche
Copy link
Member

Could the change include tests?

@ebkalderon
Copy link
Contributor

Looks pretty good. Should this PR also include similar changes to UnboundedSender as well? I haven't tested myself to see if it fails in a similar fashion to #1440.

@Nemo157
Copy link
Member Author

Nemo157 commented Jan 30, 2019

It does, both have a disconnect method added and both Sink implementations are updated to use it. I'll add test cases covering both as well.

I have thought of a cleaner way to do this, move all the internal implementation details into an InnerSender struct (although maybe a different name to avoid having .inner.inner fields) which Sender and UnboundedSender contain in an Option, that way disconnecting a specific sender is just dropping that inner struct, and there should be no unwrapping in the internal functions.

@ebkalderon
Copy link
Contributor

@Nemo157 Just wondering, is this PR still being worked on? It appears to be finished.

@Nemo157
Copy link
Member Author

Nemo157 commented Feb 2, 2019

It's all done from my side (but with the All Hands happening this coming week, there's a good chance there's no one available to review for a bit).

@cramertj cramertj merged commit 052add8 into rust-lang:master Feb 14, 2019
@cramertj
Copy link
Member

Thanks for the fix!

@Nemo157 Nemo157 deleted the mpsc-sender-resiliency branch February 14, 2019 20:45
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.

Possible regression with mpsc channels closing early
4 participants