Skip to content

🐛 Fix QueuedInterceptor queue stalling when the active request is cancelled#2526

Open
ultramcu wants to merge 1 commit into
cfug:mainfrom
ultramcu:fix/queued-interceptor-cancel-stall
Open

🐛 Fix QueuedInterceptor queue stalling when the active request is cancelled#2526
ultramcu wants to merge 1 commit into
cfug:mainfrom
ultramcu:fix/queued-interceptor-cancel-stall

Conversation

@ultramcu
Copy link
Copy Markdown

Summary

When a request is cancelled (via CancelToken) while it is the active task in a QueuedInterceptor's internal queue — i.e. its onRequest/onResponse/onError callback is still running an async step and has not yet called handler.next/resolve/reject — the queue stalls permanently. Every subsequent request routed through the same QueuedInterceptor, including requests with no cancel token, then hangs forever.

This hits a very common, real-world setup: a QueuedInterceptor that refreshes an auth token in an async onRequest (the canonical reason to use a queued interceptor), where the user cancels the in-flight request.

Root cause

QueuedInterceptor._handleQueue only advances the queue from inside the handler's next/resolve/reject (via _processNextInQueue). Cancellation completes the request future (through listenCancelForAsyncTask) without ever completing the queued handler, so the active task is never released, processing stays true, and no further task is dequeued.

Fix

Each task now arms a one-shot cancel hook — registered only while it is the active task, so a queued task is never advanced out of turn — that releases the queue slot if the request is cancelled before the handler completes. Queue advancement is made idempotent (an advanced guard) so a normal completion and a cancellation can't double-advance. No public API change; behaviour for non-cancelled requests is unchanged.

Tests

Added two regression tests in queued_interceptor_test.dart (request queue + response queue): they cancel the active task mid-callback and assert that an independent, un-cancelled request still completes promptly. Both fail (time out / "queue stalled") on main and pass with this change.

  • Full dart test suite: 221 passed, 6 network-gated skips, 0 failures
  • dart format and dart analyze: clean
  • No public API change; CHANGELOG.md updated.

@ultramcu ultramcu requested a review from a team as a code owner May 25, 2026 10:08
@AlexV525
Copy link
Copy Markdown
Member

Thanks for the PR! Requesting our agent to review...

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a hang in QueuedInterceptor where cancelling the currently active task (whose async onRequest/onResponse/onError callback had not yet called handler.next/resolve/reject) would never release the queue slot, stalling all subsequent requests routed through the same interceptor. The fix adds an idempotent queue-advance triggered both by handler completion and by cancellation of the active task.

Changes:

  • Refactor _handleQueue to run tasks via a runTask closure with an idempotent advance() used both by _processNextInQueue and a per-task cancel hook registered only while the task is active.
  • Add a _cancelTokenOf helper that extracts the CancelToken from request/response/error payloads.
  • Add two regression tests covering the request and response queue stall scenarios, plus a CHANGELOG.md entry.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
dio/lib/src/interceptor.dart Restructures _handleQueue to advance the queue idempotently on either handler completion or cancellation of the active task.
dio/test/queued_interceptor_test.dart Adds request-queue and response-queue regression tests for the cancellation-induced stall.
dio/CHANGELOG.md Adds an Unreleased entry describing the fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread dio/lib/src/interceptor.dart
Comment thread dio/test/queued_interceptor_test.dart Outdated
When a request is cancelled (via CancelToken) while it is the active task
in a QueuedInterceptor's queue — its onRequest/onResponse/onError callback
still running an async step and not yet having called next/resolve/reject —
the queue stalled forever. The queue only advances from inside the handler
(via _processNextInQueue), but cancellation completes the request future
without completing the queued handler, so the active slot was never released
and every subsequent request through the interceptor hung, including ones
with no cancel token.

Each task now arms a one-shot cancel hook (only while it is the active task)
that releases the queue slot if the request is cancelled before the handler
completes; queue advancement is made idempotent so a normal completion and a
cancellation cannot double-advance. No public API change.

Adds request-queue and response-queue regression tests.
@ultramcu ultramcu force-pushed the fix/queued-interceptor-cancel-stall branch from ed004d6 to 1cb445f Compare May 28, 2026 14:43
@ultramcu
Copy link
Copy Markdown
Author

Thanks for the review @AlexV525 + Copilot — both findings were valid and have been addressed in 1cb445f (amended onto the existing commit so the PR stays one commit).

1. Memory leak (interceptor.dart) — fixed

The whenCancel.then(...) listener can't be detached from the CancelToken's internal completer, so the closure was indeed retaining the per-task payload (RequestOptions/Response/DioException + handler) until the token was cancelled or GC'd — exactly as Copilot described, and impactful for the long-lived shared-token usage pattern CancelToken documents.

Fix: a clearable nullable cell _InterceptorParams<T, V>? taskRef = task; is captured by the cancel closure instead of task directly. advance() nulls the cell, so once the queue moves on, the closure pins only null (Dart closures capture variables, not initial values).

Verified empirically with a Finalizer-instrumented harness (5 tasks × 16 MB payload each, with a long-lived whenCancel-like Future holding the listener):

  • Before fix: 0/5 payloads collected
  • After fix: 5/5 collected

The cancel-before-completion path still triggers advance() correctly; advanced + taskRef == null together guarantee single-fire.

2. expect(futureA, throwsA(...)) without await (queued_interceptor_test.dart) — fixed

Confirmed: reverting one test to the previous form reproduced the unhandled-async-error leak ([E] marker on a passing test).

Both tests now use the "capture and await separately" form Copilot suggested:

final aRejected = expectLater(
  futureA,
  throwsA(isA<DioException>()
      .having((e) => e.type, 'type', DioExceptionType.cancel)),
);
// ... cancellation + futureB assertions ...
await aRejected;

I went with the up-front registration (rather than await expectLater(...) after tokenA.cancel(...)) so the error listener provably attaches before the cancel propagates — no microtask-timing window.

CI

  • dart analyze — clean on changed files
  • dart test test/queued_interceptor_test.dart — 6/6 pass
  • dart test — 221 pass, 6 skipped, 1 flake (test_suite_test.dart: send progress → HTTP 502 from httpbun.com; passes in isolation, unrelated to this PR)

Ready for another look whenever you have time. 🙏

@github-actions
Copy link
Copy Markdown
Contributor

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
dio/lib/src/interceptor.dart 🟢 99.3% 🟢 99.34% 🟢 0.04%
Overall Coverage 🟢 88.88% 🟢 88.93% 🟢 0.05%

Minimum allowed coverage is 0%, this run produced 88.93%

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants