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

Determine integration story for Futures 0.3 #26

Closed
aturon opened this Issue Jul 23, 2018 · 15 comments

Comments

Projects
None yet
6 participants
@aturon
Copy link
Contributor

aturon commented Jul 23, 2018

In particular: should we follow a 0.1 <-> 0.3 shim approach for now? Or should we consider direct implementation behind a feature flag for key libraries?

@aturon aturon added the futures label Jul 23, 2018

@aturon

This comment has been minimized.

Copy link
Contributor Author

aturon commented Jul 24, 2018

Personally, I think we should get a shim library working ASAP. I believe that's the shortest path to getting Futures 0.3 usable, and immediately makes it possible to use the whole ecosystem. I think it will also be important for existing production uses of Futures 0.1. As time passes, we can also add direct 0.3 support in libraries (under a flag), which will improve the overall user experience.

The overall approach in the existing shim seems like a good starting point to me. However, I'd prioritize ergonomics a bit, and instead of having methods like into_02_compat, have a compat method and Compat type, which can be used to go in either direction. In other words, you'd implement:

  • Future03 for Compat<T> where T: Future01
  • Future01 for Compat<T> where T: Future03

and so on. I'm pretty sure this is feasible. At the very least, though, having a short, consistent method name like compat seems helpful.

I also think we should land this compatibility layer directly in the futures-util 0.3 crate.

@aturon

This comment has been minimized.

Copy link
Contributor Author

aturon commented Jul 24, 2018

@aturon

This comment has been minimized.

Copy link
Contributor Author

aturon commented Jul 24, 2018

@seanmonstar

This comment has been minimized.

Copy link

seanmonstar commented Jul 24, 2018

That sounds excellent to me! Besides centralizing maintenance while experimenting, having a shim will be a big help to the community, as it allows people to upgrade dependencies in steps, instead of requiring all-or-nothing.

@MajorBreakfast

This comment has been minimized.

Copy link
Contributor

MajorBreakfast commented Jul 24, 2018

I absolutely agree that we should develop a shim and that it should be officially supported.

  • Shimming from 0.3 to 0.1 will require boxing for !Unpin stuff. The main compat method could be the one that does boxing and we could add an additional compat_unpin method that does no boxing. I think that !Unpin will be the common case because of async functions. Therefore, it should IMO get the shorter name to a) stay symmetric and b) be more convenient to use
  • We should still intend to transition tokio to real 0.3 (after we release the shim). The compat library should be mainly intended for the wider ecosystem. We won't ever know whether async functions are fast if we box them all the time to make them Unpin and then 0.1 compatible
@aturon

This comment has been minimized.

Copy link
Contributor Author

aturon commented Jul 24, 2018

@tinaun has put together an early prototype of such a shim crate. @MajorBreakfast or @cramertj, would one of you be able to give a review for the overall approach?

@tinaun

This comment has been minimized.

Copy link

tinaun commented Jul 24, 2018

one thing about the "one type" approach - from my experiments it seems to add about a second or two to compile times in my small tests vs one type each for 0.3 -> 0.1 and 0.1 -> 0.3

#![feature(async_await, await_macro)]

use hyper::Client;
use hyper::rt::Stream;
use futures_playground::{Compat01, Compat03, ExecCompat};

use tokio::runtime::Runtime;
use std::io::{self, prelude::*};

fn main() {
    let client = Client::new();
    let fut = client.get("http://httpbin.org/ip".parse().unwrap()).compat();

    let fut = async {
        let res = await!(fut).unwrap();
        println!("{}", res.status());
        // println!("uncomment me!");

        await!(res.into_body().for_each(|chunk| {
            io::stdout().write_all(&chunk)
                .map_err(|e| panic!("example expects stdout is open, error={}", e))
        }).compat());
    };

    let mut rt = Runtime::new().unwrap();
    let fut = fut.compat(rt.executor().compat());

    let _ = rt.block_on(fut);   
}

compiles in about 6.5 seconds with two Compat types, and 8 seconds with one Compat type

even 6.5 seconds still feels pretty bad for a small an example as this - but there sure is a lot going on!

@MajorBreakfast

This comment has been minimized.

Copy link
Contributor

MajorBreakfast commented Jul 24, 2018

@tinaun I kinda like the one type approach: We should try it and see if it works out. If compilation time really turns out to be an issue, we can change it.

Here are some ideas from my side:

