Limit `futures` dependency to `Stream` via feature flag #1774

carllerche · 2019-11-15T06:30:26Z

In an effort to reach API stability, the tokio crate is shedding its
public dependencies on crates that are either a) do not provide a
stable (1.0+) release with longevity guarantees or b) match the tokio
release cadence. Of course, implementing std traits fits the
requirements.

The on exception, for now, is the Stream trait found in futures_core.
It is expected that this trait will not change much and be moved into `std.
Since Tokio is not yet going reaching 1.0, I feel that it is acceptable to maintain
a dependency on this trait given how foundational it is.

Since the Stream implementation is optional, types that are logically
streams provide async fn next_* functions to obtain the next value.
Avoiding the next() name prevents fn conflicts with StreamExt::next().

Additionally, some misc cleanup is also done:

tokio::io::io -> tokio::io::util.
delay -> delay_until.
Timeout::new -> timeout(...).
signal::ctrl_c() returns a future instead of a stream.
{tcp,unix}::Incoming is removed (due to lack of Stream trait).
time::Throttle is removed (due to lack of Stream trait).
Fix: mpsc::UnboundedSender::send(&self) (no more conflict with Sink fns).

In an effort to reach API stability, the `tokio` crate is shedding its _public_ dependencies on crates that are either a) do not provide a stable (1.0+) release with longevity guarantees or b) match the `tokio` release cadence. Of course, implmementing `std` traits fits the requirements. Instead of publically dependending on `futures` from `tokio`, the `tokio-util` crate provides the necessary bridge to go from Tokio types to `futures` traits (primarily `Stream`). Instead of implementing `Stream` for iteration, types that are logically streams provide `async fn next_*` functions to obtain the next value. Avoiding the `next()` function name will allow for forwards compatibility once the `Stream` trait is provided by `std`. Additionally, some misc cleanup is also done: - `tokio::io::io` -> `tokio::io::util`. - `delay` -> `delay_until`. - `Timeout::new` -> `timeout(...)`. - `signal::ctrl_c()` returns a future instead of a stream. - `{tcp,unix}::Incoming` is removed (due to lack of `Stream` trait). - `time::Throttle` is removed (due to lack of `Stream` trait).

ipetkov · 2019-11-15T07:04:58Z

The signal and process changes LGTM, modulo adding docs on the Signal impl methods!

taiki-e · 2019-11-15T07:18:08Z

Any reason for choosing to define our own trait instead of depends on futures via feature flags?

tokio/src/stream/mod.rs

vorot93 · 2019-11-15T11:56:10Z

Even though I do understand the reasoning behind this move, it's nevertheless not easy to accept as it kind of feels like forced reinventing the wheel. After all, is Stream trait really going to undergo a breaking change in the future?

tokio-util/src/stream/mod.rs

tokio-util/src/stream/into_std.rs

carllerche · 2019-11-15T16:58:36Z

After some discussion in discord: Tokio's next release isn't 1.0, so we can pull in futures-core via a feature flag and hopefully Stream will be stabilized before 1.0.

carllerche · 2019-11-15T19:52:00Z

Ok, based on feedback, I brought back a futures_core::Stream dependency behind a stream feature flag.

carllerche · 2019-11-15T19:53:09Z

@ipetkov which fns?

seanmonstar · 2019-11-15T19:55:06Z

There's one (big) concern with an unstable feature flag: other dependencies can enable it, and a user doesn't realize they are using "unstable" APIs. This problem was noticed in Rayon, and they switched from an unstable feature to requiring RUSTFLAGS="--cfg rayon_unstable" instead.

I've also noticed that warp master is depending on hyper's unstable-stream feature, which means anyone who uses warp and hyper will notice the "unstable" APIs are just available, like Body::wrap_stream.

carllerche · 2019-11-15T21:07:03Z

I agree that unstable feature flags aren’t a viable option.

Since we aren’t hitting 1.0 and there is a path for Stream to land in std by 1.0 I say we stay optimistic

carllerche · 2019-11-15T21:41:49Z

tokio-test/src/io.rs

@@ -122,7 +122,7 @@ impl Handle {
    /// The next operation in the mock's script will be to expect a `read` call
    /// and return `buf`.
    pub fn read(&mut self, buf: &[u8]) -> &mut Self {
-        self.tx.try_send(Action::Read(buf.into())).unwrap();
+        self.tx.send(Action::Read(buf.into())).unwrap();


The unbounded mpsc send API changed to be more ergonomic.

carllerche · 2019-11-15T21:42:10Z

tokio-tls/tests/smoke.rs

@@ -524,7 +524,7 @@ async fn client_to_server() {
    drop(env_logger::try_init());

    // Create a server listening on a port, then figure out what that port is
-    let srv = t!(TcpListener::bind("127.0.0.1:0").await);
+    let mut srv = t!(TcpListener::bind("127.0.0.1:0").await);


incoming now takes &mut self.

carllerche · 2019-11-15T21:47:58Z

tokio-test/src/io.rs

@@ -298,7 +298,7 @@ impl AsyncRead for Mock {
                Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => {
                    if let Some(rem) = self.inner.remaining_wait() {
                        let until = Instant::now() + rem;
-                        self.inner.sleep = Some(time::delay(until));
+                        self.inner.sleep = Some(time::delay_until(until));


delay was renamed delay_until to further disambiguate from delay_for.

carllerche · 2019-11-15T21:48:35Z

tokio/Cargo.toml

@@ -40,7 +41,7 @@ blocking = ["rt-core"]
 dns = ["blocking"]
 fs = ["blocking"]
 io-driver = ["mio", "lazy_static", "sync"] # TODO: get rid of sync
-io-util = ["pin-project", "memchr"]
+io-util = ["pin-project", "pin-project-lite", "memchr"]


Trying out pin-project-lite, we probably should switch.

carllerche · 2019-11-15T21:49:05Z

tokio/Cargo.toml

 iovec = "0.1"

 # Everything else is optional...
 fnv = { version = "1.0.6", optional = true }
+futures-core = { version = "0.3.0", optional = true }


Only the Stream trait is used.

carllerche · 2019-11-15T21:56:15Z

tokio/src/future/maybe_done.rs

+
+/// A future that may have completed.
+#[derive(Debug)]
+pub(crate) enum MaybeDone<Fut: Future> {


Instead of dealing w/ refactoring internals to remove dependency on the few futures-util helpers, I copied them here as a temporary measure.

carllerche · 2019-11-15T21:56:56Z

tokio/src/future/mod.rs

@@ -0,0 +1,15 @@
+#![allow(unused_imports, dead_code)]


I don't really feel like figuring out which feature flags need which helpers. I hope that some of these can be removed.

carllerche · 2019-11-15T21:57:35Z

tokio/src/io/mod.rs

 #[cfg(feature = "io-util")]
-pub use self::io::{


Move files from tokio::io::io -> tokio::io::util.

taiki-e · 2019-11-16T05:14:40Z

tokio/src/future/pending.rs

@@ -0,0 +1,44 @@
+use std::future::Future;


looks like this file is unused.

carllerche requested review from ipetkov, taiki-e, hawkw and seanmonstar November 15, 2019 06:30

carllerche added 2 commits November 14, 2019 22:35

fmt

8cef1fa

fix test

19006c3

try to fix windows

fb7f606

taiki-e reviewed Nov 15, 2019

View reviewed changes

tokio/src/stream/mod.rs Outdated Show resolved Hide resolved

jebrosen reviewed Nov 15, 2019

View reviewed changes

tokio-util/src/stream/mod.rs Outdated Show resolved Hide resolved

tokio-util/src/stream/into_std.rs Outdated Show resolved Hide resolved

bring back futures_core::Stream dep as feature flag

4ae6d4b

carllerche added 2 commits November 15, 2019 13:38

fix tests

b419acc

fmt

9671b4c

carllerche commented Nov 15, 2019

View reviewed changes

seanmonstar approved these changes Nov 15, 2019

View reviewed changes

carllerche changed the title ~~remove futures dependency from tokio~~ Limit futures dependency to Stream via feature flag Nov 15, 2019

carllerche commented Nov 15, 2019

View reviewed changes

hawkw approved these changes Nov 15, 2019

View reviewed changes

carllerche commented Nov 15, 2019

View reviewed changes

tokio/src/io/mod.rs

#[cfg(feature = "io-util")]

pub use self::io::{

Copy link

Member Author

carllerche Nov 15, 2019

Choose a reason for hiding this comment

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

Move files from tokio::io::io -> tokio::io::util.

carllerche added 7 commits November 15, 2019 14:01

try to fix ci again

b6791cd

try to fix ci again

97371c6

try to fix windows again

39ea191

try again

0d80e87

and again

53278d5

fix windows (hopefully for real, i ran it locally this time)

ab2bbcd

dox

ac047a2

taiki-e reviewed Nov 16, 2019

View reviewed changes

tokio/src/future/pending.rs

@@ -0,0 +1,44 @@

use std::future::Future;

Copy link

Member

taiki-e Nov 16, 2019

Choose a reason for hiding this comment

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

looks like this file is unused.

fmt

5b962cc

carllerche merged commit 8a7e577 into master Nov 16, 2019

taiki-e deleted the rm-futures-dep branch November 16, 2019 07:06

taiki-e mentioned this pull request Nov 16, 2019

Migrate from pin-project to pin-project-lite #1778

Merged

Riateche mentioned this pull request Dec 5, 2019

Timeout for stream #1901

Closed

udoprog mentioned this pull request Jan 4, 2020

Implement FusedStream for a few types #2048

Closed

taiki-e mentioned this pull request Apr 20, 2021

Remove uses of unstable feature(cfg_target_has_atomic) rust-lang/futures-rs#2400

Merged

petrzjunior mentioned this pull request Oct 4, 2023

Add Future::timeout() method #6047

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit `futures` dependency to `Stream` via feature flag #1774

Limit `futures` dependency to `Stream` via feature flag #1774

carllerche commented Nov 15, 2019 •

edited

Loading

ipetkov commented Nov 15, 2019

taiki-e commented Nov 15, 2019

vorot93 commented Nov 15, 2019 •

edited

Loading

carllerche commented Nov 15, 2019

carllerche commented Nov 15, 2019

carllerche commented Nov 15, 2019

seanmonstar commented Nov 15, 2019

carllerche commented Nov 15, 2019

carllerche Nov 15, 2019

carllerche Nov 15, 2019

carllerche Nov 15, 2019

carllerche Nov 15, 2019

carllerche Nov 15, 2019

carllerche Nov 15, 2019

carllerche Nov 15, 2019

carllerche Nov 15, 2019

taiki-e Nov 16, 2019

Limit futures dependency to Stream via feature flag #1774

Limit futures dependency to Stream via feature flag #1774

Conversation

carllerche commented Nov 15, 2019 • edited Loading

ipetkov commented Nov 15, 2019

taiki-e commented Nov 15, 2019

vorot93 commented Nov 15, 2019 • edited Loading

carllerche commented Nov 15, 2019

carllerche commented Nov 15, 2019

carllerche commented Nov 15, 2019

seanmonstar commented Nov 15, 2019

carllerche commented Nov 15, 2019

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

Limit `futures` dependency to `Stream` via feature flag #1774

Limit `futures` dependency to `Stream` via feature flag #1774

carllerche commented Nov 15, 2019 •

edited

Loading

vorot93 commented Nov 15, 2019 •

edited

Loading