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

Error handling for Upload Creation Procedure #2596

Open
Acconut opened this issue Jul 19, 2023 · 6 comments
Open

Error handling for Upload Creation Procedure #2596

Acconut opened this issue Jul 19, 2023 · 6 comments

Comments

@Acconut
Copy link
Member

Acconut commented Jul 19, 2023

The current -01 draft for resumable upload says about the Upload Creation Procedure:

If the client received an informational repsonse with the upload URL, it MAY automatically attempt upload resumption when the connection is terminated unexpectedly, or if a server error status code between 500 and 599 (inclusive) is received. The client SHOULD NOT automatically retry if a client error status code between 400 and 499 (inclusive) is received.

This section talks about failure handling when an informational response was received. However, the draft does not describe how a client should/may act if no informational response was received and the client does not have an upload URL to resume.

Previously, we proposed the use of Idempotency-Key for this (#2293), but we didn't agree on including it in the draft.

Can we include some other recommendation or method on how clients should handle failures without an upload URL? For example,

If the client did not receive an informational response, it MAY retry the Upload Creation Procedure if it knows in advance that the server supports multiple Upload Creation Procedures for the same file. The client MAY use indicators, such as the Idempontent-Key header field, to make the Upload Creation Procedure idempotent.

If we do not agree on including a recommendation, should we add a statement that the client must then abort the upload because it failed?

@jrflat
Copy link

jrflat commented Jul 19, 2023

If no 104 response is received, then I think we might be outside the scope of the resumable upload protocol, and the client could do whatever it normally does for a 4xx or 5xx response. We could recommend that if a client receives a 4xx, it should remove the Upload-Complete and Upload-Draft-Interop-Version header fields and not attempt another resumable upload creation at that endpoint.

Because there seemed to be consensus on server-generated URLs over client-generated tokens for this draft, I think it might be best to keep Idempotency-Key (essentially a client generated token) outside the scope of this protocol. There's already two mechanisms defined with Upload-Complete: ?0 and Upload-Complete: ?1, and adding another client-generated protocol for resumable uploads in this draft might cause unneeded complexity.

Keeping that in mind, I don't think mentioning Idempotency-Key actually provides benefit unless we create a resumable upload protocol for it. For instance, if a client sends its Idempotency-Key in an upload creation procedure, doesn't receive a 104, and gets disconnected, it can retry the upload creation. However, it doesn't know what offset to resume from, so it misses the benefit of resumable uploads and may have well retried without the Idempotency-Key.

In order to benefit, we would need to define a way to do offset retrieving with the Idempotency-Key, and at that point we're defining an entirely separate client-generated resumable upload protocol, which I think we'd want to avoid.

@guoye-zhang
Copy link
Contributor

My understanding is Idempotency-Key wouldn't be used for resumption. It will only be used for repeating an upload creation procedure as is if the first one failed without any response.

I think Idempotency-Key can reduce the risk of replaying an non-idempotent request, but not completely eliminate it since the server cannot save infinite keys. It might be better to design the interactions to be idempotent. However, I'm not opposed to supporting it.

@Acconut
Copy link
Member Author

Acconut commented Oct 26, 2023

My understanding is Idempotency-Key wouldn't be used for resumption. It will only be used for repeating an upload creation procedure as is if the first one failed without any response.

Exactly, that is the intention here. The value of Idempotency-Key cannot be used for retrieving the offset, but just for retrying the upload creation in a way that servers avoid creating duplicate upload resources. Thus, it is not necessary to create an upload protocol around it.

If the client uploads data in the upload creation and a failure occurs without the client receiving a 104, the client would be able to retry the upload creation in a safer way. The ability to resume gets lost here as the client has to start the upload from the beginning again, but at least the server does not create duplicate resources. Of course, this should only be done if the client know in advance that the server supports idempotent upload creations through an out-of-spec mechanism.

I believe that an approach, such as Idempotency-Key is just an improvement without many drawbacks. It would be optional and does not place too strict requirements on implementations. However, the current draft offers no options for the client if the upload creation fails with a 104 response. An idempotent retry could help the client to continue the upload in this case. This would be in line with the goals of resumable upload, where we try to resume an upload whenever possible.

However, I'm not opposed to supporting it.

👍

@smatsson
Copy link

smatsson commented Nov 4, 2023

Exactly, that is the intention here. The value of Idempotency-Key cannot be used for retrieving the offset, but just for retrying the upload creation in a way that servers avoid creating duplicate upload resources. Thus, it is not necessary to create an upload protocol around it.

I'm a bit confused about how this would work. What is the server supposed to respond to the client here? I might be overthinking this but let's say that the client did a POST to start the upload and included data in the request (i.e. Upload-Complete: ?0). For some reason it did not receive the 104 response. The connection breaks down so the client restarts the upload from offset 0 as it has no way of knowing that the server supports resumable uploads. The same idempotency key are used in both requests.

What is the server supposed to respond with for the second request? I can see multiple correct responses here. One being that the server sends the 104 again and then fails the request with a Conflict due to Upload-Offset being wrong. Another that the server just returns an error. Not really sure which one is more correct?

@Acconut
Copy link
Member Author

Acconut commented Nov 9, 2023

The connection breaks down so the client restarts the upload from offset 0 as it has no way of knowing that the server supports resumable uploads. The same idempotency key are used in both requests.

Idempotency-Key should only be used if the client knows that the server supports resumable uploads and respects Idempotency-Key. Otherwise, this approach could lead to duplicate, unintended upload resources on the server.

What is the server supposed to respond with for the second request?

The server should respond with a 104 and 201 Created in this case. It should not create a new upload resource, but return the upload URL for the existing upload resource. The client will retransmit the data from offset 0. The server should ignore all bytes that it had already received and only start appending after the upload offset from the server's perspective.

For example, if an upload was interrupted after byte 100 in the POST request, the client will send bytes from offset 0 again. But the server "ignores" the first 100 bytes and only appends bytes starting at offset 101 to the upload.

Does that clear it up?

@smatsson
Copy link

@Acconut Yes makes sense. Thanks for clarifying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants