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

tokio-channel: fix race with dropping Receiver #601

Closed
wants to merge 4 commits into from

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Aug 30, 2018

This branch adds a stress test to tokio-channel for a race condition
that can occur when the Receiver end of a channel is dropped as the
Sender end calls try_send, leaving the sent data in the queue. This
is based on the repro written by @simmons for
rust-lang/futures-rs#909; since the tokio-channel code is
based on the futures-rs channel implementation, the issue exists here
as well.

Note that since this branch does not resolve the issue, the stress
test currently fails.

Signed-off-by: Eliza Weisman eliza@buoyant.io

@carllerche
Copy link
Member

Thank you.

Previously, when dropping `Receiver`, there was a race condition that
permitted messages to stay in the channel indefinitely.

This patch fixes the race. Now, when `Receiver` is dropped, it is
guaranteed that no messages will be stuck in the channel (assuming there
are no further bugs, of course).
@carllerche carllerche changed the title tokio-channel: Add a stress test for racy try_send as receiver is dropped tokio-channel: fix race with dropping Receiver Aug 31, 2018
@carllerche
Copy link
Member

I have applied a fix

cc @seanmonstar

* 100ms is not enough for CI
* Spinning is not good.
Copy link
Member Author

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks good to me, thanks @carllerche!

tokio-channel/src/mpsc/mod.rs Show resolved Hide resolved
self.dec_num_messages();

// Return the message
return Ok(Async::Ready(msg));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this code to next_message makes sense to me 👍

@carllerche
Copy link
Member

Fixed upstream rust-lang/futures-rs#1241

@carllerche carllerche closed this Sep 11, 2018
@carllerche carllerche deleted the eliza/mpsc-stress branch September 11, 2018 17:12
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.

2 participants