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

Unhide tokio_tcp::ConnectFuture? #1346

Closed
udoprog opened this issue Jul 23, 2019 · 17 comments
Closed

Unhide tokio_tcp::ConnectFuture? #1346

udoprog opened this issue Jul 23, 2019 · 17 comments

Comments

@udoprog
Copy link
Contributor

udoprog commented Jul 23, 2019

Since #1131 ConnectFuture is hidden behind an impl Future. This makes it hard for libraries that embeds it in their custom futures to use since there is no public named type to use.

While a custom future should be less used in upcoming apis, it's still a papercut for the initial migration.

For example:

Can we reintroduce ConnectFuture to make it easier for downstream crates to migrate to tokio 0.2 when its out?

@udoprog
Copy link
Contributor Author

udoprog commented Jul 23, 2019

If this ends up being a concern, it might be worthwhile looking into other pub apis which have been similarly hidden behind an impl Future.

@udoprog udoprog changed the title Reintroduce tokio_tcp::ConnectFuture? Unhide tokio_tcp::ConnectFuture? Jul 23, 2019
@LucioFranco
Copy link
Member

@udoprog are you able to Box::pin the future? I do agree it is a pain but I'm not sure if its worth it to expose it now because when would we remove it? At 0.2's actual release?

@udoprog
Copy link
Contributor Author

udoprog commented Jul 24, 2019

@LucioFranco

I do agree it is a pain but I'm not sure if its worth it to expose it now because when would we remove it? At 0.2's actual release?

Probably at some later point, when people and libraries have acclimated to new usage patterns of async/await.

But if we still have a need to embed the named future in places, I don't see why we we should hide it at all. Even with existential types, we'd still have to give it a public name to make it usable.

@LucioFranco
Copy link
Member

LucioFranco commented Jul 24, 2019

Even with existential types, we'd still have to give it a public name to make it usable.

Fair point! I think that makes sense and since we don't use async fn yet for connect I think that is fine. Unless the goal is to move to an async fn.

@carllerche
Copy link
Member

@udoprog

Even with existential types, we'd still have to give it a public name to make it usable.

Why is this true?

@carllerche
Copy link
Member

@udoprog the issue is, Tokio is going to be migrating to using async fn instead of all of these futures, so they will not be nameable at all.

@udoprog
Copy link
Contributor Author

udoprog commented Jul 24, 2019

@carllerche

Why is this true?

My bad. I think we'd need to give it a name in our crate, but I guess that depends on how the implementation pans out.

EDIT: this playground showcases the issue right now while A and B have the same constraints they refer to distinct opaque types. It might be supported in the end though.

Tokio is going to be migrating to using async fn instead of all of these futures, so they will not be nameable at all.

Hm. I guess then I'm asking to consider delaying that to give library developers time to adjust: One major release moving to std::future potentially marking these named futures as deprecated. One to go full async fn.

Otherwise the choice for library devs will be boxing or redesigning parts of their code which is doable, but slows down adoption. I do appreciate that it's an undesirable decision.

I'm a bit apprehensive in general exposing public functions without the option of capturing their outputs as existential types, since it makes embedding awkward. I don't know if that's viable though.

@DoumanAsh
Copy link
Contributor

@udoprog existential types are basically can be treated as generic arguments. I'm not sure why in first place they had to name it separately, but basically they are not much different from when you have something like fn lolka<A: Debug, B: Debug>(a: A, b: B) which you want to use when A and B can be different.

But I'm not sure how it is a concern for your public API.
Each Future can be distinguished by it's Output after all, and for the most part all these futures would have different Output

@udoprog
Copy link
Contributor Author

udoprog commented Jul 25, 2019

@DoumanAsh

But I'm not sure how it is a concern for your public API.
Each Future can be distinguished by it's Output after all, and for the most part all these futures would have different Output

This reads like a misunderstanding to me, what I wrote was:

I'm a bit apprehensive in general exposing public functions without the option of capturing their outputs as existential types, since it makes embedding awkward. I don't know if that's viable though.

