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

100-continue question for Creation with Upload #151

Open
jade2k11598 opened this issue Mar 30, 2020 · 5 comments
Open

100-continue question for Creation with Upload #151

jade2k11598 opened this issue Mar 30, 2020 · 5 comments

Comments

@jade2k11598
Copy link

So in the discussion of Creation With Upload, in the example provided, the POST request seems to be missing the Expect: 100-continue as noted in the paragraph above the example. Further the response seems not quite consistent with the 100-continue use, as laid out by rfc2616-sec8, sec 8.2.3, where it states concerning the server's response:

Upon receiving a request which includes an Expect request-header field with the "100-continue" expectation, an origin server MUST either respond with 100 (Continue) status and continue to read from the input stream, or respond with a final status code. The origin server MUST NOT wait for the request body before sending the 100 (Continue) response.

So going back to the example given in Creation With Upload,, the response was this:

HTTP/1.1 201 Created
Location: https://tus.example.org/files/24e533e02ec3bc40c387f1a0e460e216
Tus-Resumable: 1.0.0
Upload-Offset: 5

To be consistent to the quoted paragraph above, the response should not include the upload-offset because the server hasn't yet read the body, since the protocol is that it must first reply to the request before attempting to read the body and that response is suppose to just contain the 100 (Continue) status, no?

@Acconut
Copy link
Member

Acconut commented Mar 30, 2020

Good point. The Creation With Upload extension does not require the use of Expect: 100-continue, it only recommends the use of it. So the example is not incorrect by not using this header. In particular, the response is correct as no 100 Continue response is necessary in this case. Furthermore, I believe it makes the example a lot more readable by leaving that part out. What do you think?

@jade2k11598
Copy link
Author

I'm not sure I'm following you. In the paragraph of Creation With Upload, it explicitly states:

In addition, the Client SHOULD include the Expect: 100-continue header in the request to receive early feedback from the Server on whether it will accept the creation request, before attempting to transfer the first chunk.

Based on that paragraph, if one wants to use the Creation with Upload, then they do need to include the Expect: 100-continue in the POST request. The request example is not consistent to that paragraph above (e.g. it's missing the Expect: 100-continue).

It also notes that the client should get a response from the server on whether it accepts the creation request before attempting to transfer the first chunk, which is consistent to the description in sec 8.2.3, of rfc2616-sec8. So the response of the server is also not consistent to the quoted paragraph above. Because it wouldn't have yet received the first chunk when it sends the response. It has to first send the response to the client before the client sends the first chunk. So having an Upload-Offset in that response just wouldn't make sense if the server hasn't yet received that chunk from the client.

Further, although the section on Creation With Upload does not mention that the response sent by the server has to have the 100-continue header in its response, because the response is a status on the 100-continue, it seems prudent to have it in its response.

I think this section on Creation With Upload is missing a bit more on the sequence of events, in that as noted by sec 8.2.3, of rfc2616-sec8:

An origin server that sends a 100 (Continue) response MUST ultimately send a final status code, once the request body is received and processed, unless it terminates the transport connection prematurely.

For completeness, the section on Creation With Upload should provide the whole sequence of events that involves this interaction. A sequence diagram would be very helpful.

@Acconut
Copy link
Member

Acconut commented Mar 30, 2020

Ok, I think there is a confusion in your side:

It also notes that the client should get a response from the server on whether it accepts the creation request before attempting to transfer the first chunk, which is consistent to the description in sec 8.2.3, of rfc2616-sec8. So the response of the server is also not consistent to the quoted paragraph above.

If the tus spec says "the Client SHOULD verify that it is supported by the Server" we are referring to sending an OPTIONS request and checking the returned Tus-Extensions header before sending the POST request. This precheck is completely unrelated to 100-continue, so I think you are confusing something there.

For completeness, the section on Creation With Upload should provide the whole sequence of events that involves this interaction. A sequence diagram would be very helpful.

I agree. Although the spec does not require the use, it's always helpful to add it. Would you be willing to open a PR for this?

@jade2k11598
Copy link
Author

If the tus spec says "the Client SHOULD verify that it is supported by the Server" we are referring to sending an OPTIONS request and checking the returned Tus-Extensions header before sending the POST request. This precheck is completely unrelated to 100-continue, so I think you are confusing something there.

I'm not suggesting the precheck is related to the 100-continue. The sentence your quoting, yes, that refers to checking on what the server supports by sending an OPTIONS request, but that's not the sentence I quoted.

For clarity, let me quote the whole paragraph in question:

If the Client wants to use this extension, the Client SHOULD verify that it is supported by the Server before sending the POST request. In addition, the Client SHOULD include the Expect: 100-continue header in the request to receive early feedback from the Server on whether it will accept the creation request, before attempting to transfer the first chunk.

Yes, the first sentence does refer to the OPTIONS request, to check if the the server has thecreation-with-upload listed in its Tus-Extension.

But the 2nd sentence of the rest of that paragraph (which is the one I had quoted in my previous post), refers to the POST request, since it talks about the client waiting for the server's response "before attempting to transfer the first chunk" . And it is in that 2nd sentence that notes that the Client SHOULD include the Expect: 100-continue header in the request --- that is in reference to the POST request.

Further, we also know the 2nd sentence doesn't refer to an OPTIONS request, because an OPTIONS request never includes a Expect: 100-continue (that only ever makes sense in the context of a POST request as as noted in sec 8.2.3, of rfc2616-sec8).

If that quoted paragraph is confusing, then maybe it should be broken down to two paragraphs. But then again, I never interpreted the precheck (first sentence of that paragraph) to be related to the rest of that paragraph (where it speaks about where the100-continue should be included).

So going back to my original point, based on that 2nd sentence, this is what I'm pointing out:

  1. The POST request example shown in section the example of Creation With Upload, is missing this Expect: 100-continue.
  2. In the same example referred to in 1), the server' response should not include the Upload-Offset, since the 2nd sentence implies that the client has to wait for the server's response before sending the first chunk (e.g. payload). The Upload-Offset should be in the final response of the server after it receives that payload (and of which I quoted earlier from sec 8.2.3, of rfc2616-sec8).

And yes, I would create a PR to include a sequence diagram, since I think there's a bit more interaction going on with Creation with Upload than what is shown in this document, and as suggested by sec 8.2.3, of rfc2616-sec8, but it looks like I lack permissions to create a PR ... that is the PR button creation is greyed out for me (or can you refer me to the correct place to create it?).

@Acconut
Copy link
Member

Acconut commented Mar 30, 2020

Thank you very much for the clarification. I think I figured out what our differences were and I agree with you.

You can start a new PR by editing the protocol.md file directly at https://github.com/tus/tus-resumable-upload-protocol/edit/master/protocol.md. Let me know if that does not work!

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

No branches or pull requests

2 participants