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

lower TUS max chunk size from infinite to 0.1 GB #2584

Merged
merged 1 commit into from
Oct 20, 2021
Merged

Conversation

wkloucek
Copy link
Contributor

@wkloucek wkloucek commented Oct 6, 2021

Description

We've lowered the TUS max chunk size from infinite to 0.1GB so that chunking actually happens.

Related Issue

Motivation and Context

How Has This Been Tested?

  • locally

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@sonarcloud
Copy link

sonarcloud bot commented Oct 6, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@butonic
Copy link
Member

butonic commented Oct 6, 2021

why would this fail? just because the POST request cannot stat the uploaded file when the upload finishes after the token has expired. IMO this is not the correct fix:

  1. When the upload finishes the task of the client is done (he transferred all bytes) and then can immediately close the connection. at least this is what I understood is what @dragotin expects.
  2. But that means the client cannot expect to get an etag. ... well we could roll a random etag and persist that in the extended attributes. then we could return this preleminary etag in the response... when postprocessing is done without changing the content that etag can be stored in the extended attributes
  3. should we return the checksum as the etag? hm ... I would prefer a random string

now taking this one step further:
how should the etag behave when we return files that are currently being uploaded in the propfind listing?

  1. we could list the preleminary etag in the propfind response
  2. when some postprocessing changes the file the mtime and etag (and the checksums) would change ... allowing the client to sync back the change.

The client should be able to transfer as many bytes as possible. Without chunking. He should be able to resume the upload and discover how many bytes are already on the server. For that the transfer token needs to be valid for a longer period of time. The TUS protocol has an expiration extension that lets clients know how long they can resume an upload: https://tus.io/protocols/resumable-upload.html#expiration The Upload-Expires header would be determined by the transfer token lifetime.

@wkloucek wkloucek closed this Oct 7, 2021
@wkloucek
Copy link
Contributor Author

wkloucek commented Oct 7, 2021

Max chunk size default should not be lowered if not needed. The upload needs to be fixed. In the meantime one could work around by lowering the chunk size to mitigate expiring tokens.

@wkloucek wkloucek deleted the lower_tus_size branch October 7, 2021 12:28
@dragotin
Copy link
Contributor

The max chunk size has to be limited when we go in production with the product, maybe to 100 MB. It has to be configurable, but let us have a reasonable default pls to provide a robust product.

Otherwise we end up with support load with every new installation, because in most installations there will be a limit.

@wkloucek wkloucek restored the lower_tus_size branch October 14, 2021 13:50
@wkloucek wkloucek reopened this Oct 14, 2021
@wkloucek
Copy link
Contributor Author

The max chunk size has to be limited when we go in production with the product, maybe to 100 MB. It has to be configurable, but let us have a reasonable default pls to provide a robust product.

Otherwise we end up with support load with every new installation, because in most installations there will be a limit.

@butonic is right, we should not set a arbitrary limit without reasoning. In tus/tus-resumable-upload-protocol#93 (comment) are some examples. But there is no "hard limit" for a "general usecase". It highly depends on the specific usecase... That's why it is configurable.

@wkloucek wkloucek requested review from dragotin and removed request for C0rby and refs October 14, 2021 13:59
Copy link
Contributor

@dragotin dragotin left a comment

Choose a reason for hiding this comment

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

Yes please, ship that!

@TheOneRing agreed.

@wkloucek wkloucek merged commit d6975c4 into master Oct 20, 2021
@delete-merged-branch delete-merged-branch bot deleted the lower_tus_size branch October 20, 2021 15:07
ownclouders pushed a commit that referenced this pull request Oct 20, 2021
Merge: a378595 8659272
Author: Willy Kloucek <[email protected]>
Date:   Wed Oct 20 17:07:44 2021 +0200

    Merge pull request #2584 from owncloud/lower_tus_size

    lower TUS max chunk size from infinite to 0.1 GB
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.

4 participants