Should AsyncRead and AsyncWrite take self by Pin?

[withoutboats]

We didn't do this because there's not a lot of motivation, but on reflection there's also not a lot of downside. Implementations of these traits are a) usually concrete (and therefore Unpin) and b) usually don't need to worry too hard about moving their receiver around in poll_read/poll_write.

The motivation for this change would be that it would enable users to write mock AsyncRead sources from async fns or possibly async generators without boxing them. Possibly someday the same will be true for AsyncWrite once generators with resume arguments are a thing (along the same line as Sink).

cc @cramertj

[Nemo157]

I have briefly experimented with this in embrio, I didn't implement much IO stuff, but one generic adaptor that came up was BufReader. It was pretty straight-forward pin-projection that I really should switch over to using pin-utils/pin-project at some point.

[cramertj]

I'm strongly in favor of this change. Implementing Read/Write with asynchronous generators would be more than occasionally handy.

[Matthias247]

I'm still heavily in favor of #1365
Not being able to implement those traits via futures and async functions makes them pure leaf components and not composable. I can not implement an async (byte) stream as a composition of some Future operations. Which is e.g. exactly what I needed to do in my HTTP/2 implementation for .NET (which I still find pretty clean). The current traits stand out as something that doesn't fit into the remaining model.

Although we can't yet have versions of those traits where the produced Future is able to borrow the IO Object, we can at least have traits that cover the case where the IO object is inside an Arc, which should be fine for longer lived and already expensive objects like sockets.

[cramertj]

@Matthias247 It's not clear what an implementation like that would look like or how it would be object-safe (since async trait methods return futures).

[Matthias247]

Object safety is certainly a problem, since it will be impossible to unify all possible returned Futures. But I'm not sure why we deem it important for these kinds of types, whereas there are enough other types around which are not object safe either.

Types that I am currently using are the followings (error handling omitted):

pub trait Stream {
    type Output;
    type Next: Future<Output = Option<Self:: Output>>;

    fn next(&mut self) -> Self::Next;
}

pub trait Sink {
    type Value;
    type Next: Future<Output = ()>;

    fn send(&mut self, value: Self::Value) -> Self::Next;
}

However Read and Write might be more problematic, because we actually want to pass the read/write buffer by &mut, and thereby need a lifetime on the returned Future (which again doesn't work today).

But still: Standardizing something that sounds like the preferred generic way to do these operations, but which actually is incompatible with the all other primitives, doesn't sound like the right approach. Maybe the current AsyncRead/Write things could be labelled as some kind of stopgap solutions until better traits are available.

[taiki-e]

I have written an experimental implementation of this: master...taiki-e:io-pin

(impl Unpin is removed because pin-project automates this.)

[withoutboats]

I think we should do this and make the change timed to coincide with #1455.

[Matthias247]

I don’t think so. The use case is extremely unclear.
As reasoned above and in the other thread, those traits will be mainly useful in the form of handwritten leaf futures, since they are not composable. I can’t see any scenario where Pin would help for this. In all the current implementations I’m aware of it doesn’t.
What I know is that Pin increases complexity of an interface by a huge margin, and that it’s probably the hardest concept to understand in current Rust (incl memory safety issues when wrongly understood and unsafely cast away). Therefore the bar for including it should be very high.

I also can’t see how generators would be useful for those interfaces. They don’t have any space to store state (eg a returned Future). So where would that Generator state be stored? If it’s inside the implementation of the Trait itself, it can’t interact with the Input parameters.

I think this needs at least a very motivating example what we can’t do without it.

[ghost]

Question: Should all polling methods take self as Pin<&mut Self> (those that start with poll_ and return a Poll<_>)? For example, Tokio's File has a bunch of polling methods - will those use Pin when we upgrade to futures 0.3?

[cramertj]

@stjepang I'd expect that type specifically to implement Unpin, so it wouldn't really matter either way for that concrete type. I think it's right that traits should prefer to use Pin<&mut Self> for polling methods, but for concrete types which are Unpin it seems fine to use &mut self.

[withoutboats]

On reflection I'm actually against this change because it would significantly inconvenience calling the futures adapters of AsyncReadExt and AsyncWriteExt. These tend to take the IO object by &mut self and then call these methods on it; they would have to be pinned instead if the IO object's poll methods took pin, which would be majorly inconvenient. (Unless we bound all the adapters by Self: Unpin, but that would seem to defeat the purpose of this change as well.)

[cramertj]

they would have to be pinned instead if the IO object's poll methods took pin, which would be majorly inconvenient

@withoutboats can you explain why you think this would be inconvenient? It's exactly how most of the rest of the API works. Having to manually reborrow is an annoying issue, but I don't see how it's different for these types than it is across the rest of the library.

[withoutboats]

@cramertj I don't know what you mean, I've looked at the API docs and found these examples that require pinning:

• The select macro
• Stream::next (not relevant since you cant really create a !Unpin stream today)
• Stream::into_future (same)

The vast majority of the "convenience APIs" don't require intermediate pinning. All three of these have long term alternatives that can avoid pinning in the common case (a "race" macro and for await loops I mean). My preference is to avoid intermediate pinning by the end user as much as possible, limiting pinning to an implementation detail of the executor.

[cramertj]

for-await loops aren't an alternative to the into_future combinator, and I don't know what you're imagining "race" does that is better/different from the existing select! macro, which has a long history of design and discussions around why it is the way it is. It's certainly possible to create a !Unpin stream today and I've used a number of them.

There are also a number of methods on Sink that all borrow Self rather than consuming it by-value. These appear all over seeing as they're the primary way of interacting with e.g. channels.

I don't really think the situation here is any different-- that is, it's perfectly fine to expose the convenience APIs on &mut self, it just means that you have to set Self: Unpin, which is a totally reasonable restriction. The result is strictly more powerful than what we have today, and the decision mirrors the existing choices we've made for Stream and Sink.

[withoutboats]

it's perfectly fine to expose the convenience APIs on &mut self, it just means that you have to set Self: Unpin

It seems to me this will make the norm for code that's generic over an IO object to add + Unpin because its going to want to call these APIs. That seems suboptimal to have to track as an additional bound throughout all these libraries, especially since people will be uncertain about whether they should do that or try to manually avoid these combinators (which IMO they should not do).

wouldn't the same code be permitted but without everyone tracking Unpin by keeping the traits how they are and only implement the bridge from a generator to an IO type on a pinned value?

[cramertj]

No-- that doesn't allow owned, non-allocating objects. It also hits a number of significant ergonomics and compiler-implementation issues, as we saw when we attempted to do the same thing for Future. I'm only arguing that we should make the same choice here that we did then.