fix(dio): prevent request hang when async interceptor throws#2499
fix(dio): prevent request hang when async interceptor throws#2499ersanKolay wants to merge 3 commits intocfug:mainfrom
Conversation
When an interceptor overrides onRequest/onResponse/onError with an async implementation that throws after an await, the handler's Completer is never completed and the request hangs indefinitely. Wrap interceptor callback invocations in a forked Zone with a custom handleUncaughtError that rejects/advances the handler when an uncaught async error occurs. Unlike the reverted PR cfug#2139, this approach does not await the callback and preserves the original microtask ordering, avoiding the regression from cfug#2167. Fixes cfug#2138
|
Sorry I accidently hit the close button. This looks cool! I shall try to play it locally somehow... |
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is |
There was a problem hiding this comment.
Pull request overview
This PR fixes a Dio request hang that occurs when an interceptor callback is implemented as async and throws after an await without completing its handler, by ensuring the handler is completed on uncaught async errors.
Changes:
- Run each interceptor callback inside a forked
Zonethat converts uncaught async errors intohandler.reject()/handler.next()to unblock the request. - Add new interceptor tests covering async-throw scenarios for
onRequest,onResponse, andonError. - Update the
CHANGELOG.mdwith the fix description.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
dio/lib/src/dio_mixin.dart |
Wraps interceptor callback execution in a forked Zone to catch uncaught async errors and complete the handler to prevent hangs. |
dio/test/interceptor_test.dart |
Adds regression tests ensuring async interceptor throws do not hang requests. |
dio/CHANGELOG.md |
Notes the fix in the Unreleased section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| handleUncaughtError: (self, parent, zone, error, stackTrace) { | ||
| if (!handler.isCompleted) { | ||
| onUncaughtError(error); | ||
| } |
There was a problem hiding this comment.
handleUncaughtError receives a stackTrace but it’s currently ignored, so the DioException created via assureDioException will use requestOptions.sourceStackTrace/StackTrace.current instead of the interceptor’s actual throw site. Please thread the provided stackTrace into the error you pass to the handler (e.g., by constructing a DioException with stackTrace: stackTrace, or by extending assureDioException to accept a stack trace) so users get actionable traces.
| specification: ZoneSpecification( | ||
| handleUncaughtError: (self, parent, zone, error, stackTrace) { | ||
| if (!handler.isCompleted) { | ||
| onUncaughtError(error); |
There was a problem hiding this comment.
The custom handleUncaughtError swallows async errors when handler.isCompleted is already true (it neither forwards to parent.handleUncaughtError nor rethrows). That can hide genuinely unhandled errors from interceptor code that happen after calling the handler (e.g., fire-and-forget futures). Consider delegating to parent.handleUncaughtError(...) when the handler is already completed so those errors are still reported, while only converting to handler.reject/next when it isn’t completed yet.
| onUncaughtError(error); | |
| onUncaughtError(error); | |
| } else { | |
| parent.handleUncaughtError(zone, error, stackTrace); |
| onRequest: (options, handler) async { | ||
| await Future<void>.delayed(const Duration(milliseconds: 10)); | ||
| throw UnsupportedError(errorMsg); | ||
| }, |
There was a problem hiding this comment.
The Future.delayed(const Duration(milliseconds: 10)) is only used to force an async gap; using Duration.zero (or Future<void>.value()/Future<void>.microtask) would keep the test intent while avoiding unnecessary wall-clock delay in the suite.
| onResponse: (response, handler) async { | ||
| await Future<void>.delayed(const Duration(milliseconds: 10)); | ||
| throw UnsupportedError(errorMsg); | ||
| }, |
There was a problem hiding this comment.
The Future.delayed(const Duration(milliseconds: 10)) is only used to force an async gap; using Duration.zero (or Future<void>.value()/Future<void>.microtask) would keep the test intent while avoiding unnecessary wall-clock delay in the suite.
| onError: (err, handler) async { | ||
| await Future<void>.delayed(const Duration(milliseconds: 10)); | ||
| throw UnsupportedError(errorMsg); | ||
| }, |
There was a problem hiding this comment.
The Future.delayed(const Duration(milliseconds: 10)) is only used to force an async gap; using Duration.zero (or Future<void>.value()/Future<void>.microtask) would keep the test intent while avoiding unnecessary wall-clock delay in the suite.
Summary
Fixes #2138.
When an interceptor overrides
onRequest/onResponse/onErrorwith anasyncimplementation and throws after anawaitwithout calling the handler, the request hangs indefinitely. The handler'sCompleteris never completed because:void, so the returnedFutureis discardedhandler.next()/reject()/resolve()Approach
Each interceptor callback is now invoked inside a forked
Zonewith a customhandleUncaughtError. When an uncaught async error occurs and the handler hasn't been completed yet, the zone handler callshandler.reject()(orhandler.next()for error interceptors) to unblock the request.This approach differs from the reverted PR #2139 which changed the callback typedefs to
FutureOr<void>and awaited the return value. That changed the microtask ordering and broke multi-interceptor chains (#2167).Zone.fork()does not await the callback -- it only catches async errors that would otherwise be lost, preserving the original execution order.Why this works
Test plan
dart formatanddart analyzeclean