sync: Add `try_recv` back #3639

zaharidichev · 2021-03-22T20:30:21Z

Signed-off-by: Zahari Dichev [email protected]

Motivation

Some time ago, the Receiver::try_recv was removed, due to users experiencing
race conditions when consuming from the chan. This problem caused consumers
to receive a TryRecvError::Empty even though a write to the chan has been complete.

Solution

The solution in this PR is to destinguish between a slot in the concurrent
block list not being ready and the list being empty. When the chan is not
empty and a slot is still not ready to be consumed, we spin in try_recv.

Fix: #3350

Signed-off-by: Zahari Dichev <[email protected]>

carllerche · 2021-03-24T17:33:29Z

Generally speaking, we try to avoid spinning. It’s going to be tricky to avoid that here though. I’m interested in thoughts on that.

zaharidichev · 2021-03-26T12:22:53Z

@carllerche Thought about it a little bit. The way things are setup at the moment, we need to either spin on pop() or spin on push(), where we try to CAS in a block state where all previous slots have been set to ready...

Another option that I have not spent too much time on is to make the receiver be able to "look ahead". So if there is a slot ahead that has been set as Ready, just read its contents and return them, marking it as Read. Then it can be ignored once the index is advanced to point at it. Not sure whether that can actually work. Figuring that out across block boundaries makes this a bit harder. WDYT? Any better suggestions?

Darksonn · 2021-03-27T13:39:32Z

tokio/src/sync/mpsc/chan.rs

+                        return Ok(value);
+                    }
+                    Some(Closed) => return Err(TryRecvError::Closed),
+                    Some(NotReady) => thread::yield_now(),


Should this be using std::hint::spin_loop?

Good question. I borrowed it from https://github.com/tokio-rs/tokio/blob/master/tokio/src/sync/mpsc/block.rs#L348. It has a comment that convinced me this is the right thing to use. But is this comment actually correct? Unclear. Any thoughts ?

Also, what about this bit of docs: https://docs.rs/loom/0.4.0/loom/#yielding

The comment you linked appears to be wrong, as the non-loom version just re-exports std::thread. It also has a comment on MSRV.

Signed-off-by: Zahari Dichev <[email protected]>

gerdoe-jr · 2021-05-31T18:37:39Z

How does it go?

Darksonn · 2021-06-01T07:57:45Z

The PR is relatively complicated, and I haven't found the time to read it thoroughly.

Darksonn · 2021-06-28T10:42:16Z

tokio/src/sync/mpsc/block.rs

            None => {
                // The compare-and-swap succeeded and the newly allocated block
                // is successfully pushed.
+                self.set_has_next();
                return new_block;
            }


Another thread could see the compare and swap before this set_has_next has completed. Is that a problem?

@Darksonn Trying to page all of that back as it has been a while. I had a thought about that when I was writing it and I think it is ok. I think what happens in the situation that you are describing is that for example if you have two blocks such:

B1 B2 | a | b | c | _ | _ | -------> | _ | _ | _ | _ |

If we are trying to read at the second to last slot of B1 and a value is being written at the first slot of B2 and the CAS has succeeded and the thread reading B1 observes that, this means that we have a block but a value will not be written to this block before the current one is marked as having a next block. In this case, we will receive an Empty error, which will technically be correct because the value is still not there. Does that sound correct or I am making things up?

If the consequence of this is that it reports Empty if a send is happening concurrently, then that's fine.

Yes, that is correct.

@Darksonn is that PR still relevant? Is there plan to merge it at some point. If not we can close it I guess.

We do want to add back try_recv, but I'm not sufficiently familiar with the internals of the mpsc channel to properly review it.

Got it, makes sense. Thanks for looking at it nevertheless

Darksonn · 2021-09-07T14:03:21Z

tokio/src/sync/mpsc/chan.rs

+        // Notify the rx tasks
        self.rx_waker.wake();
+        self.not_ready_waker.wake();


There are other places that wake rx_waker. Should the other places not be updated as well?

Darksonn · 2021-09-07T14:14:13Z

tokio/src/sync/tests/loom_mpsc.rs

+        run(&ctx);
+
+        for th in ths {
+            th.join().unwrap()


Suggested change

th.join().unwrap()

th.join().unwrap();

Darksonn · 2021-09-07T14:25:45Z

tokio/src/sync/mpsc/block.rs

+    /// Marks the block as having a next one
+    fn set_has_next(&self) {
+        self.ready_slots.fetch_or(HAS_NEXT, Release);
+    }


You call this in grow, but not in try_push. That seems incorrect to me.

Darksonn · 2021-09-18T07:29:00Z

Thank you for doing this PR. We did not end up accepting it because it was complicated and a simpler solution was available, but the solution that just got merged used several ideas from your PR, and reviewing your PR was helpful in coming up with the solution.

sync: add try_recv back

dc391ea

Signed-off-by: Zahari Dichev <[email protected]>

Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Mar 22, 2021

lint

c543074

Signed-off-by: Zahari Dichev <[email protected]>

Darksonn reviewed Mar 27, 2021

View reviewed changes

zaharidichev added 2 commits April 19, 2021 13:12

use waker

7b3a59f

Signed-off-by: Zahari Dichev <[email protected]>

Merge branch 'master' into zd/add-try-receive-back-2

003d961

zaharidichev requested a review from Darksonn April 19, 2021 13:40

Darksonn reviewed Jun 28, 2021

View reviewed changes

Darksonn reviewed Sep 7, 2021

View reviewed changes

Darksonn mentioned this pull request Sep 15, 2021

sync: implement try_recv for mpsc channels #4113

Merged

Darksonn closed this in #4113 Sep 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync: Add `try_recv` back #3639

sync: Add `try_recv` back #3639

zaharidichev commented Mar 22, 2021

carllerche commented Mar 24, 2021

zaharidichev commented Mar 26, 2021 •

edited

Loading

Darksonn Mar 27, 2021

zaharidichev Mar 27, 2021

zaharidichev Mar 27, 2021

Darksonn Mar 27, 2021

gerdoe-jr commented May 31, 2021

Darksonn commented Jun 1, 2021

Darksonn Jun 28, 2021

zaharidichev Jun 28, 2021 •

edited

Loading

Darksonn Jun 28, 2021

zaharidichev Jun 28, 2021

zaharidichev Jul 11, 2021

Darksonn Jul 11, 2021

zaharidichev Jul 11, 2021

Darksonn Sep 7, 2021

Darksonn Sep 7, 2021

Darksonn Sep 7, 2021

Darksonn commented Sep 18, 2021

sync: Add try_recv back #3639

sync: Add try_recv back #3639

Conversation

zaharidichev commented Mar 22, 2021

Motivation

Solution

carllerche commented Mar 24, 2021

zaharidichev commented Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerdoe-jr commented May 31, 2021

Darksonn commented Jun 1, 2021

Choose a reason for hiding this comment

zaharidichev Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn commented Sep 18, 2021

sync: Add `try_recv` back #3639

sync: Add `try_recv` back #3639

zaharidichev commented Mar 26, 2021 •

edited

Loading

zaharidichev Jun 28, 2021 •

edited

Loading