Simplify and stabilise AbortSignal.any ponyfill #593
Conversation
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-hive/gateway-abort-signal-any |
1.0.0-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/executor-http |
1.2.6-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
@graphql-tools/federation |
3.1.1-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/fusion-runtime |
0.10.32-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.9.4-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
1.3.40-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
1.3.28-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
1.4.12-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-common |
0.7.28-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http |
0.6.32-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-http-callback |
0.5.19-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/transport-ws |
1.0.2-alpha-35ae4466252ee2216728847c3c982221daa617af |
npm ↗︎ unpkg ↗︎ |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request updates the handling of abort signals by introducing a new dependency ( Changes
Assessment against linked issues
Possibly related PRs
Tip 🌐 Web search-backed reviews and chat
✨ 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 (
|
![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 (2)
packages/abort-signal-any/src/index.ts (1)
17-22: Consider adding a guard to avoid multiple abort calls.If multiple signals abort in quick succession, the
abortfunction may be triggered more than once. Adding a check—for instance, ifctrl.signal.aborted—could prevent redundant calls toctrl.abort()..changeset/little-plums-perform.md (1)
4-8: Refine Explanation and Improve Readability
The note now indicates thatabortSignalAnyreturns the nativeAbortSignalinstead of a customAbortSignalFromAny. The intent is clear; however, the phrasing in the sentence could be improved for clarity and grammatical correctness. Consider rephrasing as follows:-Instead of the custom `AbortSignalFromAny`. There is no need for the complication of adding signals to existing sets, chaining signals enough as well as simpler. +Instead of using a custom `AbortSignalFromAny`, the function now returns the native `AbortSignal` directly. This change simplifies signal management by eliminating the need to add signals to existing sets and chain them unnecessarily.This revision clarifies the benefits and removes ambiguity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/famous-mangos-rest.md(1 hunks).changeset/little-plums-perform.md(1 hunks).changeset/long-beds-dream.md(1 hunks).changeset/real-seals-taste.md(1 hunks)packages/abort-signal-any/src/index.ts(1 hunks)packages/runtime/src/plugins/useUpstreamCancel.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/long-beds-dream.md
🧰 Additional context used
📓 Path-based instructions (2)
packages/runtime/src/plugins/useUpstreamCancel.ts (2)
Pattern 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 changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern **: 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 console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/abort-signal-any/src/index.ts (2)
Pattern 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 changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern **: 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 console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
⏰ Context from checks skipped due to timeout of 90000ms (24)
- GitHub Check: Unit / Bun
- GitHub Check: Unit / Node v20
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: Unit / Node v18
- 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: Benchmark / bun / 1000 items
- GitHub Check: Leaks / Node v22
- GitHub Check: Snapshot / snapshot
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Build
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Bundle
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
packages/abort-signal-any/src/index.ts (1)
1-2: Handle empty iterable scenarios to avoid unintended never-aborting signals.If the iterable is empty, this returns a never-aborting signal. Verify whether this behavior is desired, or consider short-circuiting to avoid unexpected hangs in downstream code.
packages/runtime/src/plugins/useUpstreamCancel.ts (2)
21-24: Confirm the desired behavior with an empty signals array.If no signals exist,
abortSignalAny([])returns a never-aborting signal. Please verify this is the intended logic when no signals are present.
34-37: Maintain consistency for empty signals.Similarly, if there are no signals here,
abortSignalAny([])yields a never-aborting signal. Ensure this aligns with your intended upstream cancellation flow..changeset/real-seals-taste.md (1)
2-2: Patch version aligns with the bug-fix guidelines.Using a patch increment for a memory-leak fix follows the prescribed semver for bug fixes. Good job maintaining consistent versioning.
.changeset/famous-mangos-rest.md (2)
1-3: Dependency Version Update Noted
The changeset correctly indicates that the dependency@graphql-hive/gateway-abort-signal-anyis now bumped to a major version. This level of specificity is useful to inform downstream consumers of potential breaking changes.
4-6: Clear Description ofabortSignalAnyBehavior
The note explains thatabortSignalAnywill useAbortSignal.anywhen available and fall back toAbortControllerfor compatibility. This concise explanation is helpful; ensure that any related documentation elsewhere in the project is also updated to reflect this behavior..changeset/little-plums-perform.md (1)
1-3: Dependency Version Update Confirmation
Similar to the other changeset, this file specifies the major version update for@graphql-hive/gateway-abort-signal-any. Consistency in documenting this dependency update across changesets is appreciated.
🚀 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
|
|
Yes, I am on fixing the tests. |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
enisdenjo
force-pushed
the
abort-sig-any
branch
from
4105c09 to
35ae446
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/abort-signal-any/src/index.ts (1)
10-13: Convert TODO into proper documentation.The TODO comment contains valuable information about compatibility with whatwg-node signals. Consider converting it into a proper JSDoc comment to better document this limitation.
- // TODO: we cant use the native abortsignal.any because it doesnt work with signals coming from whatwg-node (ServerAdapterRequestAbortSignal) - // if ('any' in AbortSignal) { - // return AbortSignal.any(signals); - // } + /** + * Note: We can't use the native AbortSignal.any because it doesn't work with + * signals coming from whatwg-node (ServerAdapterRequestAbortSignal). + * The following native implementation is kept for reference: + * ```ts + * if ('any' in AbortSignal) { + * return AbortSignal.any(signals); + * } + * ``` + */
📜 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 (8)
.changeset/@graphql-hive_gateway-abort-signal-any-593-dependencies.md(1 hunks).changeset/famous-mangos-rest.md(1 hunks).changeset/little-plums-perform.md(1 hunks).changeset/long-beds-dream.md(1 hunks).changeset/real-seals-taste.md(1 hunks)packages/abort-signal-any/package.json(0 hunks)packages/abort-signal-any/src/index.ts(1 hunks)packages/runtime/src/plugins/useUpstreamCancel.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/abort-signal-any/package.json
✅ Files skipped from review due to trivial changes (1)
- .changeset/@graphql-hive_gateway-abort-signal-any-593-dependencies.md
🚧 Files skipped from review as they are similar to previous changes (5)
- .changeset/long-beds-dream.md
- .changeset/real-seals-taste.md
- .changeset/little-plums-perform.md
- .changeset/famous-mangos-rest.md
- packages/runtime/src/plugins/useUpstreamCancel.ts
🧰 Additional context used
📓 Path-based instructions (1)
packages/abort-signal-any/src/index.ts (2)
Pattern 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 changeset with a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mention minor semver change. When a bug is fixed, patch needs to be used. The major bump is used for a PR that has breaking changes.
Pattern **: 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 console project.
Use the global knowledge feature to search for PRs in graphql-hive/console repository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Bundle
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Snapshot / snapshot
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Build
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/abort-signal-any/src/index.ts (3)
1-2: LGTM! Clean and efficient initialization.The parameter name change to
iterablebetter reflects its type, and the upfront array conversion is necessary for multiple iterations.
4-8: LGTM! Efficient early abort check.Good optimization to return an already aborted signal without setting up event listeners.
15-28: LGTM! Clean implementation with proper cleanup.The implementation correctly chains signal abortion and includes proper cleanup of event listeners, which aligns with the PR's goal of improving memory consumption.
Let's verify the cleanup implementation with a test script:
✅ Verification successful
Cleanup logic in abort-signal-any is correctly implemented.
- The event listener for the 'abort' event is added and properly removed on abort, preventing potential memory leaks.
- The shell script confirms that this pattern matches similar implementations in the codebase, ensuring consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential memory leaks in event listener cleanup # Search for similar patterns in the codebase to ensure consistent cleanup rg -A 5 'addEventListener.*abort' --type tsLength of output: 709
Closes #303
Ref GW-30, ref GW-140
Fix is confirmed through local load tests with constant 10 VUs over 3mins:
TODO