search

tus / tus-resumable-upload-protocol

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

100 Continue on PATCH requests #152

Open hamishforbes opened 4 years ago

hamishforbes commented 4 years ago

Is there a reason Expect: 100-continue is no longer mentioned as part of the PATCH request part of the upload flow? Only as part of the creation with upload extension

It looks like it used to be in there after #9 but is now missing.

The tusd reference server also sends HTTP/1.1 100 Continue responses to invalid PATCH requests that do include the Expect header.

It seems like supporting this would be a useful feature allowing errors to be nicely returned to clients without having to drop the connection or cause issues with intermediary proxies etc

Acconut commented 4 years ago

Good catch. The reason for removing that paragraph was (if I recall correctly) that the wording was a bit off:

If the clients sends an Expect request-header field with the 100-continue expectation the server SHOULD return the 100 Continue status code before reading the request's body and sending the final response.

You can understand the sentence that the server should always send a 100 Continue, even if an error response is going to be sent. Which is obviously wrong behavior and achieves the opposite of what 100-continue is intended for.

Would you mind opening a PR to add a better paragraph back in the specification?

hamishforbes commented 4 years ago

Sure, can do.

I also have a fix for the tusd server to return error responses instead of 100 Continue and then an error response.

Although this is less useful than I had originally hoped, it turns out nginx is hardcoded to always respond with 100 Continue and that cannot be disabled, so any tusd instance running behind an nginx proxy isn't going to be able to make use of 100-continue anyway

Acconut commented 4 years ago

I also have a fix for the tusd server to return error responses instead of 100 Continue and then an error response.

Awesome. Feel free to open a PR for it and we can discuss it!

Although this is less useful than I had originally hoped, it turns out nginx is hardcoded to always respond with 100 Continue and that cannot be disabled, so any tusd instance running behind an nginx proxy isn't going to be able to make use of 100-continue anyway

That's interesting and sad at the same time. Do you have a link or resource to get more information about this limitation?

hamishforbes commented 4 years ago

Here's a couple of discussions on the nginx behaviour https://trac.nginx.org/nginx/ticket/493 https://forum.nginx.org/read.php?2,212533,212549#msg-212549

and the relevant code https://github.com/nginx/nginx/blob/3ba88365b5acef17f01671cd969c909dee5e2cde/src/http/ngx_http_request_body.c#L833

PR for the tusd change: https://github.com/tus/tusd/pull/377

Acconut commented 4 years ago

Thank you very much for the links and the PR!