fix(executor): do not use leaking registerAbortSignalListener, and handle listeners inside the execution context
#6977
Conversation
🦋 Changeset detectedLatest commit: 8efeb52 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes refactor the abort signal management in the GraphQL execution process and its associated tests. Outdated functions such as Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant E as Executor
participant AC as AbortSignal
participant EC as ExecutionContext
C->>E: Send GraphQL query with AbortSignal
E->>EC: Build Execution Context (with onSignalAbort & signalPromise)
AC->>EC: Trigger abort signal event
EC->>E: Propagate abort (reject signalPromise)
E->>C: Return abort error (DOMException)
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/executor/src/execution/__tests__/abort-signal.test.tsOops! Something went wrong! :( ESLint: 9.22.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/executor/src/execution/__tests__/stream-test.tsOops! Something went wrong! :( ESLint: 9.22.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by packages/executor/src/execution/execute.tsOops! Something went wrong! :( ESLint: 9.22.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-tools/executor |
1.4.5-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-apollo-link |
1.0.18-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-envelop |
3.0.26-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-legacy-ws |
1.1.16-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-urql-exchange |
1.0.18-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-yoga |
3.0.26-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/graphql-tag-pluck |
8.3.18-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
graphql-tools |
9.0.17-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/import |
7.0.17-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/links |
9.0.26-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/load |
8.0.18-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/apollo-engine-loader |
8.0.19-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/code-file-loader |
8.1.19-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/git-loader |
8.0.23-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/github-loader |
8.0.19-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/graphql-file-loader |
8.0.18-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/json-file-loader |
8.0.17-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/module-loader |
8.0.17-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/url-loader |
8.0.30-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/merge |
9.0.23-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/mock |
9.0.21-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/node-require |
7.0.19-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/relay-operation-optimizer |
7.0.18-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/resolvers-composition |
7.0.17-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/schema |
10.0.22-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/utils |
10.8.5-alpha-20250313112505-8efeb528d413c0973336fb16b420b5ff158d18f2 |
npm ↗︎ unpkg ↗︎ |
💻 Website PreviewThe latest changes are available as preview in: https://fd7aeab3.graphql-tools.pages.dev |
…handle listeners inside the execution context
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/executor/src/execution/__tests__/stream-test.ts (1)
586-586: Simple style improvement.This change condenses a multi-line object declaration into a single line, making the code more concise while maintaining readability for this simple object.
Consider applying this condensed style consistently to other similar objects in the file for better uniformity.
packages/utils/src/registerAbortSignalListener.ts (1)
36-39: Great optimization for already aborted signals.Moving the abort check to the beginning of the function and immediately returning a rejected promise is a significant optimization. This prevents unnecessary Promise creation when the signal is already aborted.
Consider also making the inner check at line 41 an
else ifsince it's now redundant with your early check.if (signal.aborted) { return fakeRejectPromise(signal.reason); } return new Promise<void>((_resolve, reject) => { - if (signal.aborted) { - reject(signal.reason); - return; - } registerAbortSignalListener(signal, () => { reject(signal.reason); }); });packages/executor/src/execution/__tests__/abort-signal.test.ts (1)
165-179: Simplify promise chaining in the test for better readability.Wrapping
normalizedExecutorwithPromise.resolve().then(...)is functionally correct but can be simplified using a direct call in theexpectblock:- await expect( - Promise.resolve().then(() => - normalizedExecutor({...}) - ), - ).rejects.toBeInstanceOf(DOMException); + await expect( + normalizedExecutor({...}) + ).rejects.toBeInstanceOf(DOMException);packages/executor/src/execution/execute.ts (1)
128-129: Document usage of newonSignalAbortandsignalPromisefields.Adding doc comments clarifies how callers should implement custom abort handlers and handle the resulting promise rejections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/swift-geese-behave.md(1 hunks)packages/executor/src/execution/__tests__/abort-signal.test.ts(3 hunks)packages/executor/src/execution/__tests__/stream-test.ts(1 hunks)packages/executor/src/execution/execute.ts(12 hunks)packages/executor/src/execution/promiseForObject.ts(3 hunks)packages/utils/src/registerAbortSignalListener.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: deployment
- GitHub Check: Unit Test on Node 18 (windows-latest) and GraphQL v16
🔇 Additional comments (18)
.changeset/swift-geese-behave.md (1)
1-8: LGTM: Changeset provides clear description of the PR's purpose.The changeset correctly documents the purpose of the PR: removing the leaking
registerAbortSignalListenerand handling listeners inside the execution context. This aligns with the PR objectives and provides adequate information for package updates.packages/utils/src/registerAbortSignalListener.ts (1)
1-1: Import added for handling already aborted signals.Adding the
fakeRejectPromiseimport is necessary for the optimized abort signal handling implementation.packages/executor/src/execution/promiseForObject.ts (4)
1-1: Simplified imports.The import statement has been streamlined to only include what's needed from the promise helpers package, removing dependency on
getAbortPromise.
14-18: Added signalPromise parameter for improved abort handling.The new
signalPromiseparameter provides a more flexible way to handle abort signals without relying on the potentially leakinggetAbortPromisefunction.
19-19: Early abort detection added.This change ensures that if the signal is already aborted, an exception is thrown immediately rather than proceeding with potentially unnecessary work.
37-39: Improved abort signal handling.Using
Promise.racewith the providedsignalPromiseis a cleaner approach than the previous implementation withgetAbortPromise. This change aligns with the PR's goal of fixing memory leaks related to abort signal handling.packages/executor/src/execution/__tests__/abort-signal.test.ts (2)
37-37: Use offinallycorrectly handles both success and rejection cases.This ensures that
stopped = trueis set regardless of whether the promise resolves or rejects, improving reliability.
153-153: Removal ofasyncis appropriate since no asynchronous tasks remain.The function no longer requires the overhead of an async signature.
packages/executor/src/execution/execute.ts (10)
424-425: Immediate signal check ensures early exit if already aborted.This is consistent with good abort handling practices.
495-495: Redundant safety check for the abort signal.This second explicit
throwIfAborted()call is safe and ensures the process won't proceed if the signal was triggered after variable coercion and validation steps.
497-517: Effective pattern for unifying abort listeners via a shared promise.This refactor prevents leaks by removing the need for separate calls to
registerAbortSignalListenerand unifying cleanup in one place. Great improvement!
1006-1008: Properly cleaning up iterator on abort.Attaching a listener to invoke
iterator.return()ensures resources are freed promptly on abort.
1790-1793: Chaining toassertEventStreamenforces correct event-stream validation.The error handling callback correctly re-throws a located error.
1796-1796: Refactored explicit call toassertEventStream.This succinctly checks both abort signal and the returned iterable before proceeding.
1802-1807: Graceful check ofsignalbefore validating the event stream.The logic handles pre-aborted signals and invalid values consistently.
1824-1824: Auto-callingiterator.return()on abort.Well done ensuring the subscription is promptly finalized when aborted.
2160-2161: RacingsignalPromiseensures immediate halt on signal abort.This prevents any further waiting for other promises, aligning with the updated abort handling pattern.
57-57:✅ Verification successful
Ensure
@whatwg-node/promise-helpersis properly declared as a dependency.The new import is fine. Confirm that it's included in your package manifest and locked at a stable version.
🏁 Script executed:
#!/bin/bash # Searching the package.json files for the "promise-helpers" dependency rg '"@whatwg-node/promise-helpers":' package.jsonLength of output: 51
I'll re-run the dependency search more broadly to ensure we check all package manifest files in the repository. The initial script only looked for a strict match in the root-level package.json, which might not cover cases where the dependency is declared in another package.json.
Try this extended search:
If this search still produces no output, please verify manually whether the dependency is declared in the appropriate package.json (e.g., in the "packages/executor" folder or the root manifest) so that it's properly locked at a stable version.
Action Required:
- Confirm that
@whatwg-node/promise-helpersis declared as a dependency in the relevant package manifest (either the root package.json or the specific package's manifest).- If it’s missing, please add it with a stable version lock.
🏁 Script executed:
#!/bin/bash # Recursively search all package.json files for the dependency string fd package.json -x grep -H "@whatwg-node/promise-helpers" {}Length of output: 506
Dependency Declaration Confirmed
The dependency
@whatwg-node/promise-helpersis declared in the package manifests, including in thepackages/executor/package.jsonfile with version"^1.0.0". No further action is needed.
…handle listeners inside the execution context (#6977) * fix(executor): do not use leaking `registerAbortSignalListener`, and handle listeners inside the execution context * lets go
No description provided.