fix: Http2Adapter does not fail with StateError when connection is closed#2469
fix: Http2Adapter does not fail with StateError when connection is closed#2469vanelizarov wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in the Http2Adapter that caused unhandled StateError: Bad state: Cannot add event after closing exceptions when the server closes the connection before the client finishes sending the request body.
Key Changes:
- Replaced the simple stream listener with a more robust approach using
StreamSubscription,Completer, and explicit error handling to gracefully handle StateError - Added try-catch block around the final
stream.outgoingMessages.close()call to prevent StateError propagation - Improved cancellation handling by properly cancelling subscriptions before closing streams
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| plugins/http2_adapter/lib/src/http2_adapter.dart | Refactored request stream handling to catch StateError during data transmission and gracefully complete the request without propagating the error |
| plugins/http2_adapter/test/http2_test.dart | Added test case that simulates server closing connection early to verify the StateError fix |
| plugins/http2_adapter/CHANGELOG.md | Documented the bug fix in the unreleased section |
| .gitignore | Added .history directory to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@AlexV525 should I take into account comments from AI or will you do the review manually? |
@vanelizarov AI might give helpful suggestions. If you have opposite opinions, you can leave comments saying that you disagree with the suggestion. A manual review will be sent eventually, before the PR is merged. Additionally, the suggestions in the above review seem reasonable. |
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is |
| } on StateError { | ||
| // Ignore StateError, which may occur if the stream is already closed. |
There was a problem hiding this comment.
I'm still not confident about shipping this implementation, given that we didn't find the root cause here. Could you share a reproducible case in production?
| } catch (_) { | ||
| rethrow; | ||
| } |
| # Local history | ||
| .history No newline at end of file |
There was a problem hiding this comment.
It would be better if you could submit a separate PR for this with more context.
|
Superseded by #2571. The replacement is rebuilt on the current main branch, preserves credit for the original report and reproduction, addresses the unresolved review feedback, adds an exact transport-error regression test, and fixes the upload-cancellation hang found during adversarial review. Closing this stale, conflicting PR in favor of the replacement. |
Supersedes #2469. The original report and reproduction are credited to @vanelizarov. This replacement is rebuilt on the current `main` branch and addresses the unresolved review feedback and the cancellation hang found while auditing the earlier implementation. ### Motivation When an HTTP/2 peer closes a connection before a streamed request body finishes, `http2` closes its outgoing stream controller. The request body subscription can then deliver another chunk to the closed sink, producing an unhandled `StateError: Bad state: Cannot add event after closing` instead of the transport error that caused the closure. ### Changes - Stop the request body subscription when the outgoing HTTP/2 sink has already closed. - Explicitly settle request-body waiting when cancellation stops the subscription, rather than relying on `onDone`, which is not called after cancellation. - Keep cancellation callbacks weakly referenced so shared, never-cancelled tokens do not retain completed request resources. - Assert the exact `TransportConnectionException` for a peer connection closure and cover cancellation while the source stream remains open. - Update the `dio_http2_adapter` changelog. ### Verification With only the new tests applied to `main`, the connection-closure test fails with the reported `StateError`, and the cancellation test times out after two seconds. Both pass with this change, and the cancellation test also confirms that the source stream listener is removed. `dart test` for `plugins/http2_adapter`, `melos run format`, and `melos run analyze` complete successfully. An independent adversarial pass also exercised 20 concurrent cancellations, send timeout, a synchronous stream controller, and request-source error propagation. ### Hosted verification The min, stable, and beta workflows all pass with the configured httpbun and proxy services. The stable workflow also passes formatting, analysis, publish dry-run, VM/Chrome/Firefox/Flutter tests, the example APK build, and coverage reporting. Changed-file coverage for `http2_adapter.dart` increases from 75.69% to 82.42%. ### AI assistance Implementation, tests, and local review were performed with Codex. A separate Codex sub-agent performed the adversarial review. The original diagnosis and reproduction came from @vanelizarov in #2469. ### New Pull Request Checklist - [x] I have read the [Documentation](https://pub.dev/documentation/dio/latest/) - [x] I have read the [Agent Contribution Guidelines](https://github.com/cfug/dio/blob/main/AGENTS.md) (required if any part of the change was produced with AI assistance) - [ ] I have searched for a similar pull request in the [project](https://github.com/cfug/dio/pulls) and found none *(not applicable - this supersedes #2469)* - [x] I have updated this branch with the latest `main` branch to avoid conflicts (via merge from master or rebase) - [x] I have added the required tests to prove the fix/feature I'm adding - [ ] I have updated the documentation (if necessary) *(not applicable - no public API or documented behavior changes)* - [x] I have run the tests without failures - [x] I have updated the `CHANGELOG.md` in the corresponding package ### Additional context and info (if any) The replacement preserves the final outgoing-sink close as a normal `await`; the observed `StateError` originates from adding the next body chunk after the transport has closed the sink, not from closing the sink again. Co-authored-by: Codex <noreply@openai.com>
Periodically I receive unhandled
StateError: Bad state: Cannot add event after closingin Crashlytics:Stack Trace
This problem was mentioned earlier by me in this PR.
After investigation, it appears that the error was caused by a race condition, for example, when the server closed the connection before receiving the request body.
This PR introduces a more reliable way to handle such situations. It also includes a test that can be run with the old implementation to reproduce the described issue.
New Pull Request Checklist
mainbranch to avoid conflicts (via merge from master or rebase)CHANGELOG.mdin the corresponding package