@tus/server: handle request cancellation gracefully #774
Conversation
🦋 Changeset detectedLatest commit: 8ab3a55 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 |
WalkthroughA new changeset file was added to document a patch update for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
Murderlon
changed the title
![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/server.ts (1)
288-298: Good implementation of connection closing for aborted requests.The method properly sets the
Connection: closeheader when requests are aborted, which aligns with the PR objective to prevent clients from sending entire request bodies for cancelled uploads.Consider simplifying the method by removing the redundant abort check since the caller already verifies this condition:
protected handleAbortedRequest(context: CancellationContext, resp: Response) { - const isAborted = context.signal.aborted - if (isAborted) { - // If the request was aborted, we should not send any response body. - // The server should just close the connection. - resp.headers.set('Connection', 'close') - return resp - } - + // If the request was aborted, we should not send any response body. + // The server should just close the connection. + resp.headers.set('Connection', 'close') return resp }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/modern-birds-appear.md(1 hunks)packages/server/src/server.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/server/src/server.ts (1)
packages/utils/src/models/Context.ts (1)
CancellationContext(21-25)
🔇 Additional comments (3)
packages/server/src/server.ts (2)
137-146: LGTM! Excellent implementation of graceful request cancellation handling.The Node.js runtime-specific error handling properly addresses the PR objectives by listening for error events (disconnects, timeouts) and immediately aborting the context. This ensures locks are released promptly for subsequent requests.
226-235: Well-implemented abort handling after request processing.The implementation correctly checks the abort status after the handler completes and delegates to
handleAbortedRequestwhen needed. This prevents sending response bodies for cancelled requests and ensures proper connection management..changeset/modern-birds-appear.md (1)
1-6: LGTM! Changeset documentation is accurate and appropriately scoped.The changeset correctly identifies this as a patch-level change for
@tus/serverwith a clear description that aligns with the implementation changes.
2 participants
This PR fixes 2 regressions:
Handle gracefully the request cancellation by aborting the context, which will allow the locks to be released immediately and available to be acquired by subsequent requests. Currently, this has been broken since the context was never cancelled when the request was aborted
Force closing the connection when the request was aborted, to prevent the client from needing to send the whole body before realising that the upload was cancelled (potentially from another request acquiring the lock)
CC: @Murderlon