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

@tus/server: do not emit chunkFinished on error #446

Merged
merged 2 commits into from Jun 16, 2023

Conversation

fenos
Copy link
Collaborator

@fenos fenos commented Jun 15, 2023

Related to #444

If an error occurs during the write stream, we don't want to broadcast the chunkFinished event.
This should fix potential race conditions

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.

Thanks a lot for looking into this!

packages/server/src/models/StreamSplitter.ts Outdated Show resolved Hide resolved
@Murderlon
Copy link
Member

CI fails on the linter currently

@fenos
Copy link
Collaborator Author

fenos commented Jun 15, 2023

@Murderlon actually, I think a cleaner way to handle this is to have its own error handler function instead of mixing it into the _finishChunk function, for example:

  async _handleError() {
    if (this.fileHandle === null) {
      return
    }

    await this.fileHandle.close()
    this.currentChunkPath = null
    this.fileHandle = null
  }

It feels cleaner doing it this way, since we don't even need to advance the part to next chunk either.

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.

Thank you!

@Murderlon
Copy link
Member

@Murderlon actually, I think a cleaner way to handle this is to have its own error handler function instead of mixing it into the _finishChunk function, for example:

Probably a bit cleaner yes. Let's add it.

@fenos fenos force-pushed the fix/prevent-race-condition-on-error branch from 218e57d to f0c91a5 Compare June 16, 2023 06:03
@fenos
Copy link
Collaborator Author

fenos commented Jun 16, 2023

@Murderlon Just sent the patch using its dedicated error handler function

@Murderlon Murderlon changed the title Do not emit chunkFinished on error @tus/server: do not emit chunkFinished on error Jun 16, 2023
@Murderlon Murderlon merged commit 58ca5f1 into tus:main Jun 16, 2023
3 checks passed
@Murderlon
Copy link
Member

Released https://github.com/tus/tus-node-server/releases/tag/%40tus%2Fserver%401.0.0-beta.6

@fenos fenos deleted the fix/prevent-race-condition-on-error branch June 16, 2023 19:16
Murderlon added a commit to mitjap/tus-node-server that referenced this pull request Jul 6, 2023
* main:
  @tus/server: call onUploadFinish in POST handler for empty files (tus#453)
  @tus/gcs-store@1.0.0-beta.3
  Fix npm publish in .yarnrc.yml
  @tus/s3-store@1.0.0-beta.6
  @tus/server@1.0.0-beta.6
  @tus/server: do not emit chunkFinished on error (tus#446)
  @tus/server: replace type-fest with raw types (tus#443)
  @tus/s3-store: replace underscores with hyphens in metadata field names (tus#441)
  Fix logo in README.md
  Bump aws-sdk from 2.1325.0 to 2.1368.0 (tus#423)
  Bump prettier from 2.8.0 to 2.8.8 (tus#424)
  @tus/server: use Buffer.byteLength for Content-Length (tus#432)
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.

None yet

2 participants