FillBuf: don't poll a second time on EOF #2801
Conversation
There is no hard guarantee that polling a second time will return Poll::Ready, and this is particularly likely to break in the EOF case, which is precisely where we don't need to do so at all. Both tokio::io::BufReader and futures::io::BufReader always attempt to read from the underlying reader when the buffer is empty, rather than fusing EOF.
This fixes EOF handling for buffered readers. Link: rust-lang/futures-rs#2801 Change-Id: Ie98ca6a3e1de38500b0195e9b62511501acb1d2c Reviewed-on: https://cl.tvl.fyi/c/depot/+/10086 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
taiki-e
added
A-io
|
For clarity, given the 0.3-backport label: this was PR'd and merged into the 0.3 branch, since it looked like main was heading towards 0.4. |
|
Oh, 0.3 is the wrong target branch. Filed #2802 to cherry-pick this to master. |
taiki-e
added
0.3-backport: completed
and removed
0.3-backport: pending
|
@taiki-e could you release a 0.3.30 containing this fix? |
|
I just wanna mention that the same change in #2722 was rejected, due to:
|
Ah yes, I forgot about that, but it is the more correct way. |
Filed #2812 to do this. |
|
#2812 has been published in 0.3.30. |
The bugs have been fixed, rust-lang/futures-rs#2801 and rust-lang/futures-rs#2812 were merged and ended up in that release. Change-Id: Iefd990d2d1719b884504093343e54e9c5258e2e2 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10414 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
The bugs have been fixed, rust-lang/futures-rs#2801 and rust-lang/futures-rs#2812 were merged and ended up in that release. Change-Id: I301c0ffc951f04a5b3b7267e922771c837a3f5a9 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10415 Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
FWIW, that's what I did initially while running into the bug. It seemed like the obvious approach, but instead there was a comment about waiting for Polonius to permit it. The borrow checker has no semantic effects on defined behaviour, since it's essentially a linter, so if Polonius can permit it, it is already safe. I inferred there must be a strong prejudice against using |
There is no hard guarantee that polling a second time will return Poll::Ready, and this is particularly likely to break in the EOF case, which is precisely where we don't need to do so at all.
Both tokio::io::BufReader and futures::io::BufReader always attempt to read from the underlying reader when the buffer is empty, rather than fusing EOF.