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

Add unit test showing some unexpected behaviour of mpsc::channel #1997

Conversation

bikeshedder
Copy link
Contributor

@bikeshedder bikeshedder commented Dec 20, 2019

In 'deadpool' I noticed some weird behaviour when calling Receiver::try_recv on a channel which is protected by a semaphore in such a way that the size can't exceed its limit: https://github.com/bikeshedder/deadpool/blob/6daa2e7b5bd06d0b516b83f9da1902848e7812e7/src/lib.rs#L266

I noticed more objects being returned to the pool than the max_size which just didn't make any sense. After some digging I found that try_recv sometimes does not return Ok even if something has already been put into the channel before via try_send.

This behaviour can be reproduced with the following steps:

  • Create a semaphore with size 1
  • Create a channel and fill it with one message
  • Spawn two tasks
  • One of the tasks manages to acquire the permit, receive the message from the channel and send it back. Afterwards the permit is dropped.
  • Now the other task tries to do the same and fails during the try_recv call.

I have no idea how to fix that but please find that test case attached.

As using channel just with try_send and try_lock is a bit wasteful in my case anyways I just found crossbeam_queue which supports concurrent push/pop without any locking.

I'm afraid I might have actually introduced that bug myself when adding Receiver::try_recv.

for _ in 0..2 {
let ctx = ctx.clone();
tasks.push(tokio::spawn(async move {
let permit = ctx.sem.acquire();
Copy link
Member

Choose a reason for hiding this comment

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

You are not awaiting the permit. Adding an .await makes the test pass.

@carllerche
Copy link
Member

Looks like the bug was in the test. I'm going to close this as the test passes once it is fixed 👍

I think if you didn't explicitly drop the permit, you would have gotten a must_use warning?

@carllerche carllerche closed this Dec 22, 2019
@bikeshedder
Copy link
Contributor Author

I have just rebased the branch to the current master, fixed the error in the test and changed the scheduler to threaded_scheduler and increased the count of permits, workers and cycles. As that error is highly timing dependant it's not guaranteed that the error will show up on the first run. When increasing the CYCLES to 100_000 the error will almost always show up.

I totally messed that test up while trying to come up with a test that contains as little noise as possible. That's the smallest test I was able to come up with. I bet there is an even simpler way to reproduce that which I'm currently unaware of.

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