Tokio: AsyncFd usability issues

[Keruspe]

AsyncFd has (imo) some usability issues which could be improved quite easily (I can provide patches if the suggested solutions sounds good to you.

Let's say I have a custom TLS stream, that is provided by an external source and has not been created by tokio. the naive approach would be to register it using AsyncFd:

let stream = AsyncFd::new(stream);

Then if I want to e.g. write to it using some poll_write-like method:

let guard = ready!(stream.poll_write_ready(cx))?;
guard.with_io(|| stream.write(buf))

This looks ok buf if I need a mutable reference for writing, this won't compile since there already is a reference stored in the guard.
Current workaround it to create the AsyncFd with just the raw fd from the stream and cache the readable/writable state, which is clearly non optimal.

What I suggest:

• First, in the with_io closure, the guard should pass its reference to the inner stream: guard.with_io(|stream| stream.write(but))
• Second, either make poll_write_ready and firends take a &mut and pass the &mut to with_io, or add some new poll_write_ready_mut which does so with some AsyncFdReadyGuardMut or such.

WDYT?

[bdonlan]

These suggestions make sense (and some of them resemble what we've discussed in other threads), but I'd be inclined to wait for 1.0, given that 1.0 is coming soon, the with_io change is not backwards compatible (and adding another method would be confusing) and workarounds exist (eg, using a Mutex, or using a raw fd and keeping state elsewhere).

[Darksonn]

I think this is certainly something we should improve.

[arcnmx]

I tried replacing PollEvented with AsyncFd and ran into the same issue; AsyncFd taking ownership of the fd (combined with the guard borrowing it) makes it unusable. So +1 to both suggestions here!

[bIgBV]

@LucioFranco I can pick this up.

[arcnmx]

Another usability point I got bit by yesterday but now seems obvious in hindsight... it's awkward that clearing the readiness flag doesn't register the task waker, so one might end up having to use it like...

loop {
    let mut ready = ready!(fd.poll_read_ready(cx))?;
    match ready.with_io(|| file.read(buffer)) {
        Err(e) if e.kind() == ErrorKind::WouldBlock => continue,
        res => break Poll::Ready(res),
    }
}

If with_io returned Poll<io::Result<T>> instead, then it would be more fool-proof and obvious how to use it. It seems kind of silly to return Poll::Ready(Err(ErrorKind::WouldBlock)) and hope the poll is called again by the caller, but that also seems like it might be what the current api is encouraging?

[carllerche]

@arcnmx I agree. We should make that change.

Who here is going to work on the PR for these changes?

[bIgBV]

@carllerche I wanted to take a stab at this.

[carllerche]

Changes have been merged to master. Thanks all.

Also, this is the final Tokio 1.0 issue :tada: