fix: BufWriter must not re-poll an already-completed future after an error - #856
Open
arbelonson-source wants to merge 1 commit into
Open
arbelonson-source wants to merge 1 commit into
arbelonson-source wants to merge 1 commit into
Conversation
…error BufWriter's poll_write/poll_flush/poll_shutdown, and the plain-async put(), all handled the Prepare/Flush state's inner future the same way: on Ok, transition state and continue; on Err, propagate the error via `?` inside `ready!(...)` (or `.await?`) without ever updating `self.state`. Since `?` short-circuits before the state assignment runs, `self.state` is left sitting on the same future that just returned Ready -- if write/flush/ shutdown is called again afterward (e.g. calling close() after write() already failed, as reported), that same, already-completed future gets polled a second time, which panics with `` `async fn` resumed after completion `` instead of returning an error. Adds a terminal `BufWriterState::Errored` variant. Every place that used to propagate a Prepare/Flush error via `?` now instead transitions into Errored and returns the error explicitly; every state-matching function (poll_write, poll_flush, poll_shutdown, put, abort) has a corresponding Errored arm. abort() treats Errored the same as Buffer/Prepare (Ok(())), since a failed Prepare/Flush future never durably created anything remotely to clean up. Fixes apache#810 Generated-by: Claude Opus 5, via Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #810.
Rationale for this change
BufWriter::poll_write,poll_flush,poll_shutdownand the plain-asyncput()all handle thePrepare/Flushstate's inner future the same way: onOkthey move to the next state, onErrthey propagate with?insideready!(...)(or.await?) without touchingself.state. Because?short-circuits before the state assignment,self.stateis left holding the future that just completed. The next call — e.g.close()after a failedwrite(), exactly the sequence in #810 — polls that finished future again and panics with`async fn` resumed after completioninstead of returning an error.What changes are included in this PR?
BufWriterState::Erroredvariant. Every site that used to propagate aPrepare/Flusherror via?now transitions intoErroredand returns the error explicitly; every state-matching function (poll_write,poll_flush,poll_shutdown,put,abort) gets anErroredarm that returns an error rather than polling anything.abort()treatsErroredlikeBuffer/Prepare(Ok(())): a failedPrepare/Flushfuture never durably created anything remote to clean up.InMemorystore and then callflush,shutdownandabort. With only the production change reverted, theflushandshutdowntests fail with the original panic at the state machine; theabortone already passed and is there to pin that behaviour.cargo test --lib buffered,cargo fmt --checkandcargo clippy --all-targets --all-features -- -D warningsare clean on top of currentmain(279572e).Are there any user-facing changes?
Behavioural only: after a failed write, a further
flush/shutdown/writeon the sameBufWriternow returns anErrinstead of panicking. No API changes.Disclosure, as offered on #810 and per the ASF generative-tooling guidance: this change was produced by an autonomous agent (the code by Claude Opus 5, the rebase and re-verification today by Claude Fable 5.1, both via Claude Code) on behalf of the account owner; the commit carries a
Generated-by:trailer. Happy to adjust anything.