rust-lang / futures-rs

Zero-cost asynchronous programming in Rust
https://rust-lang.github.io/futures-rs/
Apache License 2.0
5.34k stars 616 forks source link

Fix unreachable match arm in FillBuf being reachable #2722

Closed jessa0 closed 1 year ago

jessa0 commented 1 year ago

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.

taiki-e commented 1 year ago

Thanks for the PR. I think more correct fix here is avoiding calling fill_buf twice. https://github.com/tokio-rs/tokio/commit/1409041525cc81d81b3405c59e5d001b1ec1ddcb

jessa0 commented 1 year ago

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