fix(dio): guard dispatch-stage handler completion, add request context to duplicate-completion errors - #2611
ryanaidilp wants to merge 1 commit into
Conversation
efbdadb to
c283db5
Compare
…text to duplicate-completion errors Fixes cfug#2613 - Thread RequestOptions through _BaseHandler so the "handler already called" StateError identifies which request triggered it, instead of being a bare, untraceable message. - Guard the post-_dispatchRequest handler.resolve/reject calls with isCompleted checks, since a racing cancellation can already have completed the handler.
c283db5 to
51c6771
Compare
CaiJingLong
left a comment
CaiJingLong
left a comment
There was a problem hiding this comment.
The added request context needs to follow the RequestOptions actually passed to each interceptor. The inline finding is reproducible on this head and currently identifies the wrong method/path after an earlier interceptor replaces the options.
Local verification at 51c6771 in an isolated worktree, using Dart 3.13.5: the existing interceptor, queued-interceptor, and cancel-token suites pass (61 tests), and analysis of lib/test is clean. An additional regression that replaces GET /original with POST /replacement fails because the error still reports GET /original. Using the callback's incoming RequestOptions makes that regression pass; the experimental source change was restored afterward.
Please also add a deterministic regression for the dispatch-stage cancellation change, or narrow the corresponding fix claim: the new test currently exercises duplicate completion diagnostics, while listenCancelForAsyncTask races futures without completing this handler itself. The reported cancellation fix needs its own failing-before/passing-after evidence.
The hosted package-verification and CodeQL runs remain action_required. Review performed with OpenAI Codex assistance.
| requestOptions.cancelToken, | ||
| Future(() async { | ||
| final handler = RequestInterceptorHandler(); | ||
| final handler = RequestInterceptorHandler(requestOptions); |
There was a problem hiding this comment.
[P2] Use the RequestOptions passed to this callback for the diagnostic context
requestOptions is the outer variable and is only replaced when dispatch starts. If an earlier request interceptor calls handler.next(options.copyWith(path: '/replacement', method: 'POST')), the next interceptor receives those new options, but its handler still captures the original GET /original. If it completes twice, the new error message points to the wrong request. I reproduced this with two request interceptors on the current head. Please construct the handler from state.data as RequestOptions (the same object passed to cb) and add a regression covering replacement of the options between interceptors.
2 participants
Summary
_BaseHandler's duplicate-completionStateError("Thehandlerhas already been called...") now includes the request method + URL, so it's traceable when it surfaces detached from the caller (e.g. viaZone.handleUncaughtErrorfor an async interceptor that double-completes after anawait)._dispatchRequesthandler.resolve/handler.rejectcalls inDioMixin.fetchwithisCompletedchecks, matching the pattern already used in_observeInterceptorCallback. A racing cancellation can otherwise complete the same handler concurrently.RequestOptions?constructor parameter on_BaseHandler/RequestInterceptorHandler/ResponseInterceptorHandler/ErrorInterceptorHandleris optional and positional, defaulting tonull.Test plan
dart test test/interceptor_test.dart test/queued_interceptor_test.dart test/cancel_token_test.dart— all pass, including a new regression test asserting the crash message identifies the offending request.dart analyze— no issues.dart format --set-exit-if-changed— clean.... (GET /resource)instead of a bare string.