Interaction of FIN and message parsing #2003
Conversation
MikeBishop
added
design
|
Maybe I'm not thinking about this the right way, but if a body does not have a content-length, then how is the end of the message determined, if not by the end of the stream? |
|
That's true, and should have been obvious. Ugh.
|
draft-ietf-quic-http.md
Outdated
|
|
||
| Changes to the state of a request stream do not directly affect message | ||
| processing. Endpoints MUST process complete HTTP messages without relying on | ||
| stream closure as an end-of-message signal. For example, servers do not abort a |
There was a problem hiding this comment.
This is wrong. The statement to make is:
The end of a stream (in either direction) indicates the end of a message.
Incomplete messages can be processed, and ideally endpoints do that after receiving a complete header block.
|
Updated with Martin's suggestion: End of stream data is end of message, but you SHOULD be consuming partial messages without waiting for the end. End of stream with a partial message is an error. |
Co-Authored-By: MikeBishop <[email protected]>
MikeBishop
changed the title
|
@RyanatGoogle and @martinduke, I'd ideally like your input before I merge this. |
|
Thanks, this addresses my concern. I don't see how the client MUST has any force given that there's no time limit; perhaps it ought to be a SHOULD? |
draft-ietf-quic-http.md
Outdated
| corresponding QUIC stream is closed in the appropriate direction. | ||
| After sending a request, a client MUST close the stream for sending; after | ||
| sending a final response, the server MUST close the stream for sending. At | ||
| this point, the QUIC stream is fully closed. |
There was a problem hiding this comment.
To @martinduke's comment, maybe it is enough to say "A message is terminated when the stream is closed." Instead of all these words.
There was a problem hiding this comment.
Oh, and now I realize that this is what it said before... Not sure whether this is an improvement then.
There was a problem hiding this comment.
Yeah, the original provoking issue was that he didn't FIN, the server didn't respond, and both are doing totally spec-compliant things. With this language, it's at least clarified that while the server SHOULD process that request on receipt, it isn't required to until the stream closes, so the client needs to close promptly.
|
You're right that a MUST with no deadline is somewhat toothless, but I don't think it's SHOULD-level. (If we were to fully expand it, you MUST close it eventually and SHOULD do so promptly.) I think the force is actually in the server's SHOULD: While the server SHOULD process messages before you FIN, it's not obligated to. Therefore, if you want to ensure a response, you'll need to close promptly as a practical matter. |
|
It sounds like there's a blanket rule except for one special flower - HTTP_CONNECT. So perhaps we should just call out the exception rather than dancing around it, e.g. "With the exception of requests that contain the CONNECT method, senders use a FIN to indicate the end of an HTTP request or response. Absent CONNECT, senders MUST NOT make the sending of FIN dependent on receipt of a response from the peer. Absent CONNECT, senders MUST NOT send additional data on the stream after the request or response. However, due to the possible presence of a CONNECT method, and the opportunity to send HTTP_EARLY_DATA, servers SHOULD begin processing partial HTTP messages once enough of the message has been received to make progress. If a client stream terminates without enough of the HTTP message to provide a complete response, the server SHOULD abort its response with the error code HTTP_INCOMPLETE_REQUEST." This MUST strikes me as a testable condition. |
|
Formally, the data transferred on the TCP connection is the body of the HTTP request, so it's not a special flower in that sense. But it's special in that it's the only case where the server MUST process the headers before the end of the "message." The requirement that the client MUST NOT make FIN dependent on receiving anything would be a good wording improvement. |
|
Alright, how about this: "Senders use a FIN to indicate the end of an HTTP request or response. Absent a CONNECT method in the request, senders MUST NOT make the sending of FIN dependent on receipt of a response from the peer. However, due to the possible presence of a CONNECT method, and the opportunity to send HTTP_EARLY_DATA, servers SHOULD begin processing partial HTTP messages once enough of the message has been received to make progress. If a client stream terminates without enough of the HTTP message to provide a complete response, the server SHOULD abort its response with the error code HTTP_INCOMPLETE_REQUEST." I'm open to making the server requirement a similar MUST -- "servers MUST NOT be dependent on a FIN to start processing..." if people think that would help avoid CONNECT deadlocks. |
|
Not seeing how early data (or HTTP_EARLY_DATA, whatever that is) has any bearing on this. I would drop the first particle from the second paragraph and concentrate on the recommendations: process when you can, use HTTP_INCOMPLETE_REQUEST if you need to. I don't think we need a MUST attached to the recommendations - if you don't follow the recommendations, then it's your loss. |
Argh, I meant HTTP_EARLY_RESPONSE. As for the final wording, I am happy with whatever you guys decide. |
|
Did something along those lines -- see what you think. |
draft-ietf-quic-http.md
Outdated
| corresponding QUIC stream is closed in the appropriate direction. | ||
| After sending a request, a client MUST close the stream for sending; clients | ||
| MUST NOT make stream closure dependent on receiving a response to their request, | ||
| unless using the CONNECT method (see {{the-connect-method}}). After sending a |
There was a problem hiding this comment.
This sentence is confusing to me, because it is unclear what "unless" applies to.
How about changing the sentence to: Unless using the CONNECT method (see {{the-connect-method}}), a client MUST close the stream for sending after sending a request. Clients MUST NOT make stream closure dependent on receiving a response to the request.
There was a problem hiding this comment.
And this highlights the confusion, because I meant the "unless" to be on the opposite clause. :-)
With the formulation we've already got, the entire CONNECT stream is one large HTTP request message, so it doesn't make sense to say you don't need to close a stream after the request in that case. However, stream closure most definitely depends on the response you get back to the headers of your CONNECT request.
There was a problem hiding this comment.
I like @kazuho's working slightly better, but aside from that, ship it!
Fixes #1972.
While the discussion on the issue wasn't conclusive in either direction, I think the presence of CONNECT and early response means servers will need to be processing HTTP messages as they arrive, rather than waiting for the FIN as an end-of-message signal. Also, if the message is large, the alternative risks deadlock on a large POST.
This PR expands on the "stream state change isn't part of the message framing" language to clarify that both peers MUST be able to process complete HTTP messages regardless of whether the stream has been closed or not.