@tus/server: don't use AbortSignal.any to fix memory leak #718
Conversation
🦋 Changeset detectedLatest commit: e0526c6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
WalkthroughThis update patches the @tus/server package and refactors the MemoryLocker class to avoid using AbortSignal.any. The changes remove the problematic abortSignal logic and replace it with an abortController along with a new onAbort event listener. Additionally, the dependency has been updated via the changeset file, ensuring that the fix mitigates memory leaks in older Node.js versions without raising the minimum required Node.js version. Changes
Assessment against linked issues
✨ 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
Documentation and Community
|
![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 (1)
packages/server/src/lockers/MemoryLocker.ts (1)
52-63: LGTM! Consider enhancing error handling.The new implementation properly handles abort signals and ensures cleanup. However, consider adding explicit error handling for the Promise.race to distinguish between timeout and other potential failures.
const lock = await Promise.race([ this.waitTimeout(abortController.signal), this.acquireLock(this.id, requestRelease, abortController.signal), -]) +]).catch(error => { + if (error === ERRORS.ERR_LOCK_TIMEOUT) { + throw error; + } + throw new Error(`Failed to acquire lock: ${error.message}`); +});
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/silver-students-invent.md(1 hunks)packages/server/src/lockers/MemoryLocker.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/silver-students-invent.md
🔇 Additional comments (2)
packages/server/src/lockers/MemoryLocker.ts (2)
68-71: Excellent cleanup implementation!The cleanup in the finally block ensures proper resource management by:
- Removing the event listener
- Aborting the controller
This prevents memory leaks and ensures resources are released even if errors occur.
53-57: Successfully addresses memory leak concerns.The implementation avoids AbortSignal.any by using a single AbortController and proper event listener management, effectively addressing the memory leak issue mentioned in the PR objectives.
Let's verify that AbortSignal.any is not used anywhere else in the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining usage of AbortSignal.any rg -l "AbortSignal\.any"Length of output: 26
I'll re-run the search using a more precise command to verify that no usage of AbortSignal.any exists in the repository:
🏁 Script executed:
#!/bin/bash # Search for any occurrence of AbortSignal followed by '.any' rg -n "AbortSignal" | grep -E "\.any"Length of output: 37
Memory leak fix verified.
The repository does not contain any usage of AbortSignal.any, and the changes in MemoryLocker.ts correctly prevent potential leaks by using a single AbortController with proper event listener cleanup.
Closes #717
Closes #716