fix(safe-operations): share one fallback AbortSignal across a withBoundedParallel batch (#221) - #403
Merged
Merged
Conversation
…ndedParallel batch withBoundedParallel built the signal handed to each handler as `options?.signal ?? new AbortController().signal` *inside* the worker loop. With no outer signal, that allocated a fresh AbortController per item and discarded it immediately — nothing held a reference to call .abort(), so signal.aborted was permanently false and per-item cancellation silently never worked (#221). Compute the fallback signal once per call, before the loop. Every item in an outer-signal-less call now receives the same AbortSignal instance. It's still not externally abortable without an API change (out of scope here), but it's a coherent shared object rather than a dead one built per item. The one real call site (components/stream/BulkWithdrawButton.tsx) always passes its own options.signal and ignores the handler's third parameter, so this is fully backward compatible. Closes #221
17 tasks
Contributor
Author
|
CI here fails at the
Both need a |
Contributor
Author
|
Root cause filed as #404. |
Contributor
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.
Rewrite of #382 on a main-repo branch (so CI runs — the fork PR had no
checks). Same fix, same test.
Problem
withBoundedParallel(lib/safe-operations.ts) built theAbortSignalhanded to each handler as
options?.signal ?? new AbortController().signalinside the worker loop. When the caller passes no outer signal, this
allocated a fresh
AbortControllerfor every item and threw it awayimmediately — nothing kept a reference to call
.abort(), sosignal.abortedstayedfalseforever and per-item cancellation silentlynever worked (#221).
Fix
Compute the fallback signal once per call, before the loop starts. Every
item in an outer-signal-less call now gets the same
AbortSignalinstance.It is still not externally abortable without an API change (returning the
controller / an
onAborthook) — out of scope for this bug — but it is acoherent shared object rather than a dead one manufactured per item.
The only real call site,
components/stream/BulkWithdrawButton.tsx, alwayspasses its own
options.signaland never reads the handler's thirdparameter, so this is fully backward compatible; the handler type
(
signal: AbortSignal, neverundefined) is unchanged.Test
lib/safe-operations.test.ts— new'shares a single fallback signal across items when no outer signal is provided': three items, asserts all threereceive the exact same
AbortSignalinstance (the old code gave each adistinct dead one). Existing signal tests unchanged.
Local
npm installis not possible in this environment (the lockfileregistry is unreachable here), so verification is via CI on this branch.
Closes #221