Bad completion of some futures in io::copy_bidirectional

[Armillus]

Bad completion of some futures in io::copy_bidirectional

[Darksonn]

Yes. If your IO resource relies on futures internally, then the poll_flush that finishes the flush future the N bytes must not return Ready. Instead, that call to poll_flush should start another flush operation for the remaining X bytes. You don't return Ready until the X bytes have also been flushed.

[mox692]

I could fixed the problem by copying this code here, just before the loop.

This could be a fix, but I think that ideally it should be implemented in such a way that reads to buffer do not stop if there is room in the buffer but flushing future is pending.

[Darksonn]

Reading before writing to include more data in the write makes sense to me.

I'll close this issue, but if you want to adapt the PR to optimize something, then that's fine with me.

[Armillus]

I could fixed the problem by copying this code here, just before the loop.

This could be a fix, but I think that ideally it should be implemented in such a way that reads to buffer do not stop if there is room in the buffer but flushing future is pending.

I agree, that would be definitely more efficient. As I imagine it, this solution would look like this:

loop {
    // If our buffer is empty, then we need to read some data to
    // continue.
    if self.pos == self.cap && !self.read_done {
       // This code remains the same
    }

    // If a flush future is in progress, do not write until it is finished
    if self.need_flush {
        ready!(writer.as_mut().poll_flush(cx))?;
        #[cfg(any(
          feature = "fs",
          feature = "io-std",
          feature = "net",
          feature = "process",
          feature = "rt",
          feature = "signal",
          feature = "sync",
          feature = "time",
        ))]
        coop.made_progress();
        self.need_flush = false;
    }
    
    // Remaining code is untouched
}

Note that any error during flushing is ignored in my code, since it is also ignored elsewhere in the function. The flushing code could probably be factorized, since it's the same as the one in case of a pending read.

Besides, I think that we can fix the other similar problem coming from this optimization (read future may never be properly polled upon termination) by changing the condition at the entry of the loop:

loop {
    // If our buffer is empty, then we need to read some data to
    // continue. Otherwise, if we can read a bit more data, do it
    // to improve the chances of a large write
    let is_readable = (self.pos == self.cap) || (self.cap < self.buf.len());
    
    if is_readable && !self.read_done {
       // This code remains the same
    }

    // Remaining code is untouched
}

[Armillus]

While digging into the first issue I had (with just a poll_write() implementation), I could confirm that this optimization is partially meaningless for now. Indeed, the current implementation will not poll again a pending read future before the next poll_write(), which is not fatal, but makes the whole thing useless if the reader is not immediately ready.

Hence, I've reworked my fixes and implemented them in the related PR. With those fixes, everything works as expected, both in the real project where I spotted the issue and the minimal example provided here.

The fix related to the flush future is definitely the most important since it breaks the poll_flush contract, but the modification of the read condition seemed to improve a bit the overall performance in my case (high workload with a lot of data flowing from especially one side).

[Darksonn]

If poll_flush returns Ready and you haven't flushed everything, then your IO resource is incorrect.

[Armillus]

If poll_flush returns Ready and you haven't flushed everything, then your IO resource is incorrect.

So if I write N bytes of data, then I start to flush (thus getting a Poll::Pending), then I write again X bytes of data, to eventually flush again and getting a Poll::Ready(Ok(())), it will necessary mean that N + X bytes of data have been flushed properly?

[Armillus]

Ok, thank you very much, I did not understand it this way. The documentation is not very explicit in this sense in my opinion, but maybe that I'm the only one to think so and that I've misunderstood its wording.

Hence, I guess that the only thing that still makes sense in this issue and the related PR is about this optimization. It's a minor detail, but with the current implementation, if the read operation is pending, then the next write might occur before the next read, without letting any chance for a second poll for the read operation. In other terms, we could potentially allow for more optimizations by trying to read a second time before writing again when both operations are pending. What do you think @Darksonn?