tokio-rs / tokio

A runtime for writing reliable asynchronous applications with Rust. Provides I/O, networking, scheduling, timers, ...
https://tokio.rs
MIT License
26.76k stars 2.46k forks source link

Exposing combinators #2723

Open Darksonn opened 4 years ago

Darksonn commented 4 years ago

We have considered exposing the combinators of streams as well as AsyncRead and such. This issue tracks whether we want to do this, however the general opinion has been that they shouldn't be public in v1.0, which is not far away (#2718), even if they are temporarily made public in v0.2.

In particular the return types of the following methods would become public:

To avoid cluttering the stream module, we could create a tokio::stream::adapters or tokio::stream::combinators module.

A quick overview of pros & cons:

Pros:

Cons:

It is worth noting that we already have some public combinators, e.g. io::Take and io::StreamReader, as well as many public leaf-IO resources such as io::Empty that are in some sense similar to combinators. We should consider moving these into new modules if we do that.

hawkw commented 4 years ago

IMO, the main benefit for keeping these types private is to reserve the ability to replace them with async fns if/when that becomes possible. I'm less concerned about cluttering the documentation with combinator types, as we could also add #[doc(hidden)] attributes, or export them all in a submodule so that they don't show up in the main stream module.

If reserving the ability to make them async fns someday is the primary reason to keep them private, we should then think about whether this is likely to happen. If/when async fn trait methods are possible on stable, will we want to rewrite these combinators to be async fns?

It's worth noting that if we want to reserve the ability to use async fns for these methods eventually, that is still a breaking change with the current API surface. Currently, the various Stream combinators will all automatically impl Unpin if all their type parameters do, while an async fn-based implementation will never be Unpin. Therefore, using async fn would be a breaking change.

If being able to replace these combinators with async fns is something we care about, we should probably also add PhantomPinned to them in 0.3, making them !Unpin. If we don't make them !Unpin, we are essentially committing to not replacing them with async fn for the next three years after when Tokio 1.0 is released. From my perspective, if we don't make the combinators !Unpin, then we should just go ahead and export the types publicly, especially since many users seem to want this.

Darksonn commented 4 years ago

Note that this issue is specifically about combinators, and that nothing in the list of methods actually returns a Future.

hawkw commented 4 years ago

Whoops! In that case, the motivation for not exposing the combinators that I mentioned doesn't really apply at all.

alce commented 4 years ago

As far as I understand, there are at least two things to consider:

Another possibility can be not to use combinators at all and return Pin<Box<dyn Stream<...>>> in lieu of impl Stream<..> although it seems a little heavy handed.

Edit: ...on second thought the Pin<Box<...>> thing doesn't make sense, does it?

carllerche commented 4 years ago

This is not critical to solve for 0.3. I am going to punt.

osa1 commented 3 years ago

In the meantime, is there anything users can do to e.g. pass return value of StreamExt::fuse to a function? Because the type is not public, we currently can't implement a function that takes return value of StreamExt::fuse as argument. Do we have any workarounds for this?

Darksonn commented 3 years ago

Sure, you can use generics to accept any stream.

fn takes_stream<S: Stream<Item = ...>>(s: S) {
    ...
}

In the specific case of fuse, you could also use the analogous method from the futures crate.

chapa commented 3 years ago

Hi!

Until combinators of streams are public, is it possible to write a function that returns a Throttle?

pub fn foo() -> /* what do I put here ? */ {
    stream::iter(vec![1, 2, 3]).throttle(Duration::from_secs(1))
}

If I try to use a generic type:

pub fn foo<S: Stream<Item = i32>>() -> S {
    stream::iter(vec![1, 2, 3]).throttle(Duration::from_secs(1))
}

I get the error:

pub fn foo<S: Stream<Item = i32>>() -> S {
           - this type parameter       - expected `S` because of return type
    stream::iter(vec![1, 2, 3]).throttle(Duration::from_secs(1))
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter `S`, found struct `stream::throttle::Throttle`

I don't understand, why isn't Throttle a S?

nytopop commented 3 years ago

@chapa

I don't understand, why isn't Throttle a S?

Because S is decided by whoever calls foo, not foo. For example, someone might attempt to call it in such a way that S is not the return type of stream::iter, as in foo::<SomethingElse>().

Returning an impl Trait should work in this case:

pub fn foo() -> impl Stream<Item = i32> {
    stream::iter(vec![1, 2, 3]).throttle(Duration::from_secs(1))
}
Darksonn commented 3 years ago

Yeah, the throttle stream may not be an S, since the caller might have chosen S to be some other stream type than Throttle.

chapa commented 3 years ago

Ok thank you, impl Trait solves the problem in that case indeed. Actually I should have explain my real problem instead of trying to find a simpler equivalent that is not so equivalent :sweat_smile:

I wanted a trait with a method that returns a Stream but I couldn't have impl Stream as return type because "impl Trait not allowed outside of function and inherent method return types". And since I can't use generics on the method (as we saw here), I ended up with an associated type :

pub trait MyTrait {
    type Stream: Stream<Item = i32>;

    fn foo(&self) -> Self::Stream;
}

But now I'm stuck on trait implementations, I would have liked to do something like this :

pub struct MyStruct {}

impl MyTrait for MyStruct {
    type Stream = impl Stream<Item = i32>;

    fn foo(&self) -> Self::Stream {
        stream::iter(vec![1, 2, 3]).throttle(Duration::from_secs(1))
    }
}

But impl Trait in type aliases is unstable and needs the type_alias_impl_trait feature.

Is there another way to have a trait's method returning a Stream ? (without enabling unstable features)

asonix commented 3 years ago

Rather than returning an impl Stream you could try returning a Box<dyn Stream> or a Pin<Box<dyn Stream>> and suffer the allocation until it's possible to return impl Foo in traits

chapa commented 3 years ago

It works well with a Pin<Box<dyn Stream>>, thank you! I'm not sure if I prefer this or using the unstable feature, but now I know it's possible without it.

carllerche commented 3 years ago

Exposing combinators is not a breaking change. I am going to remove the 1.0 tag.

talchas commented 3 years ago

Is it a good idea in the first place to have these combinators which cause conflicts with futures::stream::StreamExt but don't replace futures entirely? timeout and throttle obviously are tokio-specific, and all, any, merge at least don't exist at the moment in futures. The rest however cause name conflicts the moment you need any of the combinators in futures that don't exist in tokio.

carllerche commented 3 years ago

Tokio aims to provide everything commonly needed to implement an async app with Rust.

NeoLegends commented 3 years ago

This this a decision you intend to stick to or is that something you might want to reconsider in the future? Reason being I presume most projects will rely on both futures and tokio at the same time and it might just make the adoption story easier for users if those conflicts didn't arise.

That said, that's a decision that can be made when time comes to decide whether to expose the combinators, or not.

Darksonn commented 3 years ago

Note that with the introduction of the async-stream crate, exposing the various combinator types in that crate is something we can experiment with, without having to commit to that for Tokio 1.0.

ahornby commented 3 years ago

Hi, I'd like to be able to call Chain::into_inner() to get back to the two component parts, what's the recommended route to be able to do that?

alcjzk commented 2 years ago

Why not outsource this decision to the users of the crate, by having combinators re-exported in a module(s) behind a feature flag? That way those who need access to these types can choose said access with the cost of some extra namespace clutter.

I don't really see why this would need to be an API level restriction.

Darksonn commented 2 years ago

I am open to exposing the combinators in tokio-stream, but I don't think it makes sense to hide them behind a feature flag.

tazjin commented 1 year ago

This is required for cases where a concrete combinator type must be named for whatever reason (e.g. during a trait implementation where there is nothing else to "attach" a type parameter to).