RESUMABLE: Require offset and length to match upon completion#3463
RESUMABLE: Require offset and length to match upon completion#3463Acconut wants to merge 7 commits into
Conversation
|
Why do we deactivate the upload resource instead of simply responding with a 4xx error with I'd think we'd want to just make this fall under the existing inconsistent length problem type. |
|
You're right. We can adjust the language to meet the existing requirements on keeping the length consistent. |
|
I think this should be a hard failure. It's unclear how this could be the result of a transmission error, other than TCP somehow corrupting the bytes which is outside the scope of the protocol to recover. However, I'm also fine with it being retryable since it doesn't complicate the implementation. |
|
Even if it isn't a realistic transmission error, the server implementation may be able to be simpler if it can just reuse the inconsistent upload length 4xx response, rather than special-casing this one means that the upload length can be inconsistent. |
Apply the terminology and problem type updates from resumable-upload-length. Co-authored-by: Cursor <cursoragent@cursor.com>
|
I've adjusted the section of length to use representation's length instead of upload length. We introduced this term, but it has the same meaning as the representation's length, so I think this makes it easier for the reader to understand what value is communicated. It should also be clearer that the length must stay consistent over the upload. The Please let me know what you think. |
| ### Length {#upload-length} | ||
|
|
||
| The length of an upload is the number of bytes of representation data that the client intends to upload. | ||
| The length of the representation data might not be known when starting the transfer, for example, because the representation is taken from a streaming source. The representation's length will, however, be known at the latest when the upload is complete. |
There was a problem hiding this comment.
This last sentence is untrue. The server can mark an upload as completed (I assume you meant "completed", not "complete") before the length is known.
There was a problem hiding this comment.
I'm not sure I understand. The length will always be known when the upload is complete (i.e. request with Upload-Complete: ?1 was fully received) because the full representation has been received at that point.
There was a problem hiding this comment.
The server can send an early response with Upload-Complete: ?1, deeming the upload complete without a request being sent with Upload-Complete: ?1.
There was a problem hiding this comment.
In that case the upload is complete and the server learns the length of the transferred representation. No further data can be appended and the length is final.
There was a problem hiding this comment.
But the client never finished transferring the representation, and it didn't communicate the length through either of the two length indicators this PR documents, so the server cannot know the full representation's length. Setting aside semantics though, the server implementation required to actually compute the length from a request discarded early seems useless and unnecessarily complex when the server no longer needs to be aware of the length. The server still won't necessarily know the length, and unlike for the 2 indicators, it isn't required to.
There was a problem hiding this comment.
Can we just rephrase this as “when the client completes the upload”?
There was a problem hiding this comment.
Can we just rephrase this as “when the client completes the upload”?
That's possible. Thinking out load, the consequence would be that if the server serves an early response, the upload either
- is completed and does not have a length, or
- is completed, but
offset < length.
Both seem fine to me since the server stopped the upload early, so it's not unexpected that the upload is halted in such an intermittent state. Using the suggested phrasing would resolve the topic in a rather simple way, wouldn't it?
There was a problem hiding this comment.
Yes, it would! (I swore I reacted to Guoye's comment with 👍 before, but it disappeared.)
| - MUST include the offset in the `Upload-Offset` header field ({{upload-offset}}), | ||
| - MUST include the `Upload-Complete` header field ({{upload-complete}}) indicating whether a final response was produced from processing the uploaded representation, | ||
| - MUST include the length in the `Upload-Length` header field, unless the client has not supplied one, by providing the corresponding headers as described in ({{upload-length}}), | ||
| - MUST include the representation's length in the `Upload-Length` header field if the client has supplied one through corresponding headers as described in ({{upload-length}}), |
There was a problem hiding this comment.
Why don't we just remove "through corresponding headers" since that could be misinterpreted as excluding the final request content length itself as a signal? I know you're referring to when Upload-Complete: ?1 is present, but I don't think that header itself is what supplies the representation's length. This previously referred to either Upload-Length or Upload-Offset+Content-Length, which actually did supply the length.
| - MUST include the representation's length in the `Upload-Length` header field if the client has supplied one through corresponding headers as described in ({{upload-length}}), | |
| - MUST include the representation's length in the `Upload-Length` header field if the client has indicated one as described in ({{upload-length}}), |
There was a problem hiding this comment.
There original phrasing has been chosen on purpose based on feedback from the previous Working Group Last Call (#3204). So let's keep that and just adjust it to use "representation's length".
| - MUST include the representation's length in the `Upload-Length` header field if the client has supplied one through corresponding headers as described in ({{upload-length}}), | |
| - MUST include the representation's length in the `Upload-Length` header field, unless the client has not supplied one, by providing the corresponding headers as described in ({{upload-length}}), |
There was a problem hiding this comment.
Yeah, but that was before the representation's length was specified as being deduced from things other than headers. I don't think this phrasing is appropriate anymore. The reason that phrasing was chosen is because it clarifies under what circumstances the server must know the length. Citing the aforementioned indicators rather than "headers" should be equally sufficient.
If you disagree, I'd want to hear @LPardue's opinion on this before this is merged. (I don't consider my comment resolved, so if you're able to un-mark it as resolved, that would be nice so I don't have to start a new thread for this review.)
There was a problem hiding this comment.
How about this?
- MUST include the representation's length in the
Upload-Lengthheader field, unless the server has not discovered the length through the mechanisms described in ({{upload-length}}),
There was a problem hiding this comment.
This addresses my problem, but to also address the point of the aforementioned PR, shouldn't it mention the client indicating the length rather than the server discovering it?
|
This may be out of scope, but do we want to rename the Basing this on |
Co-authored-by: Grant Gryczan <grantgryczan@gmail.com>
There's value in keeping the header naming consistent (Upload-Complete, Upload-Offset, Upload-Limit, Upload-Length), so the prefix serves a good purpose. In addition, renaming it to Repr-Length would raise more questions about its relation to Content-Length and its general use outside of uploads. I'm not sure there's much to gain here. |
|
|
||
| Title: | ||
| : Inconsistent Upload Length Values | ||
| : Inconsistent Representation Length Values |
There was a problem hiding this comment.
Should we keep the problem type inconsistent-upload-length since the new name sounds like it might be more widely applicable?
We already defined upload length to be the representation’s length to be uploaded. I suspect most people are still confused about resource vs representation vs content.
There was a problem hiding this comment.
We can also keep the previous name 👍
| ### Length {#upload-length} | ||
|
|
||
| The length of an upload is the number of bytes of representation data that the client intends to upload. | ||
| The length of the representation data might not be known when starting the transfer, for example, because the representation is taken from a streaming source. The representation's length will, however, be known at the latest when the upload is complete. |
There was a problem hiding this comment.
Can we just rephrase this as “when the client completes the upload”?
Closes #3430.
This PR clarifies that the offset and length must match when the upload is about to be completed. The draft had already requirements that the length derived from
Content-Lengthis consistent, but there was a loophole when the header is missing:(from #3430 (comment))
This gap is closed by explicitly saying that the length must be consistent in the end.