let fut03  = /* Some `TryFuture03<Ok = T, Error = E> + Unpin` */;
let both = fut03.compat(); // Future03<Result<T, E>> + Unpin and
                           // Future01<Item = T, Error = E>

let fut03 = /* Some `Future03<Output = T> + Unpin` */;
let both = fut03.never_error().compat(); // Future03<Result<T, Never>> + Unpin and
                                         // Future01<Item = T, Error = Never>

let fut03 = /* Some `Future03<Output = T>` */;
let both = fut03.never_error().boxed().compat(); // Future03<Result<T, Never>> + Unpin and
                                                 // Future01<Item = T, Error = Never>

// ---

let fut01 = /* Some `Future01<Item = T, Error = E>` */;
let both = fut01.compat(); // Future03<Result<T, E>> + Unpin and
                           // Future01<Item = T, Error = E>

let fut01:  = /* Some `Future01<Item = T, Error = Never>` */;
let fut03 = fut01.compat().unwrap(); // Future03<Output = T>

New combinators:

  • compat creates the Compat future. Can be called on TryFuture03 + Unpin and Future01 and returns Compat which implements both Future03<Output = Result<T, E>> and Future01<Item = T, Error = E>
  • boxed boxes the Future03, i.e. makes it Unpin
  • unwrap makes a TryFuture03<Ok = T, Error = E> into a Future03<Output = T>. It panics if there's an error. (Note unwrap_or_else already exists)
  • never_error converts a Future03<Output = T> into a Future03<Output = Result<T, Never>>

Note: TryFuture is used as function input, Future<Output = Result<T, E>> as output. That way input and output both work for both traits.

What do you think?

Edit: Changed box to boxed because box is a keyword
Edit: Boxing works as long as we don't attempt to make it a trait object (which we don't need/want to)


@tinaun Do you want to open a PR? (Only if you want!)

Some suggestions for the PR:

  • Use where clauses
  • Use multiple files
  • Type param naming conventions: Fut = future, F= function, T = item/output, E = error
@tinaun

This comment has been minimized.

Copy link

tinaun commented Jul 24, 2018

ok, i can do that - do you have a preferred way of handling 0.3 Executors? to wrap a Future03 in a Future01 there needs to be an Executor to create a Context - is passing it in the combinator fine? is there a better way to do it?

@MajorBreakfast

This comment has been minimized.

Copy link
Contributor

MajorBreakfast commented Jul 25, 2018

@tinaun I think we need the param because there is AFAIK no mechanism to get the executor from the current Task in 0.1. My futures 0.1 knowledge is really spotty, though

The code by @seanmonstar uses an exec param as well

Let's use a param for now. Maybe we can add an executor to the 0.1 Task by updating the library? It might be possible to do this without breaking the 0.1 public API. (again, I'm not really familiar with 0.1)

@Nemo157

This comment has been minimized.

Copy link
Contributor

Nemo157 commented Jul 27, 2018

I think we need the param because there is AFAIK no mechanism to get the executor from the current Task in 0.1. My futures 0.1 knowledge is really spotty, though

One thing is that tokio provides a globally accessible executor, and as far as I'm aware the majority of futures 0.1 is using tokio's executor. We can provide an additional compatibility shim targeting tokio specifically for when you are running on that executor. (I have this working, but it's currently a bit of a mess, will hopefully open a PR to discuss it more next week once the initially compat PR is merged).

@tinaun

This comment has been minimized.

Copy link

tinaun commented Jul 27, 2018

ideally a tokio shim would allow people to write futures 0.3 + tokio 0.1 code which could be upgraded to use a future version of tokio with default futures 0.3 support as painlessly as possible, i wouldn't want libraries to get stuck in a hopefully intermediate state

@luser

This comment has been minimized.

Copy link

luser commented Jul 30, 2018

FWIW, if anyone wants to see how this pans out in a nontrivial real-world project, I'd love to see this in sccache. We're using a number of tokio crates there along with hyper (as a client). It would be great to figure out the upgrade path to futures 0.3 for that project!

@aturon

This comment has been minimized.

Copy link
Contributor Author

aturon commented Jul 30, 2018

@luser great idea! I think @alexcrichton would be able to help mentor such an effort, as well.

@aturon

This comment has been minimized.

Copy link
Contributor Author

aturon commented Sep 21, 2018

I'm gonna close this out -- we now have multiple compatibility options!

@aturon aturon closed this Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.