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

stream: coordinating Tokio 1.0 with the availability of Stream in std #2870

Closed
carllerche opened this issue Sep 23, 2020 · 9 comments · Fixed by #3277
Closed

stream: coordinating Tokio 1.0 with the availability of Stream in std #2870

carllerche opened this issue Sep 23, 2020 · 9 comments · Fixed by #3277
Assignees
Labels
A-tokio Area: The main tokio crate C-proposal Category: a proposal and request for comments M-stream Module: tokio/stream
Milestone

Comments

@carllerche
Copy link
Member

carllerche commented Sep 23, 2020

Currently, Tokio uses the Stream trait provided by futures-core. There is an ongoing effort to define a Stream trait within std. Tokio is currently aiming to release 1.0 by the end of the year. Given the Rust release cadence, Stream may not be available in std on the stable channel by the end of the year.

If Stream is not available on the Rust stable channel by the end of the year, we have two options.

  1. Delay Tokio 1.0 until Stream becomes available in std.
  2. Remove Stream usage from Tokio until 6 months after it becomes available in std (due to the proposed MSRV policy).

Stream usage

Currently, Stream is implemented in Tokio by:

  • mpsc::Receiver
  • broadcast::Receiver
  • signal::Signal

Tokio also provides a tokio::stream module containing a number of useful stream utilities. A list can be found here.

Removing the Stream dependency

If Stream is removed from the tokio crate, it can be placed temporarily in the tokio-util crate. This crate is not intended to reach 1.0 by the end of year. Stream usage within Tokio is already isolated via the stream feature flag. While useful, it is not a critical dependency.

Removing Stream from tokio would impact the discoverability of the utilities. This could be mitigated by including an empty tokio::stream module with documentation explaining where to find the utilities.

Adding Stream post 1.0

Adding Stream after an initial 1.0 release would take at least 6 months given the currently proposed MSRV policy (#2718). According to the policy, 1.x releases are permitted to increase the MSRV, but must support all Rust versions released in the past 6 months.

Next steps

I will leave this open-ended for now as we collect comments. We can then decide on the direction we want to take.

@carllerche carllerche added C-proposal Category: a proposal and request for comments A-tokio Area: The main tokio crate labels Sep 23, 2020
@carllerche carllerche added this to the v1.0 milestone Sep 23, 2020
@carllerche carllerche mentioned this issue Sep 23, 2020
10 tasks
@sfackler
Copy link
Contributor

I don't know what the current thought is, but there was an idea that the futures crate could start transparently reexporting std's Stream when being built against a suitable version to avoid forcing a big migration.

@taiki-e
Copy link
Member

taiki-e commented Sep 23, 2020

See also rust-lang/futures-rs#2207

@carllerche
Copy link
Member Author

@taiki-e thanks for that. My understanding is that it would still technically be a breaking change. My gut is that I would rather start a 1.0 stability guarantee on strong footing.

@davidbarsky
Copy link
Member

For exhaustiveness, I think there that there's a third option: a six month MSRV policy can take effect well after a 1.0 is released. I'm not convinced that a Tokio 1.0 release needs to be coupled to a six month MSRV policy at release. This is something that can be added retroactively.

@taiki-e
Copy link
Member

taiki-e commented Sep 24, 2020

@carllerche Yeah, that's technically breaking change. AFAIK, theoretically, if the signature of the std Stream trait doesn't change after futures_core v1 released, and the build script works, and the user doesn't accidentally rely on the unstable Stream trait (that requires#![feature]), no breakage will occur. But, in any case, when defining Stream in futures_core, the risk cannot be zero.

FYI: I've created a demo of how that's implemented: https://github.com/taiki-e/futures-compat-experiment

@taiki-e taiki-e added the M-stream Module: tokio/stream label Oct 7, 2020
@taiki-e
Copy link
Member

taiki-e commented Oct 7, 2020

Perhaps another idea for this would be to use a (disabled by default) feature flag that includes the version number: like stream03, stream1.
(Once std Stream is stable, make the stream1 feature to no-op and always implement the stream from futures_core. If the implementation of futures_core's Stream is different from std's Stream, we can implement std's Stream directly as they don't conflict.)

@Keruspe
Copy link
Contributor

Keruspe commented Nov 14, 2020

Related: rust-lang/rust#79023

@carllerche
Copy link
Member Author

As of this comment, Stream trait RFC is not yet merged. I expect it is mostly ready to go, but I am not exactly clear on the expected timeline.

If Stream lands in stable sometime in January 2021, I think we could consider delaying the 1.0 release, but probably not more than that. However, I would expect Stream to not be available in std until later.

Assuming that Stream lands in stable February 2021 or later, I propose:

  • Extract the contents of tokio::stream into the tokio-stream crate.
  • Leave an empty tokio::stream module documenting that Stream utilities exist in tokio-stream and will be merged back into tokio once the Stream trait lands in std.
  • Remove Stream impls for misc types and document how to use async-stream to get your own stream implementation. These types already have inherent fns to consume and do not depend on their Stream implementations.
    • tokio::fs::ReadDir
    • tokio::io::Lines
    • tokio::net::{TcpListener, UnixListener}
    • tokio::signal::{CtrlC, CtrlBreak}
    • tokio::sync::broadcast
    • tokio::time::Interval

Once Stream lands in std, we add tokio::stream back. However, we must do this in a way that preserves the Tokio's MSRV guarantee of ~6 months. To do this, we can add a build script that detects the Rust version. If using a version of Rust that includes Stream, then re-export that Stream and use it. Otherwise, define a Stream trait that matches the version in std and use that one. This is a similar strategy as the one proposed for futures-rs but does not introduce Stream until after Stream is in the std stable channel. This prevents potential breaking changes with mismatched Stream versions

@carllerche
Copy link
Member Author

The stream::Iter utility uses the coop helper which is not public. Instead, we could hard limit the number of items returned to 32 then yield. It is not ideal but is temporary.

cc @LucioFranco

LucioFranco added a commit that referenced this issue Dec 14, 2020
LucioFranco added a commit that referenced this issue Dec 14, 2020
This change removes all references to `Stream` from
within the `tokio` crate and moves them into a new
`tokio-stream` crate. Most types have had their
`impl Stream` removed as well in-favor of their
inherent methods.

Closes #2870
carllerche pushed a commit that referenced this issue Dec 16, 2020
This change removes all references to `Stream` from
within the `tokio` crate and moves them into a new
`tokio-stream` crate. Most types have had their
`impl Stream` removed as well in-favor of their
inherent methods.

Closes #2870
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]>
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-proposal Category: a proposal and request for comments M-stream Module: tokio/stream
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants