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

Fix unreachable match arm in FillBuf being reachable #2722

Closed
wants to merge 1 commit into from

Conversation

jessa0
Copy link

@jessa0 jessa0 commented Mar 16, 2023

I hit this unreachable!() inside FillBuf with an AsyncRead implementation (wrapped in a BufReader) which calls a Javascript callback thru neon (which is an async operation) for the actual read, which means every "new" poll_read (i.e. either the first call ever or the first call after a Ready is returned) will return Pending at least once, including after EOF is hit, which is what triggers this bug. Of course, the AsyncRead implementation could track when EOF is hit and return Ready(0) forever after that, which is indeed what I used as a workaround, but I can't find anything in the AsyncRead documentation which actually requires implementors to do this.

Alternatively to this fix, the AsyncRead documentation could require implementors to always return Ready(0) after an EOF, and the unreachable!() changed to a panic!(), but that seems like more potential downstream work than merging this fix, and seems easy to screw up.

Another similar alternative would be for the AsyncBufRead documentation to require implementors to always return Ready(&[]) after an EOF.

The added test triggers the panic if run without the bug fix.

@jessa0 jessa0 requested a review from taiki-e as a code owner March 16, 2023 00:13
@taiki-e
Copy link
Member

taiki-e commented Mar 18, 2023

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

@jessa0
Copy link
Author

jessa0 commented Mar 18, 2023

That would be better indeed. If you're willing to merge that tokio commit, want me to bring it into this PR for you?

@jessa0 jessa0 closed this Jun 15, 2023
@jessa0 jessa0 deleted the fix-fill-buf-unreachable branch June 15, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants