Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FillBuf: don't poll a second time on EOF #2801

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Conversation

edef1c
Copy link
Contributor

@edef1c edef1c commented Nov 18, 2023

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.

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.
tvlbot pushed a commit to tvlfyi/tvix that referenced this pull request Nov 19, 2023
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
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@taiki-e taiki-e merged commit 0fe4b88 into rust-lang:0.3 Nov 20, 2023
24 checks passed
@taiki-e taiki-e added A-io Area: futures::io 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Nov 20, 2023
@edef1c edef1c deleted the fillbuf-eof branch November 20, 2023 14:30
@edef1c
Copy link
Contributor Author

edef1c commented Nov 20, 2023

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 taiki-e mentioned this pull request Nov 20, 2023
@taiki-e
Copy link
Member

taiki-e commented Nov 20, 2023

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

@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Nov 20, 2023
@flokli
Copy link

flokli commented Nov 23, 2023

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

@jessa0
Copy link

jessa0 commented Nov 23, 2023

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
Copy link
Member

taiki-e commented Nov 23, 2023

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

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

@taiki-e
Copy link
Member

taiki-e commented Dec 7, 2023

it is the more correct way.

Filed #2812 to do this.

@taiki-e
Copy link
Member

taiki-e commented Dec 24, 2023

#2812 has been published in 0.3.30.

tvlbot pushed a commit to tvlfyi/tvix that referenced this pull request Dec 24, 2023
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
tvlbot pushed a commit to tvlfyi/tvix that referenced this pull request Dec 24, 2023
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>
@edef1c
Copy link
Contributor Author

edef1c commented Feb 5, 2024

I think more correct fix here is avoiding calling fill_buf twice. tokio-rs/tokio@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants