Implement Upload-Defer-Length Extension #182

acj · 2018-04-29T00:30:46Z

This is a first pass at supporting the upload-defer-length protocol extension. It's worked well in my own testing, but I suspect that there may be edge cases that aren't being handled properly. For example, it's difficult for the S3 backend to ensure that the minimum chunk size is respected when the full file size isn't known.

Happy to discuss and iterate on this further.

Acconut · 2018-04-29T21:23:03Z

That is amazing, great work! I never expected to see such as great PR out of the void! I only had a quick look over this but it appears to be done very solid.

Your concern about data stores which do not support deferring lengths is very valid. As you said the S3 data store would not support this extension without additional work. Therefore, I would propose that data stores explicitly opt-in if they support deferring length. This would work similar to how data stores announce that they support the concatenation extension, for example:

Create a new LengthDeferrerDataStore (or similarly named) interface in https://github.com/tus/tusd/blob/master/datastore.go. This interface only requires a SupportsDeferredLength() bool function to be implemented which is always expected to return true. The purpose of this method is that a data store explicitly express their support of the extension.
Add a UseLengthDeferrer function to the Composer struct similar to UseConcat in https://github.com/tus/tusd/blob/master/composer.go#L119-L122. This function is used by the tusd user to say that they want to enable the extension.
Datastores then can call the UseLengthDeferrer function to announce their support as done in https://github.com/tus/tusd/blob/master/filestore/filestore.go#L52

What do you think about this approach? If this is a bit confusing, I am happy to explain it to you.

acj · 2018-04-30T12:40:20Z

@Acconut That approach sounds good to me. I'll work on implementing it for this PR. Thank you for the detailed suggestion.

The S3 data store is my primary use case, so I'd like to make it capable of deferring the upload length -- probably in a separate PR. I think that the key problem is recognizing when we've written a chunk smaller than MinPartSize (which should be the terminal chunk) and then try to write a subsequent chunk.

Do you think it would suffice to add a field to FileInfo, something like HaveWrittenShortChunk bool, that we can inspect in the S3 data store's WriteChunk function and then abort the upload with an appropriate error?

Acconut · 2018-04-30T18:17:39Z

The S3 data store is my primary use case, so I'd like to make it capable of deferring the upload length -- probably in a separate PR

Sounds good to me 👍

I think that the key problem is recognizing when we've written a chunk smaller than MinPartSize (which should be the terminal chunk) and then try to write a subsequent chunk.

Yes, exactly, that is the main problem with the S3 storage.

Do you think it would suffice to add a field to FileInfo, something like HaveWrittenShortChunk bool, that we can inspect in the S3 data store's WriteChunk function and then abort the upload with an appropriate error?

No, I don't think that this is a good solution. If a user sends a patch request with a size smaller than MinPartSize (this does not have to happen on purpose but can just be caused by a network interruption), they will not be able to contain the upload because HaveWrittenShortChunk will be set to true. This forces users to be aware that tusd is using AWS S3 as a backend and to act accordinly. IMO, tusd should be an abstraction above the data store and not leak such implementation details.

On the other hand, I do not have a perfect alternative approach. The one that I think is best suited is following: If we receive a chunk which is larger than MinPartSize, we upload is as a part to our multipart upload. If the chunk is too small, we do not upload it to the multipart upload but instead store it inside a intermediate different S3 object (which is not a multipart upload). Then if we receive the next chunk, we first download the intermediate S3 object to prepend it to the next chunk which then (hopefully) is bigger than MinPartSize, so we can upload it as a multipart part.

This approach is rather complex but might solve the problem. What do you think?

acj · 2018-05-03T10:18:53Z

Sorry for the delay. I hadn't considered the network interruption case -- that changes things a bit. Your approach makes sense to me, though, and seems safe if tusd is configured with a locker. I appreciate the suggestion and will try it in a separate PR.

Acconut · 2018-05-08T17:36:41Z

gcsstore/gcsstore.go

@@ -257,3 +258,7 @@ func (store GCSStore) GetReader(id string) (io.Reader, error) {

 	return r, nil
 }
