futures: add support for std::futures::Future #40

mbilker · 2019-04-03T16:40:59Z

This PR adds a std::future::Future implementation to Instrumented<T>~~, but with some caveats:~~
~~- To support async functions, the inner future must be boxed.~~
~~- If someone can figure out how to support !Unpin futures without boxing, please do so.~~ Thanks hawkw!

hawkw · 2019-07-08T19:08:05Z

@mbilker I took a look at this, and I found a potential solution for supporting !Unpin futures without boxing. Due to lifetime issues, it does require cloning the span, but it may be possible to avoid that in the future.

Here's the diff for my solution.

diff --git a/tracing-futures/Cargo.toml b/tracing-futures/Cargo.toml
index 791e3a49..09fbf75b 100644
--- a/tracing-futures/Cargo.toml
+++ b/tracing-futures/Cargo.toml
@@ -12,6 +12,7 @@ futures = "0.1"
 tracing = "0.1"
 tokio = { version = "0.1", optional = true }
 tokio-executor = { version = "0.1", optional = true }
+pin-utils = "0.1.0-alpha.4"
 
 [dev-dependencies]
 tokio = "0.1.22"
diff --git a/tracing-futures/src/lib.rs b/tracing-futures/src/lib.rs
index 55d63746..a4531493 100644
--- a/tracing-futures/src/lib.rs
+++ b/tracing-futures/src/lib.rs
@@ -37,10 +37,6 @@ pub trait Instrument: Sized {
     fn instrument(self, span: Span) -> Instrumented<Self> {
         Instrumented { inner: self, span }
     }
-
-    fn boxed_instrument(self, span: Span) -> Instrumented<Pin<Box<Self>>> {
-        Instrumented { inner: Box::pin(self), span }
-    }
 }
 
 pub trait WithSubscriber: Sized {
@@ -69,16 +65,23 @@ pub struct WithDispatch<T> {
 
 impl<T: Sized> Instrument for T {}
 
-impl<P: std::future::Future + Unpin> std::future::Future for Instrumented<P> {
-    type Output = P::Output;
 
-    fn poll(self: Pin<&mut Self>, lw: &mut Context) -> std::task::Poll<Self::Output> {
-        let this = self.get_mut();
-        let _enter = this.span.enter();
-        Pin::new(&mut this.inner).poll(lw)
+impl<F: std::future::Future> Instrumented<F> {
+    pin_utils::unsafe_pinned!(inner: F);
+}
+
+impl<F: std::future::Future> std::future::Future for Instrumented<F> {
+    type Output = F::Output;
+
+    fn poll(mut self: Pin<&mut Self>, lw: &mut Context) -> std::task::Poll<Self::Output> {
+        let span = self.as_ref().span.clone();
+        let _enter = span.enter();
+        self.as_mut().inner().poll(lw)
     }
 }
 
+impl<F: std::future::Future + Unpin> Unpin for Instrumented<F> {}
+
 impl<T: futures::Future> futures::Future for Instrumented<T> {
     type Item = T::Item;
     type Error = T::Error;
@@ -115,16 +118,6 @@ impl<T: Sink> Sink for Instrumented<T> {
 }
 
 impl<T> Instrumented<T> {
-    /// Borrows the `Span` that this type is instrumented by.
-    pub fn span(&self) -> &Span {
-        &self.span
-    }
-
-    /// Mutably borrows the `Span` that this type is instrumented by.
-    pub fn span_mut(&mut self) -> &mut Span {
-        &mut self.span
-    }
-
     /// Consumes the `Instrumented`, returning the wrapped type.
     ///
     /// Note that this drops the span.

As an aside, it looks like your CI build is currently failing on the stable and Rust 1.34 builds. I think we should feature-flag std::future support for now, since we want to maintain compatibility with earlier versions. We may want to feature-flag futures 0.1 as well, so that users of std::future need not also pull in the futures crate.

We should also update the CI configuration so that the beta and nightly builds enable the std::future feature flag for the futures crate.

Once we make those changes, I'd be happy to merge this! Thanks for working on it!

mbilker · 2019-07-08T19:34:27Z

@hawkw Thanks for looking into this! I actually had this feature flagged before with the name with-std-future, and I can add it back if that name is good.

I originally removed the feature flag since tokio as a whole was moving to have native std::future::Future support, but then tokio-trace split out from tokio into tracing, so I'll go add the feature flag back.

hawkw · 2019-07-08T19:45:31Z

I actually had this feature flagged before with the name with-std-future, and I can add it back if that name is good.

I'd probably call the feature flags std-future and futures-0.1, personally, but whatever works is fine. :)

mbilker · 2019-07-08T20:00:52Z

I actually had this feature flagged before with the name with-std-future, and I can add it back if that name is good.

I'd probably call the feature flags std-future and futures-0.1, personally, but whatever works is fine. :)

I like those too. Would futures-01 or futures-1 make more sense? I can use futures-0.1 but the line in Cargo.toml is:

[features]
"futures-0.1" = ["futures"]

hawkw · 2019-07-08T20:06:47Z

Hmm, I suppose to avoid quoting, we ought to go with futures-01?

mbilker · 2019-07-08T20:16:03Z

If I make futures-01 a feature flag, should I use tokio::prelude::Future in tracing-futures/src/executor.rs?

tracing/tracing-futures/src/executor.rs

Lines 2 to 5 in f4752ff

    
           use futures::{ 
        
               future::{ExecuteError, Executor}, 
        
               Future, 
        
           };

hawkw · 2019-07-08T20:23:34Z

If I make futures-01 a feature flag, should I use tokio::prelude::Future in tracing-futures/src/executor.rs?

tracing/tracing-futures/src/executor.rs

Lines 2 to 5 in f4752ff

use futures::{

future::{ExecuteError, Executor},

Future,

};

Probably, when the tokio feature is enabled.

I think we may just want to use fully qualified imports for everything now (rather than conditionally importing Future from different places), in order to avoid conflicts when multiple feature flags are enabled.

mbilker · 2019-07-08T20:44:17Z

Alright, I rebased this on the current master branch and pushed a new commit with your suggestions and pin-utils implementation. I left in span and span_mut as the code compiled with them.

mbilker · 2019-07-08T21:09:05Z

I will mark this PR as ready to review when I write some tests or rework the futures 0.1 tests into ones for std::future::Future.

mbilker · 2019-07-08T22:41:01Z

It's actually pretty difficult to use std::future::Future with the released version of tokio and since they removed the async-await-preview and tokio-futures stuff. Using tokio-futures from the v0.1.x branch is not possible with more recent versions of Rust nightly.

In a project where I am using hyper, tokio, and std::future::Futures, I pulled the compatibility modules for providing a futures 0.1 shim interface for std::future::Future from tokio's commit history as it was removed. It definitely works alright. I'll have to see what I can do here to make sure the std::future::Future stuff is tested.

hawkw · 2019-07-08T23:57:41Z

I'm not sure if we'll need tokio to implement most of the fairly simple tests in tracing-futures for std::future, but if we do, I'm okay with taking out git dependencies on tokio crates for now. We'll want to adopt the crates.io alpha releases as they come out though.

hawkw · 2019-07-10T21:50:08Z

Hi @mbilker, what's the current status of this? It would be great to merge this soon!

mbilker · 2019-07-10T22:34:19Z

I have tested it with a project of mine that is using async functions and the await features enabled and it’s working there. I am still trying to get tokio-test up and running in the test suite.

mbilker · 2019-07-10T22:36:12Z

I do still need to test variations of this pattern as there was a case a few weeks back where if I didn’t do an instrument on a future and awaited that future, then the future wouldn’t follow the current span.

hawkw · 2019-07-10T22:57:11Z

Okay, if there anything I can do to help out, please let me know!

mbilker · 2019-07-11T01:44:36Z

I went ahead and used MockTask from tokio-test to provide a bare-bones Waker that can be used for polling the futures to completion. I wrote a small test harness that polls those futures to completion and returns the value from Poll::Ready.

The tests run successfully on my machine with cargo test --features=std-future. It looks like something happened in regards to the serde_derive and syn crates that is causing nightly to fail.

Here is where I'll ask for your help, is there a way to only conditionally include tokio-test for Rust nightly, but not Rust stable or beta? I expect there to be no solution to this, unfortunately.

mbilker · 2019-07-14T07:35:45Z

I can do that

- To support async functions, the inner future must be boxed. - If someone can figure out how to support !Unpin futures, please do so.

- Thank you Eliza Weisman (hawkw) for the solution using the pin-utils crate - Added feature gates for the futures 0.1, tokio, and std::future implementations

mbilker · 2019-07-14T09:37:30Z

Actually, since the cargo test --all does not do non-default feature flags, I can add another script to the Travis config to do all the features from tracing-futures. This also brings about the issue that any non-default feature has not been running in the CI for some time now...

mbilker · 2019-07-14T09:39:55Z

I put up the tests over at https://github.com/mbilker/tracing/tree/std-future-test

mbilker · 2019-07-14T09:41:44Z

Since I did a push (and rebase) with and added Travis section to test all features from tracing-futures, it ran those tests in https://travis-ci.org/tokio-rs/tracing/builds/558472465 and the tests passed.

hawkw

This LGTM and I'm happy to merge it as-is.

From a code organisation perspective, I'm wondering if it might be nicer to put the feature-flagged code for futures-01 and std-future in separate modules, with the cfg attributes on the modules, and then re-export the contents of those modules? It would mean we wouldn't have to feature-flag as many individual items. However, this isn't a blocker and we can always re-organise the code later.

tracing-futures/src/executor.rs

hawkw · 2019-07-14T18:23:13Z

tracing-futures/src/lib.rs

@@ -157,7 +191,9 @@ mod tests {
    extern crate tokio;

    use super::{test_support::*, *};
-    use futures::{future, stream, task, Async};
+
+    #[cfg(feature = "futures-01")]


Instead of putting cfg attributes on each individual import and test in this module, what do you think of having separate test modules for futures-01 and std-future? Then we can just feature-flag the entire module, and not have to worry about individual items within that module.

That works for me. Both test modules would use the same base PollN so there will also need to be a third module that is used by both testing modules.

The feature-flagged modules could be submodules of the main test module?

It definitely works like that. I can make it a separate top-level module, but then I have to mark PollN as pub(crate) and it's two constructors as pub(crate).

hawkw · 2019-07-14T18:26:07Z

Also, how would you feel about just merging the tests added in https://github.com/mbilker/tracing/tree/std-future-test into this branch, and merging the whole thing in one commit? I'd prefer to add tests and new code together when practical.

mbilker · 2019-07-14T18:27:23Z

That works for me, but do note the addition of tokio-test as a dev dependency causes tests to fail under stable and beta because tokio-test uses a #![feature(...)] flag.

mbilker · 2019-07-14T18:29:22Z

But! I wonder if moving those tests to a separate subcrate, like the tracing tests with the log feature, would allow this all to work.

hawkw · 2019-07-14T21:32:50Z

That works for me, but do note the addition of tokio-test as a dev dependency causes tests to fail under stable and beta because tokio-test uses a #![feature(...)] flag.

Ah, right, I had forgotten about that. Let's merge this PR now, and merge the tests later. My bad.

But! I wonder if moving those tests to a separate subcrate, like the tracing tests with the log feature, would allow this all to work.

Yeah, if it's not in the workspace, we can run those tests only on nightly builds. Sounds good to me.

… the nightly toolchain

mbilker · 2019-07-14T23:47:07Z

That should be everything. Please take a look and leave comments on what I should clarify.

hawkw

This looks great! Thanks for the contribution.

tracing-futures/src/executor.rs

tracing-futures/test_std_future/tests/test.rs

- test_std_future is part of the namespace so having std_future as part of the test function name is not needed

hawkw · 2019-07-15T22:55:23Z

This looks great, thanks for fixing the last nit around test names (which was entirely optional). I'm going to merge this now.

Thanks again for all the work that went into this change, @mbilker! I know it took a while to get through, and all your time is really appreciated.

mbilker force-pushed the std-future branch 2 times, most recently from ac50c45 to c51bfd9 Compare April 11, 2019 01:51

mbilker force-pushed the std-future branch 2 times, most recently from d06efbc to 2d09719 Compare April 19, 2019 20:05

mbilker force-pushed the std-future branch from 2d09719 to 0cd0ff5 Compare May 25, 2019 08:24

mbilker force-pushed the std-future branch from 0cd0ff5 to a05d1cd Compare June 22, 2019 03:38

mbilker force-pushed the std-future branch 3 times, most recently from 55bc69f to 3aaf140 Compare July 4, 2019 05:48

mbilker force-pushed the std-future branch from 3aaf140 to 73d2126 Compare July 8, 2019 20:43

mbilker force-pushed the std-future branch from 73d2126 to b720bd9 Compare July 8, 2019 20:53

hawkw self-requested a review July 10, 2019 22:57

mbilker marked this pull request as ready for review July 11, 2019 01:31

mbilker changed the title ~~futures: add support for std::futures::Future and futures 0.3~~ futures: add support for std::futures::Future Jul 11, 2019

mbilker added 8 commits July 14, 2019 09:08

futures: add support for std::futures::Future and futures 0.3

4c5e989

- To support async functions, the inner future must be boxed. - If someone can figure out how to support !Unpin futures, please do so.

futures: support std::futures::Future with !Unpin futures

15c663a

- Thank you Eliza Weisman (hawkw) for the solution using the pin-utils crate - Added feature gates for the futures 0.1, tokio, and std::future implementations

futures: fix formatting

00e44b3

futures: rustfmt

a332aed

futures: use tokio-test for testing std::future::Future unit tests

a93068d

futures: rustfmt

a24b1b4

futures: remove futures_api conditional include

5abf588

futures: add testing of all features under nightly

8097d08

mbilker force-pushed the std-future branch from 95171b8 to 8097d08 Compare July 14, 2019 09:12

mbilker force-pushed the std-future branch from 8097d08 to a332aed Compare July 14, 2019 09:39

hawkw approved these changes Jul 14, 2019

View reviewed changes

mbilker added 2 commits July 14, 2019 23:16

futures: separate std-future tests into submodule that is only run on…

94a128c

… the nightly toolchain

futures: move futures-01 tests to submodule

d21ef29

mbilker force-pushed the std-future branch from c82a1d1 to d21ef29 Compare July 14, 2019 23:28

futures: fix travis build

3e9f577

hawkw approved these changes Jul 15, 2019

View reviewed changes

tracing-futures/src/executor.rs Show resolved Hide resolved

tracing-futures/test_std_future/tests/test.rs Outdated Show resolved Hide resolved

future: remove extraneous part of function names

385cbca

- test_std_future is part of the namespace so having std_future as part of the test function name is not needed

hawkw merged commit d6750d0 into tokio-rs:master Jul 15, 2019

mbilker deleted the std-future branch July 15, 2019 23:23

davidbarsky mentioned this pull request Jul 18, 2019

Support std::future #117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

futures: add support for std::futures::Future #40

futures: add support for std::futures::Future #40

mbilker commented Apr 3, 2019 •

edited

Loading

hawkw commented Jul 8, 2019 •

edited

Loading

mbilker commented Jul 8, 2019

hawkw commented Jul 8, 2019

mbilker commented Jul 8, 2019

hawkw commented Jul 8, 2019

mbilker commented Jul 8, 2019

hawkw commented Jul 8, 2019 •

edited

Loading

mbilker commented Jul 8, 2019

mbilker commented Jul 8, 2019

mbilker commented Jul 8, 2019 •

edited

Loading

hawkw commented Jul 8, 2019

hawkw commented Jul 10, 2019

mbilker commented Jul 10, 2019

mbilker commented Jul 10, 2019

hawkw commented Jul 10, 2019

mbilker commented Jul 11, 2019

mbilker commented Jul 14, 2019

mbilker commented Jul 14, 2019

mbilker commented Jul 14, 2019

mbilker commented Jul 14, 2019

hawkw left a comment

hawkw Jul 14, 2019

mbilker Jul 14, 2019

hawkw Jul 14, 2019

mbilker Jul 15, 2019

hawkw commented Jul 14, 2019

mbilker commented Jul 14, 2019

mbilker commented Jul 14, 2019

hawkw commented Jul 14, 2019

mbilker commented Jul 14, 2019

hawkw left a comment

hawkw commented Jul 15, 2019

futures: add support for std::futures::Future #40

futures: add support for std::futures::Future #40

Conversation

mbilker commented Apr 3, 2019 • edited Loading

hawkw commented Jul 8, 2019 • edited Loading

mbilker commented Jul 8, 2019

hawkw commented Jul 8, 2019

mbilker commented Jul 8, 2019

hawkw commented Jul 8, 2019

mbilker commented Jul 8, 2019

hawkw commented Jul 8, 2019 • edited Loading

mbilker commented Jul 8, 2019

mbilker commented Jul 8, 2019

mbilker commented Jul 8, 2019 • edited Loading

hawkw commented Jul 8, 2019

hawkw commented Jul 10, 2019

mbilker commented Jul 10, 2019

mbilker commented Jul 10, 2019

hawkw commented Jul 10, 2019

mbilker commented Jul 11, 2019

mbilker commented Jul 14, 2019

mbilker commented Jul 14, 2019

mbilker commented Jul 14, 2019

mbilker commented Jul 14, 2019

hawkw left a comment

Choose a reason for hiding this comment

hawkw Jul 14, 2019

Choose a reason for hiding this comment

mbilker Jul 14, 2019

Choose a reason for hiding this comment

hawkw Jul 14, 2019

Choose a reason for hiding this comment

mbilker Jul 15, 2019

Choose a reason for hiding this comment

hawkw commented Jul 14, 2019

mbilker commented Jul 14, 2019

mbilker commented Jul 14, 2019

hawkw commented Jul 14, 2019

mbilker commented Jul 14, 2019

hawkw left a comment

Choose a reason for hiding this comment

hawkw commented Jul 15, 2019

mbilker commented Apr 3, 2019 •

edited

Loading

hawkw commented Jul 8, 2019 •

edited

Loading

hawkw commented Jul 8, 2019 •

edited

Loading

mbilker commented Jul 8, 2019 •

edited

Loading