fix(dio): preserve shared Future errors across interceptors#2567
Conversation
Remove per-callback error zones while observing returned interceptor Futures in their invocation zone. Preserve handler-driven ordering, queued error stack traces, and completed-handler uncaught errors. Co-Authored-By: Codex <noreply@openai.com>
## Motivation `httpbun.com` is currently returning HTTP 502 responses, which makes the integration tests fail independently of dio changes. The failure is visible in the [PR #2567 stable job](https://github.com/cfug/dio/actions/runs/29637101718/job/88061247325?pr=2567). CI already starts `httpbun.local`, but HTTP/2 pinning and all browser tests still used the public service. > upstream sharat87/httpbun#35 ## Changes - Reuse the shared test-server URL in HTTP/2 pinning tests and generate their certificate fingerprint from the configured host. - Keep local HTTPS for VM and HTTP/2 tests. - Start a second local HTTP container for Chrome, Firefox, and Flutter tests, avoiding browser-specific temporary-CA trust configuration. - Keep `httpbun.com` as the default outside the CI-local setup. - Avoid assuming `onBadCertificate` and `validateCertificate` receive different certificates; that varies with the certificate chain and trust store. This touches TLS and certificate-pinning test infrastructure only; production TLS behavior is unchanged. Maintainers: please apply the extra scrutiny required for this sensitive area. ## Verification `melos run format` and `melos run analyze` pass. The local Chrome shared suite passes 54 tests with one existing skip against the HTTP container, including CORS, redirects, status codes, timeouts, and uploads. GitHub Actions passes on the minimum, stable, and beta SDK jobs, covering VM, Chrome, Firefox, and Flutter tests; the stable job also passes APK build and coverage generation. *Implementation, tests, and local review by Codex.* --------- Co-authored-by: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
Review conclusion: ApproveChange summaryRemoves the per-callback forked error zones introduced by #2499 (which broke cross-request Review details
Confirmed key points
Suggestions (non-blocking)
|
Co-Authored-By: Codex <noreply@openai.com>
Addressed in
Addressed in the PR description under API and Compatibility Boundaries. It now states that the wrapper-subclass checks are compatibility guards rather than a new user-visible behavior, and that Thank you for the detailed review. |
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is |
CaiJingLong
left a comment
There was a problem hiding this comment.
Review conclusion: Approve
Change summary
Removes the per-callback forked error zones introduced by #2499 (which broke cross-request Future error delivery for shared Completers in deduplication interceptors, #2565) and replaces them with a non-awaiting _observeInterceptorCallback listener. The listener preserves #2499's hang-prevention for async callbacks (returned Future errors reject/advance an incomplete handler) while restoring same-zone Future error delivery so shared-failure deduplication works again. Adds private _invokeRequest/_invokeResponse/_invokeError dynamic-dispatch entry points to capture the runtime Future of async on* overrides without changing the public void API, and rewires QueuedInterceptor._handleQueue and the wrapper mixin through the same observer.
Review details
| Dimension | Result | Notes |
|---|---|---|
| Correctness | Pass | The observer is registered synchronously in the callback's existing zone, never awaits the Future, and keeps handler completion as the only pipeline signal. Incomplete-handler errors route to reject/next; complete-handler late errors route to callbackZone.handleUncaughtError, preserving #2499's late-error visibility. QueuedInterceptor returns null at the top-level _handle* so observation happens exactly once inside _handleQueue; FIFO and queue-release-on-cancel are unchanged. The dispatch callback's return null + observation also adds a safety net for non-DioException escapes from _dispatchRequest. |
| Tests | Pass | 52 interceptor/queued tests pass locally (dart test test/interceptor_test.dart test/queued_interceptor_test.dart). New coverage includes: the deterministic #2565 reproduction (asserts 2 requests, 1 adapter fetch, 2 joined errors, 0 uncaught, 0 pending), async Interceptor subclass errors on all three paths, InterceptorsWrapper/QueuedInterceptorsWrapper subclass override dispatch + async subclass errors, async constructor callbacks, queued request/response/error failures with queue release for the following request, the #2167 non-awaiting guard, and late-error-after-handler-completion reaching the callback zone. dio_mixin_test, cancel_token_test, adapters_test, exception_test also pass. The 7 failures in the full VM suite (test_suite_test, stacktrace_test, options_timeout_integration_test) are environmental network failures (DNS/Socket/real-timeout), not related to this change. |
| Style | Pass | Matches existing patterns; no debug residue; dynamic dispatch is localized and commented. Conventional Commits with scope, lowercase subjects, Co-Authored-By: Codex trailer on every commit. Branch fix/2565-interceptor-error-zones follows §8.1. |
| Risk | Pass | No public signature/typedef change, no SDK lower-bound increase, no Dart 3-only feature. Behavior delta vs 5.10.0 is narrow: a Future started inside a callback that is neither returned nor handled is no longer caught by a Dio-forked zone and will not unblock an incomplete handler — documented as an explicit ownership boundary. This edge case was incidental to #2499 (which targeted the async callback's own returned Future, now covered by the observer). Sensitive-area note: this touches the interceptor pipeline ordering/error propagation; the change reverts the #2499 zone boundary while preserving its hang fix, and the #2167 microtask-ordering regression is explicitly guarded by a test. |
| Docs | Pass | CHANGELOG.md updated under ## Unreleased with one user-facing bullet. No public API doc changes needed (no API surface change). |
| Repo constraints | Pass | Motivation grounded in #2565; one PR/one concern; regression test included; AI attribution present in PR description and commit trailers; no drive-by dependency bumps; no public-API break. |
Confirmed key points
- Correctness: cross-request shared-
Futureerror delivery restored; #2499 async-throw hang prevention preserved via the observer;QueuedInterceptorqueue ordering and cancel-release unchanged; wrapper subclasson*overrides dispatch correctly (private_invoke*overrides removed from the mixin). - Tests: #2565 regression test asserts the exact issue invariants (single fetch, both requests terminate, no uncaught error, no pending handler); #2167 non-awaiting guard prevents reintroducing the #2139 microtask regression; all 52 interceptor/queued tests pass locally.
- Risk: no public API or SDK-constraint change; the only behavior delta (unowned detached
Futures no longer caught) is a documented ownership boundary, not a regression of #2499's targeted case.
Suggestions (non-blocking)
- The ownership-boundary rationale for unowned detached
Futures is described in the PR body but not in a code comment near_observeInterceptorCallback; a one-line pointer comment there would help future maintainers understand why only returnedFutures are observed. - Consider noting in the PR description that the second CHANGELOG bullet from an earlier commit was intentionally removed (commit
6b42fa2), so reviewers don't expect a "preserve wrapper overrides" changelog entry — the net behavior vs 5.10.0 for wrapper subclass overrides is unchanged.
Verification performed
dart analyze --fatal-infos lib/src/interceptor.dart lib/src/dio_mixin.dart: no issues.dart test test/interceptor_test.dart test/queued_interceptor_test.dart: all 52 tests passed.dart test test/dio_mixin_test.dart test/cancel_token_test.dart test/adapters_test.dart test/exception_test.dart: all passed (one skipped TLS test).- Full
dart testsuite: 241 passed, 6 skipped, 7 failed — all 7 failures are network-dependent environmental tests (DNS resolution, real socket connections, live timeouts) unrelated to this change.
Closes #2565
Problem
Dio 5.10.0 introduced a regression in the interceptor execution model. A custom interceptor that deduplicates concurrent requests by sharing a
Completercan expose an uncaughtDioException, leave the duplicate request pending, or dispatch the same request more than once when the adapter fails immediately.The failure is not caused by request deduplication itself. It is caused by the interaction between deduplication and the per-callback error zones introduced by #2499.
Root Cause
Interceptor callbacks intentionally have public
voidreturn types:Dart still allows an async implementation or callback to produce a runtime
Future<void>, but the public static type does not expose that Future. Before #2499, an async exception thrown beforenext,resolve, orrejectcould therefore leave the handler's completer pending forever.#2499 addressed that hang by invoking every callback inside a newly forked
Zonewith a customhandleUncaughtError. If the handler was incomplete, the zone converted the uncaught error into a handler error; if the handler was already complete, it forwarded the error to the parent zone. This preserved the publicvoidAPI and did not await callbacks.The new zone boundary changed Dart's Future error-delivery semantics. Dart Future errors do not cross between distinct error zones in the same way as ordinary values. The deterministic #2565 sequence is:
The regression is the per-callback error-zone isolation; the immediate adapter failure is the deterministic timing trigger that exposes it in this reproduction. This fix removes the callback-specific zones created by Dio; it does not make a shared Future safe when application code deliberately creates and listens to it across unrelated external error zones, which remain governed by Dart's normal error-zone rules. Before the implementation, a test-only probe reproduced two
onRequestcallbacks, one adapter fetch, zero joined-error deliveries, one uncaught error, and a timed-out pending request on the base implementation. The committed regression test asserts the repaired behavior after the fix.Implementation
1. Capture runtime callback Futures without changing the API
A private library typedef,
_InterceptorCallback<T, V>, returnsObject?.Interceptornow has private virtual_invokeRequest,_invokeResponse, and_invokeErrorentry points that dynamically invoke the publicon*method:This is deliberate:
Interceptorsubclass can overrideFuture<void> onRequest/onResponse/onError; dynamic invocation retains the runtime Future even though the base API isvoid.The wrapper compatibility path is kept explicit.
InterceptorsWrapperandQueuedInterceptorsWrapperstill expose their existing publicvoid on*methods. The wrapper mixin dynamically invokes the constructor-supplied callback and observes its runtime result there. It no longer uses private mixin overrides that bypass a subclass's publiconRequest,onResponse, oronErroroverride. This preserves downstream wrapper-subclass behavior while retaining support for async constructor callbacks.2. Observe, do not await, the returned Future
_observeInterceptorCallbackis registered synchronously immediately after callback invocation, in the callback's existing zone:The callback Future is never awaited and is never used as the pipeline completion signal. Handler completion remains the only normal signal that advances the request, response, or error chain.
When the observed Future fails:
handler.reject(assureDioException(error, ..., stackTrace), true)handler.reject(assureDioException(error, ..., stackTrace), true)handler.next(assureDioException(error, ..., stackTrace))The original
StackTraceis preserved in all conversions. The error listener consumes the original Future error and handles it through the interceptor handler, so the derived listener Future does not create a second uncaught error.If the callback has already completed its handler and then its returned Future fails, the error is deliberately sent to the zone captured when the listener was registered. This preserves #2499's late-error visibility for fire-and-forget behavior without allowing Dio to complete a handler a second time. A detached Future that the callback neither returns nor handles remains outside Dio's ownership and is not reclassified as a handler failure. If the callback also never completes its handler, Dio cannot use that detached Future to unblock the request, so the request can still remain pending. This is an explicit ownership boundary rather than an attempt to catch every asynchronous task started by user code.
3. Preserve normal and queued pipeline timing
For normal interceptors,
DioMixin.fetchinvokes the private_invoke*method inside the existing pipeline Future, observes the runtime result, and returns the handler Future. It no longer forks a callback-specific error zone.For queued interceptors,
QueuedInterceptor._handleQueuereceives the matching private_invoke*method. The callback is still called inside the existing synchronous try block, its returned Future is observed immediately, and queue advancement still occurs only through handler completion or cancellation. FIFO behavior and the separate request/response/error queues are unchanged.The effective timing is therefore:
This removes the cross-request error-zone boundary while preserving handler-driven ordering. Awaiting the callback Future was intentionally avoided because the await-based approach in #2139 changed microtask ordering and broke following interceptor behavior reported in #2167; #2169 reverted #2139.
API and Compatibility Boundaries
InterceptorSendCallback,InterceptorSuccessCallback, andInterceptorErrorCallbackremain publicvoid Function(...)typedefs.Interceptor,InterceptorsWrapper,QueuedInterceptor, andQueuedInterceptorsWrapperare all covered.on*methods instead of being bypassed by an internal closure path.The wrapper-subclass checks are compatibility guards, not a new user-visible behavior. A temporary separate CHANGELOG bullet for preserving those overrides was intentionally removed in
6b42fa2; the final changelog contains only the #2565 user-facing fix.Tests
The regression and compatibility coverage includes:
Interceptorsubclasses with asynconRequest,onResponse, andonErroroverrides that throw after an await.InterceptorsWrapperandQueuedInterceptorsWrappersubclasses overriding all three publicon*methods synchronously, with async wrapper-subclass coverage on the request path.The focused VM and Chrome suites each pass all 52 tests.
Verification
Locally,
melos run format,melos run analyze, anddart analyze --fatal-infospassed. CI run 29642622254 passed on min, beta, and stable SDK matrices, including VM, Chrome, Firefox, Flutter, APK build, and coverage.The final coverage report is:
dio/lib/src/dio_mixin.dartdio/lib/src/interceptor.dartA full local
melos run testwas attempted, but network-dependent VM and browser tests encountered external HTTP 502, CORS, and timeout failures. The CI workflow starts local httpbun containers on each GitHub Actions runner and completed successfully.Unverified
The minimum Dart 2.18 SDK was not installed locally; the remote min SDK workflow passed. Local Firefox and Flutter targets were unavailable or blocked by local configuration, but both passed in the remote CI matrices.
AI Attribution
Implementation, tests, commit, and local review by Codex.