+
+func (store GCSStore) SupportsDeferredLength() bool {
+	return true


Frankly, I am not so sure whether GCSStore supports this extension out of the box.

Acconut · 2018-05-08T17:37:47Z

No worries, I wasn't really available either. May I ask what's the status on this PR? Is it finished or are you still working on it?

acj · 2018-05-08T20:04:45Z

It's finished in terms of implementation, but I wanted to ask if any additional tests are needed. Since you mentioned it in another comment, is there anything specific to GCSStore that concerns you?

Acconut · 2018-05-10T17:23:50Z

Great, however I believe there is still a function missing in the PATCH handler which permanently stores the upload's size once it's known for uploads with deferred length. Currently, the length is only attached to the PATCH request's response (https://github.com/tus/tusd/pull/182/files#diff-4e64569f67c7c0214e072f52dbe8d504R466) but further PATCH or HEAD requests will not contain the correct upload's length, AFAIK.

Since you mentioned it in another comment, is there anything specific to GCSStore that concerns you?

Yes, there is. Frankly, I am not very proficient in the GCSStore code and since it has not been shown yet that GCSStore is compatible with this extension, I would not assume it is.

acj · 2018-05-12T21:14:41Z

I believe there is still a function missing in the PATCH handler which permanently stores the upload's size once it's known for uploads with deferred length.

Yes, good catch. I've added a PutInfo function on DataStore to handle persistence.

since it has not been shown yet that GCSStore is compatible with this extension, I would not assume it is.

Fair enough. I haven't found any documentation that suggests a problem, but it would be good to test this against a real GCS bucket. I've disabled it for now.

Please let me know if any other changes or tests are needed.

Acconut · 2018-05-13T12:51:13Z

I've added a PutInfo function on DataStore to handle persistence.

Great idea, however I would have made the function less general, e.g. DeclareLength instead of PutInfo. The reason is that users may assume that the PutInfo function is able of overwriting any value in the FileInfo struct. However, this is not always the case:
For example, the ID, IsPartial, IsFinal and PartialUploads properties should never be changed after the upload has been created. The Offset property should only be changed when WriteChunk is involved. On S3 (and also possibly other stores) metadata cannot be modified after an upload has been created.
Therefore, I would propose to just add a DeclareLength(length int) error function to the LengthDeferrerDataStore inferface. In that turn we can also get rid of the SupportsDeferredLength which I only proposed so that we do not have an empty interface.

Sorry for this inconvenience but does this make sense? Everything else is looking perfect 👍

acj · 2018-05-13T13:16:55Z

No worries. That makes sense and sounds good to me. I'll make the suggested change.

the ID, IsPartial, IsFinal and PartialUploads properties should never be changed after the upload has been created

Do you think this property should be documented somewhere, e.g. on each field of the FileInfo struct? It seems obvious in retrospect, but I didn't think about it when I was working on the last batch of changes.

acj · 2018-05-13T14:49:20Z

Okay, I did a bit of branch cleanup and force-pushed. Your DeclareLength idea worked out very well, I think. It's cleaner, simpler, and easier to understand. 👍

Acconut · 2018-05-15T19:29:20Z

This looks amazing, thank you very much! I had a quick look over this but I am going to test it in full depth this week.

Acconut

I had a deeper look into this and it's very solid work! There are still some edge cases but those are hard to spot, let me know what you think about them. Also, I took the freedom to merge master into your branch, so you don't have to deal with the merge conflict.

Furthermore, the sizeOfUploads function, which is used for calculating the size when concatenating uploads, needs a check if the partial uploads have a deferred size: https://github.com/tus/tusd/pull/182/files#diff-4e64569f67c7c0214e072f52dbe8d504R867

Acconut · 2018-05-31T13:27:42Z

unrouted_handler.go

@@ -115,7 +119,7 @@ func NewUnroutedHandler(config Config) (*UnroutedHandler, error) {
 	}

 	// Only promote extesions using the Tus-Extension header which are implemented
-	extensions := "creation,creation-with-upload"
+	extensions := "creation,creation-with-upload,creation-defer-length"


creation-defer-length should only be added if the data storage support it, similar to how the Termination extension is handled in the next lines.

Acconut · 2018-05-31T13:33:22Z

unrouted_handler.go

+	if handler.composer.UsesLengthDeferrer && info.SizeIsDeferred && r.Header.Get("Upload-Length") != "" {
+		uploadLength, err := strconv.ParseInt(r.Header.Get("Upload-Length"), 10, 64)
+		if err != nil || uploadLength < 0 {
+			handler.sendError(w, r, ErrInvalidOffset)


ErrInvalidOffset seems to not be the correct error here. What about ErrInvalidUploadLength?

Acconut · 2018-05-31T13:37:26Z

unrouted_handler.go

+
+		info.Size = uploadLength
+		info.SizeIsDeferred = false
+		if err := handler.composer.LengthDeferrer.DeclareLength(id, info.Size); err != nil {


An additional check if the new length is not smaller than the current offset (info.Offset <= uploadLength) and the new length does not exceed the file limit (see https://github.com/tus/tusd/blob/master/unrouted_handler.go#L264-L268) before the call to DeclareLength would be needed.

Acconut · 2018-05-31T13:39:25Z

unrouted_handler.go

 		w.Header().Set("Upload-Offset", strconv.FormatInt(offset, 10))
 		handler.sendResp(w, r, http.StatusNoContent)
 		return
 	}

+	if handler.composer.UsesLengthDeferrer && info.SizeIsDeferred && r.Header.Get("Upload-Length") != "" {


Although this check is perfectly fine, I would love to see an error if someone tries to declare the length for an upload for a) an upload where length deferring is not enabled and b) an upload where the length has already been declared.

Acconut · 2018-05-31T13:53:56Z

unrouted_handler.go

@@ -460,7 +488,7 @@ func (handler *UnroutedHandler) writeChunk(id string, info FileInfo, w http.Resp
 	offset := info.Offset

 	// Test if this upload fits into the file's size
-	if offset+length > info.Size {
+	if !info.SizeIsDeferred && offset+length > info.Size {
 		return ErrSizeExceeded
 	}



The following lines where maxSize is defined also need a consideration if the size is deferred. Something like:

maxSize := info.Size - offset // If the upload's length is deferred and the PATCH request does not contain the Content-Length // header (which is allowed if 'Transfer-Encoding: chunked' is used), we still need to set limits for // the body size. if info.SizeIsDeferred { if handler.config.MaxSize > 0 { // Ensure that the upload does not exceed the maximum upload size maxSize = handler.config.MaxSize - offset } else { // If no upload limit is given, we will allowed arbitrary sizes maxSize = math.MaxInt64 } } if length > 0 { maxSize = length }

acj · 2018-06-03T20:09:02Z

Thanks again for the detailed feedback. I've fixed the things you pointed out, rebased onto current master, and pushed. Let me know if anything else needs to be done.

Acconut · 2018-06-04T18:24:10Z

Amazing! Thank you very, very much for your hard work and patience with my feedback :)

kvz · 2018-06-05T08:28:45Z

unrouted_handler.go

+				handler.sendError(w, r, ErrInvalidUploadLength)
+			}
+		} else {
+			handler.sendError(w, r, ErrNotImplemented)


I think christmas trees like these should be avoided if reasonably possible by returning home early. I think there's an official term for it, but I couldn't find it. Maybe this post that I did find helps illustrate the concept tho http://blog.timoxley.com/post/47041269194/avoid-else-return-early :)

i realize this pr has been merged already and it's nothing critical. just a friendly tip for future endeavours :)

Thanks for the feedback, @kvz. I agree -- an early return would be better. I also realized that this PR doesn't add Upload-Defer-Length to the CORS allowed header list, so I'll create a followup PR that addresses both.

Acconut reviewed May 8, 2018

View reviewed changes

acj force-pushed the acj/add-upload-defer-length-support branch from 89d69fc to 675a499 Compare May 13, 2018 14:36

Acconut requested changes May 31, 2018

View reviewed changes

acj added 15 commits June 3, 2018 12:56

Add SizeIsDeferred to FileInfo

82bc04c

Add error constants for deferred upload length

1affbbd

Add basic Upload-Defer-Length header handling

1ab5231

Add creation-defer-length to supported extensions

c94e0cd

Update the upload length in FileInfo when it's sent

64c02a9

Avoid incorrectly returning errors when upload length is deferred

5252c98

Add POST tests

acaaa10

Add HEAD tests

77a8455

Add LengthDeferrerDataStore interface

55f99cb

Add length deferrer support to composer

d40c50f

Add length deferrer support to FileStore

5bf2133

Add length deferrer support to FullDataStore

cb96d06

Regenerate handler mock

326986c

When it's requested, verify length deferrer support in the datastore

c6d5ad0

Integrate DeclareLength into handler's PatchFile

7bd1cd6

acj added 4 commits June 3, 2018 12:57

Add DeclareLength tests for PATCH requests

0976863

Conditionally include creation-defer-length if the composer supports it

d545b65

Return upload length error if it's too small or too large

22fdd39

Return appropriate errors if upload-length is misused in PATCH requests

a366b00

acj force-pushed the acj/add-upload-defer-length-support branch from 34b0afd to 8385faa Compare June 3, 2018 17:01

acj added 4 commits June 3, 2018 16:03

If size is deferred, then constrain how much of the request body to read

8c3ceeb

Remove creation-defer-length declaration from tests that don't use it

a74c468

Specify MaxSize in PATCH tests that refer to it

750d4d5

If a partial upload has a deferred size, then it's not finished

c605926

acj force-pushed the acj/add-upload-defer-length-support branch from 8f9785b to c605926 Compare June 3, 2018 20:03

Acconut approved these changes Jun 4, 2018

View reviewed changes

Acconut merged commit d9d0f7c into tus:master Jun 4, 2018

kvz reviewed Jun 5, 2018

View reviewed changes

acj mentioned this pull request Jun 11, 2018

Add CORS support for Upload-Defer-Length header #191

Merged

acj deleted the acj/add-upload-defer-length-support branch June 11, 2018 22:43

paulrosenzweig mentioned this pull request Jul 20, 2018

Upload streams from the browser tus/tus-js-client#118

Closed

acj mentioned this pull request Nov 19, 2018

S3Store: Add support for creation-defer-length extension #219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Upload-Defer-Length Extension #182

Implement Upload-Defer-Length Extension #182

acj commented Apr 29, 2018

Acconut commented Apr 29, 2018

acj commented Apr 30, 2018

Acconut commented Apr 30, 2018

acj commented May 3, 2018

Acconut May 8, 2018

Acconut commented May 8, 2018

acj commented May 8, 2018

Acconut commented May 10, 2018

acj commented May 12, 2018

Acconut commented May 13, 2018

acj commented May 13, 2018

acj commented May 13, 2018

Acconut commented May 15, 2018 •

edited

Acconut left a comment

Acconut May 31, 2018

Acconut May 31, 2018

Acconut May 31, 2018

Acconut May 31, 2018

Acconut May 31, 2018

acj commented Jun 3, 2018

Acconut commented Jun 4, 2018

kvz Jun 5, 2018

kvz Jun 5, 2018

acj Jun 11, 2018

Implement Upload-Defer-Length Extension #182

Implement Upload-Defer-Length Extension #182

Conversation

acj commented Apr 29, 2018

Acconut commented Apr 29, 2018

acj commented Apr 30, 2018

Acconut commented Apr 30, 2018

acj commented May 3, 2018

Choose a reason for hiding this comment

Acconut commented May 8, 2018

acj commented May 8, 2018

Acconut commented May 10, 2018

acj commented May 12, 2018

Acconut commented May 13, 2018

acj commented May 13, 2018

acj commented May 13, 2018

Acconut commented May 15, 2018 • edited

Acconut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acj commented Jun 3, 2018

Acconut commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Acconut commented May 15, 2018 •

edited