FillBuf: don't poll a second time on EOF

[edef1c]

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.

[edef1c]

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.

[taiki-e]

Oh, 0.3 is the wrong target branch. Filed #2802 to cherry-pick this to master.

[flokli]

@taiki-e could you release a 0.3.30 containing this fix?

[jessa0]

I just wanna mention that the same change in #2722 was rejected, due to:

I think more correct fix here is avoiding calling fill_buf twice. tokio-rs/tokio@1409041

[taiki-e]

more correct fix here is avoiding calling fill_buf twice. https://github.com/tokio-rs/tokio/commit/1409041

Ah yes, I forgot about that, but it is the more correct way.

[taiki-e]

it is the more correct way.

Filed https://github.com/rust-lang/futures-rs/pull/2812 to do this.

[taiki-e]

2812 has been published in 0.3.30.

[edef1c]

I think more correct fix here is avoiding calling fill_buf twice. https://github.com/tokio-rs/tokio/commit/1409041

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 unsafe {} in this code leading to the complicated hack, and went for the point fix on EOF behaviour, hoping it would save me a lot of controversy in getting a fix merged.