fix(http2_adapter): handle closed streams during uploads#2571
Merged
Conversation
Stop request body subscriptions when the peer closes the HTTP/2 connection, while preserving cancellation deallocation behavior. Co-authored-by: Ivan Elizarov <elizarov.vanya@gmail.com> Co-Authored-By: Codex <noreply@openai.com>
7 tasks
Contributor
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is |
CaiJingLong
approved these changes
Jul 21, 2026
CaiJingLong
left a comment
Contributor
There was a problem hiding this comment.
Review conclusion: Approve
Summary of changes
Stops the request-body subscription when the HTTP/2 outgoing sink has already closed (peer connection closure during upload), and explicitly settles the adapter future on cancellation instead of relying on onDone (which is not called after cancel()). Cancellation callbacks now hold only WeakReferences to the subscription/completer/stream so a shared, never-completed cancel token cannot retain completed request resources.
Review details
| Dimension | Conclusion | Notes |
|---|---|---|
| Correctness | Pass | The StateError from stream.outgoingMessages.add(...) on a closed sink is now caught and the subscription is stopped via stopRequestStream(), letting the response listener's onError surface the real TransportConnectionException. Cancellation now completes requestCompleter explicitly, fixing the hang caused by onDone not firing after cancel(). Idempotency holds: stopRequestStream, the cancelFuture.whenComplete, and the sendTimeout path all guard on requestCompleter.isCompleted and use .ignore() on repeated close/cancel. |
| Tests | Pass | Two new tests under group('request stream'). Verified locally: with only the tests applied to main, the closure test fails with StateError: Bad state: Cannot add event after closing and the cancellation test times out at 2s; both pass with this change. The cancellation test also asserts requestController.hasListener is false, confirming the subscription is removed. |
| Style | Pass | Matches existing patterns (WeakReference already used for streamWR, .ignore() idiom consistent with the rest of the file). No debug residue, no commented-out code. dart analyze clean, dart format clean. |
| Risk | Pass | Internal implementation change only; no public API or behavior change for the non-error path. Sensitive area (transport/cancellation) is touched, but the change narrows failure modes rather than broadening them. SDK lower bound is >=3.0.0; WeakReference has been available since Dart 2.17. Regression risk is low: the non-cancellation, non-closure path is behaviorally identical (listen → add → onDone completes → close). |
| Documentation | Pass | plugins/http2_adapter/CHANGELOG.md updated under ## Unreleased with a user-facing bullet. No public API or documented behavior changes, so no README/doc-comment updates needed (correctly marked N/A in the PR checklist). |
| Repo constraints | Pass | Branch fix/2469-http2-closed-stream follows category/ticket-id-description. Commit uses Conventional Commits (fix(http2_adapter): ..., lowercase imperative subject) with Co-Authored-By trailers for both human and AI contributors. PR description discloses AI assistance and states verification honestly in prose. Supersedes #2469 per the description. |
Confirmed key points
- Correctness: the reported
StateErroris eliminated and the underlying transport error propagates; the cancellation hang is fixed by explicitly completingrequestCompleterin thecancelFuture.whenCompletecallback. - Tests: regression tests reproduce both bugs on
mainand pass with the fix;dart test test/http2_test.dartpasses (the only failures in the broader suite are environmental — proxy/httpbun/SSL-pinning services not available locally). - Risk: no public API change, no SDK bump, no dependency change; sensitive transport path is made more robust.
Suggestions (non-blocking)
- The three pre-existing environmental test failures (
request with payload via proxy,test_suite_test.dart,SSL pinning untrusted certificate tested and allowed #2) are unrelated to this PR and require external services; no action needed here.
This comment was generated by AI agent omp (model: glm-5-2).
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
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.
Supersedes #2469.
The original report and reproduction are credited to @vanelizarov. This replacement is rebuilt on the current
mainbranch 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,
http2closes its outgoing stream controller. The request body subscription can then deliver another chunk to the closed sink, producing an unhandledStateError: Bad state: Cannot add event after closinginstead of the transport error that caused the closure.Changes
onDone, which is not called after cancellation.TransportConnectionExceptionfor a peer connection closure and cover cancellation while the source stream remains open.dio_http2_adapterchangelog.Verification
With only the new tests applied to
main, the connection-closure test fails with the reportedStateError, 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 testforplugins/http2_adapter,melos run format, andmelos run analyzecomplete 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.dartincreases 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
mainbranch to avoid conflicts (via merge from master or rebase)CHANGELOG.mdin the corresponding packageAdditional context and info (if any)
The replacement preserves the final outgoing-sink close as a normal
await; the observedStateErrororiginates from adding the next body chunk after the transport has closed the sink, not from closing the sink again.