Migrating the net library to async-net?

[erickt]

In the style of #836, I noticed that async-std's net is nearly the same as https://github.com/smol-rs/async-net. Would it be worth porting async-std to use async-net?

[yoshuawuyts]

@erickt oh yes, that'd be great! -- I believe that would allow us to pull in ringbahn later on with fairly little effort as well, as those crates somewhat mirror each other.

[Kestrer]

I don't think this would be possible due to async-std's implementation of I/O traits on references. The problem is that async-std currently (and incorrectly, since wake-ups get lost) implements Async{Read, Write} on shared references to its I/O types, whereas async-net avoids this. Without making changes to async-net it's not possible to add this functionality, and I think it would be better to avoid these implementations in general because it's such a footgun.

Since async-std 2 is going to have to be released at some point anyway (for AsyncIterator) the removal of implementations of I/O traits on shared references could be done as a part of that.

[yoshuawuyts]

@Kestrer the only thread related to references in async-net that I can find is this 1. Admittedly I haven't looked particularly closely at issues recently, but I'm not aware of any synchronization issues in async-std's IO primitives that would not also occur in std's IO primitives.

Do you have any references you could share on this?

[Kestrer]

@yoshuawuyts The problem I was talking about was that, due to AsyncRead's poll-based design, each AsyncRead instance should hold one waker which it sets on poll_read and wakes when the resource becomes readable. However, an &TcpStream cannot hold a waker - the internal TcpStream can, but there may be more references and thus more wakers that need to be registered than the one waker the TcpStream itself holds. And because different &TcpStreams cannot be differentiated, it's not like each stream can store a map of different references to wakers.

Previously I assumed that async-std would just lose wakeups here (hence my comment), but it turns out async-std does avoid that. Because Async::poll_readable will wake the old waker if a new waker is seen but the resource has not yet become readable, two tasks with &TcpStreams (or cloned TcpStreams) competing for the waker slot will get caught spinnning in a cycle of continually waking each other up. This does avoid the wakups getting lost issue, but isn't an ideal solution.

One option is to remove the Clone and "I/O traits on references" APIs altogether, but that is a severe loss of functionality. Another option is to not implement the I/O traits on references, but still keep the Clone based API. This would involve changing Async::readable and Async::writable to return named 'static futures - if we're lucky, we could implement this by just cloning the inner Arc<Source> to make them 'static, but that may cause bugs if readable/writable assume the I/O resource has not been dropped, so that'll have to be investigated. Then TcpStream could store both an Arc<Async<std::net::TcpStream>> and a Readable and Writable. AsyncRead and AsyncWrite would poll the Readable and Writable, and cloning the TcpStream would generate new Readable and Writable futures with their respective methods on Async.

Edit: Ah, I see the async-net does do basically the solution I mentioned above, but it boxes and type-erases the futures. I was sort of considering that to not be an option when writing the above paragraph due to the overhead it would have, but if we switch to async-net anyway it will be fine. And we can always optimize later.