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 back mpsc try_recv #3350

Closed
Darksonn opened this issue Dec 26, 2020 · 10 comments · Fixed by #4113
Closed

Add back mpsc try_recv #3350

Darksonn opened this issue Dec 26, 2020 · 10 comments · Fixed by #4113
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-sync Module: tokio/sync

Comments

@Darksonn
Copy link
Contributor

Darksonn commented Dec 26, 2020

The mpsc try_recv method was removed in #3263 due to problems with the implementation. This issue tracks adding it back.

Note that it is currently possible to do unconstrained(rx.recv()).now_or_never() using tokio::task::unconstrained and FutureExt::now_or_never, but be aware that it has the same issue as the old try_recv that previously sent messages may not be available immediately. To be clear, they are not lost, the messages are just not received until later.

@jeremyandrews
Copy link

jeremyandrews commented Feb 5, 2021

In trying to upgrade Goose to Tokio 1.0+ I've run into a regression due to the removal of mpsc::try_recv. Reviewing this and linked issues, it sounds like I'm running into the bug that caused try_recv to be removed in the first place, however I don't experience any problems with Tokio's 0.2 implementation of try_recv.

For example, I was using try_recv to synchronize metrics from user threads to the parent threads, but replacing this with now_or_never() tests no longer pass (and through some manual debugging it seems this is because the metrics aren't showing up in the channel in a timely manner anymore, so Goose exits before synchronizing all metrics -- I was able to get tests to pass by adding some tiny sleeps and retries).

Is there any reason why upgrading to Tokio 1.0+ and switching to futures::FutureExt::now_or_never() would exacerbate this problem? Are there any recommendations on how I might get this as reliable as it was with try_recv in Tokio 0.2 so I can complete my upgrade and get tests reliably passing again?

@Darksonn
Copy link
Contributor Author

Darksonn commented Feb 5, 2021

@jeremyandrews In your case where you really need try_recv for synchronization, I recommend that you try replacing your channel with either flume or async-channel.

@jeremyandrews
Copy link

@Darksonn Thanks, will do.

@bikeshedder
Copy link
Contributor

When writing deadpool that try_recv issue (See PR #2020) did bite me hard. That issue was triggered only under heavy load and benchmarking conditions and looked like undefined behavior at first glance. It took me several pots of tea and a long debugging sessions to finally isolate try_recv as being the source of it. That's why I switched to using crossbeam + tokio::sync::Semaphore. It's good that try_recv is gone from the public API until this is fixed.

@jeremyandrews I also wrote deadqueue which is implemented the same way but also has some more advanced features like resizable queues. It features a slightly different API than usual, but might be worth a try since it comes with some features that the usual channel implementations with RX/TX-pairs don't have.

@jeremyandrews
Copy link

@bikeshedder Thanks for the feedback. I was able to get things working by switching to flume (my needs are relatively simple), but I'll take a look at deadqueue as an alternative option as well.

bors bot pushed a commit to sigp/lighthouse that referenced this issue Feb 10, 2021
## Issue Addressed

resolves #2129
resolves #2099 
addresses some of #1712
unblocks #2076
unblocks #2153 

## Proposed Changes

- Updates all the dependencies mentioned in #2129, except for web3. They haven't merged their tokio 1.0 update because they are waiting on some dependencies of their own. Since we only use web3 in tests, I think updating it in a separate issue is fine. If they are able to merge soon though, I can update in this PR. 

- Updates `tokio_util` to 0.6.2 and `bytes` to 1.0.1.

- We haven't made a discv5 release since merging tokio 1.0 updates so I'm using a commit rather than release atm. **Edit:** I think we should merge an update of `tokio_util` to 0.6.2 into discv5 before this release because it has panic fixes in `DelayQueue`  --> PR in discv5:  sigp/discv5#58

## Additional Info

tokio 1.0 changes that required some changes in lighthouse:

- `interval.next().await.is_some()` -> `interval.tick().await`
- `sleep` future is now `!Unpin` -> tokio-rs/tokio#3028
- `try_recv` has been temporarily removed from `mpsc` -> tokio-rs/tokio#3350
- stream features have moved to `tokio-stream` and `broadcast::Receiver::into_stream()` has been temporarily removed -> `tokio-rs/tokio#2870
- I've copied over the `BroadcastStream` wrapper from this PR, but can update to use `tokio-stream` once it's merged tokio-rs/tokio#3384

Co-authored-by: realbigsean <[email protected]>
CaptainAchab pushed a commit to TankerHQ/sdk-rust that referenced this issue Mar 12, 2021
Gelbpunkt added a commit to Gelbpunkt/twilight that referenced this issue Apr 19, 2021
@threema-danilo
Copy link

When dealing with a one-shot channel that must be polled from time to time, what are the tradeoffs between the unconstrained+now_or_never workaround versus a select! { biased; ... } call including the oneshot receiver and an always-ready future?

@threema-danilo
Copy link

After asking in the #tokio-users channel on Discord I can now answer this question:

  • The biased select! call works the same as now_or_never
  • The unconstrained requirement applies to the select! workaround as well: If the future is not unconstrained, then it may return "not ready" even though it's ready (due to Tokio's automatic cooperative task yielding).

@Darksonn
Copy link
Contributor Author

For anyone listening in here, the next Tokio release will happen next week and will include a try_recv method on both bounded and unbounded mpsc channels.

@lacasaprivata2
Copy link

hi - why does try_recv require mutable reference?

@Darksonn
Copy link
Contributor Author

Because the mpsc channel is a multi-producer single-consumer channel, and using an immutable reference would allow several receivers to receive in parallel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-sync Module: tokio/sync
Projects
None yet
5 participants