Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handling consistent cancellation across stream and locks #699

Merged
merged 2 commits into from
Jan 18, 2025

Conversation

fenos
Copy link
Collaborator

@fenos fenos commented Jan 17, 2025

This PR fixes #697 by always cancelling the context when the req.on('close') is emitted.

The close event on the request (req) object is emitted when the underlying connection is terminated before the request is fully processed. This typically happens when the client abruptly disconnects, closes the browser, or cancels the request (e.g., a page refresh or navigation away).

Handled Cancellation scenarios:

  • Client aborts the request (signal is propagated to the proxy stream and lockers)

    • If the locker is trying to acquire a lock, it will stop since there is no more need
    • Lock is acquired, the proxy stream is closed gracefully so that we can finishing uploading the remaining bytes in the buffer before releasing the lock
  • Client completes the request correctly (without keep alive) the on('close') is still triggered, it will not have any effect but probably good practice to make sure all future resources are clean up.

  • Client completes the request correctly (with keep-alive) the on('close') is not triggered, context is still cancelled upon req.on('finish'), it will not have any effect but probably good practice to make sure all future resources are clean up.

Copy link

changeset-bot bot commented Jan 17, 2025

⚠️ No Changeset found

Latest commit: 5291f62

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@fenos fenos force-pushed the fix/handling-consistent-cancellation branch 6 times, most recently from cf72ff1 to 4184c93 Compare January 17, 2025 16:39
@fenos
Copy link
Collaborator Author

fenos commented Jan 17, 2025

@Murderlon when you have sometime please have a test with this.
From my testing seems to work nicely

@fenos fenos force-pushed the fix/handling-consistent-cancellation branch from 4184c93 to 5291f62 Compare January 17, 2025 16:54
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for picking it up immediately.

@Murderlon Murderlon merged commit 42c6267 into main Jan 18, 2025
3 checks passed
@Murderlon Murderlon deleted the fix/handling-consistent-cancellation branch January 18, 2025 13:25
Murderlon added a commit to netdown/tus-node-server that referenced this pull request Jan 20, 2025
* main: (59 commits)
  Replace demo folder with StackBlitz (tus#704)
  @tus/gcs-store: correctly pass content type (tus#702)
  @tus/s3-store: fix zero byte files (tus#700)
  Update package-lock.json
  [ci] release (tus#696)
  fix: handling consistent cancellation across stream and locks (tus#699)
  @tus/s3-store: Change private modifier into protected (tus#698)
  Create funding-manifest-urls
  Bump @aws-sdk/client-s3 from 3.703.0 to 3.717.0 (tus#695)
  Bump mocha from 10.4.0 to 11.0.1 (tus#693)
  Bump @biomejs/biome from 1.9.2 to 1.9.4 (tus#694)
  [ci] release (tus#690)
  Bump @aws-sdk/client-s3 from 3.701.0 to 3.703.0 (tus#685)
  @tus/s3-store: fix part number increment (tus#689)
  Revert "Bump rimraf from 3.0.2 to 6.0.1 (tus#681)"
  Bump @aws-sdk/client-s3 from 3.682.0 to 3.701.0 (tus#683)
  Bump @changesets/cli from 2.27.9 to 2.27.10 (tus#682)
  Bump rimraf from 3.0.2 to 6.0.1 (tus#681)
  Bump @types/node from 20.11.5 to 22.10.1 (tus#679)
  Ignore JSON for Biome formatting
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lock on file never unlocked when request ends prematurely
2 participants