Fix memory leaks on AbortSignal usage and improve memory consumption #596 #598
Conversation
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-tools/executor-http |
1.2.6-alpha-47e6ab8f4fd75cb2df3144402526b7684fb34fde |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/federation |
3.1.1-alpha-47e6ab8f4fd75cb2df3144402526b7684fb34fde |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/fusion-runtime |
0.10.32-alpha-47e6ab8f4fd75cb2df3144402526b7684fb34fde |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.9.4-alpha-47e6ab8f4fd75cb2df3144402526b7684fb34fde |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
1.3.40-alpha-47e6ab8f4fd75cb2df3144402526b7684fb34fde |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
1.3.28-alpha-47e6ab8f4fd75cb2df3144402526b7684fb34fde |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
1.4.12-alpha-47e6ab8f4fd75cb2df3144402526b7684fb34fde |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-common |
0.7.28-alpha-47e6ab8f4fd75cb2df3144402526b7684fb34fde |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http |
0.6.32-alpha-47e6ab8f4fd75cb2df3144402526b7684fb34fde |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http-callback |
0.5.19-alpha-47e6ab8f4fd75cb2df3144402526b7684fb34fde |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-ws |
1.0.2-alpha-47e6ab8f4fd75cb2df3144402526b7684fb34fde |
npm ↗︎ unpkg ↗︎ |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes remove the dependency on the custom abort signal package ( Changes
Sequence Diagram(s)sequenceDiagram
participant Executor as HTTP Executor
participant AbortManager as AbortSignal.any
participant Upstream as Upstream Service
Executor->>AbortManager: Combine multiple abort signals
AbortManager-->>Executor: Return combined signal
Executor->>Upstream: Execute fetch with combined signal
Upstream-->>Executor: Respond or trigger abort
sequenceDiagram
participant Gateway as Gateway Runtime
participant TimeoutPlugin as Timeout Handler
participant AbortManager as AbortSignal.any
participant Upstream as Subgraph Service
Gateway->>TimeoutPlugin: Initiate request with timeout options
TimeoutPlugin->>AbortManager: Aggregate execution and timeout signals
AbortManager-->>TimeoutPlugin: Return combined abort signal
TimeoutPlugin->>Upstream: Send request with combined signal
Upstream-->>TimeoutPlugin: Return response or signal timeout
Assessment against linked issues
Possibly related PRs
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/transports/common/src/index.ts (1)
6-8: Add JSDoc documentation for the new function.The implementation looks good, but adding documentation would help explain the purpose of this wrapper function and any specific requirements or behaviors.
+/** + * Combines multiple AbortSignal instances into a single signal that is aborted when any of the input signals are aborted. + * @param signals - Array of AbortSignal instances to combine + * @returns A new AbortSignal that is aborted when any of the input signals are aborted + */ export function abortSignalAny(signals: AbortSignal[]): AbortSignal { return AbortSignal.any(signals); }packages/runtime/tests/upstream-timeout.test.ts (1)
75-75: Consider moving the promise resolution.Moving the promise resolution before the assertions would make the test flow clearer and prevent any potential memory leaks from the unresolved promise.
- greetingsDeferred.resolve('Hello, World!'); + // Resolve the promise before assertions to prevent memory leaks + greetingsDeferred.resolve('Hello, World!'); + expect(resJson).toEqual({ + data: { + hello: null, + }, + errors: [ + expect.objectContaining({ + message: expect.stringMatching( + /(The operation was aborted due to timeout|The operation timed out.)/, + ), + path: ['hello'], + }), + ], + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
.changeset/@graphql-hive_gateway-runtime-598-dependencies.md(1 hunks).changeset/@graphql-mesh_transport-common-598-dependencies.md(1 hunks).changeset/@graphql-tools_executor-http-598-dependencies.md(1 hunks).changeset/fair-queens-sniff.md(1 hunks)packages/abort-signal-any/CHANGELOG.md(0 hunks)packages/abort-signal-any/package.json(0 hunks)packages/abort-signal-any/src/index.ts(0 hunks)packages/executors/http/package.json(0 hunks)packages/executors/http/src/index.ts(4 hunks)packages/runtime/package.json(0 hunks)packages/runtime/src/plugins/useUpstreamCancel.ts(2 hunks)packages/runtime/src/plugins/useUpstreamTimeout.ts(4 hunks)packages/runtime/tests/upstream-timeout.test.ts(2 hunks)packages/transports/common/package.json(0 hunks)packages/transports/common/src/index.ts(1 hunks)packages/transports/http-callback/src/index.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (6)
- packages/transports/common/package.json
- packages/runtime/package.json
- packages/abort-signal-any/package.json
- packages/executors/http/package.json
- packages/abort-signal-any/CHANGELOG.md
- packages/abort-signal-any/src/index.ts
✅ Files skipped from review due to trivial changes (4)
- .changeset/fair-queens-sniff.md
- .changeset/@graphql-mesh_transport-common-598-dependencies.md
- .changeset/@graphql-tools_executor-http-598-dependencies.md
- .changeset/@graphql-hive_gateway-runtime-598-dependencies.md
🧰 Additional context used
📓 Path-based instructions (3)
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/transports/http-callback/src/index.tspackages/transports/common/src/index.tspackages/executors/http/src/index.tspackages/runtime/src/plugins/useUpstreamCancel.tspackages/runtime/src/plugins/useUpstreamTimeout.tspackages/runtime/tests/upstream-timeout.test.ts
`packages/transports/**`: The transports configures how to d...
packages/transports/**: The transports configures how to deal with HTTP requests that are made from the GraphQL gateway to the GraphQL subgraphs. Transports manipulates the HTTP calls, headers, protocols and others.
packages/transports/http-callback/src/index.tspackages/transports/common/src/index.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/transports/http-callback/src/index.tspackages/transports/common/src/index.tspackages/executors/http/src/index.tspackages/runtime/src/plugins/useUpstreamCancel.tspackages/runtime/src/plugins/useUpstreamTimeout.tstsconfig.jsonpackages/runtime/tests/upstream-timeout.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (41)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert type-merging-batching
- GitHub Check: Examples / Convert subscriptions-with-transforms
- GitHub Check: Examples / Convert programmatic-batching
- GitHub Check: Examples / Convert openapi-subscriptions
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert openapi-additional-resolvers
- GitHub Check: Examples / Convert json-schema-subscriptions
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert hmac-auth-https
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Examples / Convert apq-subgraphs
- GitHub Check: Examples / Convert file-upload
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Binary built on macos-13
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: Bun Docker image
- GitHub Check: Node Docker image
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
🔇 Additional comments (11)
packages/runtime/src/plugins/useUpstreamCancel.ts (2)
20-23: LGTM! Clean implementation of signal handling.The signal handling is correctly implemented using the native
AbortSignal.anyAPI. The code properly collects all relevant signals and combines them effectively.
33-36: LGTM! Consistent signal handling in onSubgraphExecute.The implementation mirrors the signal handling in onFetch, maintaining consistency throughout the codebase.
packages/runtime/tests/upstream-timeout.test.ts (1)
77-123: LGTM! Well-structured test for memory leak verification.The test case effectively verifies that no memory leaks occur when the upstream service responds within the timeout period. Good use of
createDisposableServerfor cleanup.packages/runtime/src/plugins/useUpstreamTimeout.ts (3)
58-64: LGTM! Improved timeout handling with deferred promise.The use of
createDeferredand proper event listener cleanup improves memory management.
104-107: LGTM! Thorough cleanup of resources.The cleanup in the finally block ensures all resources are properly released, preventing memory leaks.
139-147: LGTM! Consistent signal handling in onFetch.The signal handling matches the pattern used in other parts of the code, maintaining consistency.
packages/transports/http-callback/src/index.ts (1)
160-160: LGTM! Good use of native AbortSignal.any.The change to use native
AbortSignal.anyinstead of a custom implementation is a good improvement that:
- Reduces dependencies
- Uses standardized browser APIs
- Maintains the same functionality
packages/executors/http/src/index.ts (3)
165-166: LGTM! Simplified abort check.The change to use
disposeCtrl.signaldirectly improves code clarity while maintaining the same functionality.
213-226: LGTM! Improved signal handling.The changes effectively:
- Collect all signals in an array
- Use native
AbortSignal.anyfor combining signals- Remove dependency on custom implementation
472-473: LGTM! Consistent abort handling in retry logic.The change maintains consistency with other abort checks by using
disposeCtrl.signaldirectly.tsconfig.json (1)
61-61: LGTM! Clean path mapping removal.The removal of the
@graphql-hive/gateway-abort-signal-anypath mapping aligns with the broader changes to use native abort signal APIs.
Solution of the reproduced #596
Potential fix for #303
Blocked by ardatan/whatwg-node#2031Alternative to #593 by removing the ponyfill completely
Closes #596
Closes #303
Closes #593