So using existential types to capture the output of a function would be fine. I'm saying that I'm wary of exposing public async fns in this library unless something like that exists (in stable Rust).

@DoumanAsh
Copy link
Contributor

Well then we should urge stabilisation of existential types because for async fn it is essential thing, as otherwise we're limiting usage of async await to internals only

@LucioFranco
Copy link
Member

I think the one benefit of keeping it hidden is that it will start forcing people to either use Box or migrate to async/await. If we release two breaking changs 0.2 and 0.3 very shortly after, this will create some unneeded fragmentation. A big reason we are still on tokio 0.1 and not any higher versions.

@udoprog does something like boxing work while you wait to rewrite to async fn?

@dekellum
Copy link
Contributor

Interested read this. I hadn't realized feature(existential_type) was this far along.

either use Box or migrate to async/await

In case it helps, there is the possible middle ground of composing an async fn into a (non-async) fn that returns impl Future. See:

https://github.com/dekellum/body-image/blob/futures-compat-uni/body-image-futio/src/futures_03.rs#L28

which composes an async fn (in same file) here:

https://github.com/dekellum/body-image/blob/futures-compat-uni/body-image-futio/src/futures_03.rs#L58

Said in words: The author of fn request_dialog_03 doesn't need to know the exact existential return type of async fn resp_future_03 in order to compose over it and return its own new existential impl Future return type which depends on that prior type.

Apologies if that is already obvious or an uninteresting variant of "migrate to async/await".

@udoprog
Copy link
Contributor Author

udoprog commented Jul 25, 2019

@LucioFranco

I think the one benefit of keeping it hidden is that it will start forcing people to either use Box or migrate to async/await. If we release two breaking changs 0.2 and 0.3 very shortly after, this will create some unneeded fragmentation. A big reason we are still on tokio 0.1 and not any higher versions.

I'm actually bringing this up to avoid fragmentation right now. Pushing people towards structurally rewrite their libraries will be a roadblock to migrate to the next version of Tokio. In my view the nice way to do these sort of things for such a fundamental crate as tokio would be to build new APIs and deprecate old ones over some period of time. Breakage is guaranteed since the Future trait is incompatible. But there's a difference in how painful a migration is between rewriting things locally in place compared to restructuring.

@udoprog does something like boxing work while you wait to rewrite to async fn?

I don't know, I'm not the author of those crates. I'm bringing this up as a papercut I encountered when I tried to upgrade the irc crate to latest master. In my view it's up to Tokio as a project to decide if boxing is a good enough middle ground or not. As folks start migrating I'm sure they'll encounter this, and at least it's good to have a solid answer.

@LucioFranco
Copy link
Member

Pushing people towards structurally rewrite their libraries will be a roadblock to migrate to the next version of Tokio. In my view the nice way to do these sort of things for such a fundamental crate as tokio would be to build new APIs and deprecate old ones over some period of time.

I agree with you on this one, my worry is how much this might hinder us in finding the right api's. I think with such a fundamental change like async/await we are kinda stuck in this spot. I'm not totally sure what the right approach is still :(

In my view it's up to Tokio as a project to decide if boxing is a good enough middle ground or not. As folks start migrating I'm sure they'll encounter this, and at least it's good to have a solid answer.

@carllerche I'd like to hear your thoughts on this

@DoumanAsh
Copy link
Contributor

I'm actually bringing this up to avoid fragmentation right now. Pushing people towards structurally rewrite their libraries will be a roadblock to migrate to the next version of Tokio.

tokio 0.2 will sure take a while because it requires async/await stabilization so I think people will have more than enough time
Hard breaking change is better here because it would require less breaking changes in future

@carllerche
Copy link
Member

Exposing the future types will prevent tokio from using async / await. I rather push towards getting libs using async / await. As mentioned, futures can always be boxed to name them.

@udoprog
Copy link
Contributor Author

udoprog commented Aug 1, 2019

"Permit impl Trait in type aliases" RFC merged a couple of days back (rust-lang/rfcs#2515). Still some ways to go before stabilization but encouraging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants