tus / tus-resumable-upload-protocol

Open Protocol for Resumable File Uploads
https://tus.io
MIT License
1.48k stars 103 forks source link

Protocol does not handle race against a resource #146

Closed mmatuska closed 2 months ago

mmatuska commented 4 years ago

The protocol does not tell us what to do if a PATCH, DELETE or HEAD request is submitted against the same resource as an already running PATCH upload. How should the server tell the client that the resource is busy?

Acconut commented 4 years ago

That's true, the protocol does not specify this. Most implementations (e.g. tusd) do not allow concurrent access and will return a 423 Locked status if they detect parallel requests to the same upload resource.

However, since an implementation may also allow concurrent requests (e.g. a HEAD request while a PATCH request is running), the protocol does not currently force locking upon implementations.

mitar commented 4 months ago

One alternative to locks is something called idempotency keys. It is very useful when primarily source of conflicts comes from the fact that the client itself retried, but the server has not yet noticed that the previous attempt failed.

Acconut commented 2 months ago

I am not sure that idempotency keys could help in this case. If the server is still handling a request (that might be abandoned by the client) and another request with the same idempotency key arrives, the server still needs to figure out to handle a potential conflict. Or am I missing something from your proposal?

Acconut commented 2 months ago

I would like to leave another comment regarding the original comment that the tus protocol does not specify how to handle concurrent access to an upload resource. By leaving this out of the spec, we give the implementations for freedom to develop and experiment with different methods for controlling this concurrency, instead of restricting us to one approach, which might not work well in every use case. For example, the locking approach in tusd has evolved over the years and now features a method where newer requests can terminated earlier, abandoned request to unlock an upload resource (in a safe way).

mitar commented 2 months ago

I am not sure that idempotency keys could help in this case. If the server is still handling a request (that might be abandoned by the client) and another request with the same idempotency key arrives, the server still needs to figure out to handle a potential conflict. Or am I missing something from your proposal?

How I imagine idempotency keys could help is that if another request comes with the same idempotency key then the server can know that the new request is coming from the same client so it can reuse the old lock. So in a way, locks are held by idempotency keys.

Acconut commented 2 months ago

That makes sense, thank you! Your idea seems to have a similar effect to how locks are implemented in tusd: Whenever a request arrives for a locked upload, tusd will attempt to stop and shutdown the earlier response that holds the lock in an orderly fashion, thereby releasing the lock. The newer request can then acquire the lock and access the upload resource to resume the upload. Effectively, the lock is transferred without risking the upload resource's integrity. More details at https://tus.github.io/tusd/advanced-topics/locks/#avoiding-locked-uploads. This is also how concurrent access should be handled in the draft for HTTP resumable uploads: https://datatracker.ietf.org/doc/draft-ietf-httpbis-resumable-upload/

The only noticeable different is that tusd does not require a shared token, such as the Idempotency-Key you mentioned. With tusd's setup any user that has access to an upload resource (might be restricted through authentication), can request a lock to be released. However, including a shared token is an interesting idea and worth discussing. It could prevent other clients from interrupting an upload (accidentally or incidentally).

mitar commented 2 months ago

A nice way to frame it. Thanks. :-)

Acconut commented 2 months ago

I will close this issue for now as the original question has been answered: tus leaves the question about upload locking up to the server implementation. The IETF draft we are working on contains explicit requirements about locking.