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

Specify behavior on incomplete requests #1643

Merged
merged 6 commits into from Aug 14, 2018
Merged

Conversation

MikeBishop
Copy link
Contributor

Fixes #1596 (and an oversight I caught in passing) by specifying server behavior if it reaches the end of readable data on a stream and hasn't parsed a full HTTP request yet. There are a couple cases here, and I think this generically covers all the interesting ones.

  • Client sends FIN early. Server sees truncated request, but non-error end of stream.
  • Client sends RST_STREAM part-way through request. Server sees truncated request with read error.
  • Client sends a RST_STREAM (but not a STOP_SENDING) after sending a fully-formed request.
    • ...and packet loss occurred. No retransmissions, so same as sending it part-way through request.
    • ...after the server has read the request and generated / begun generating a response. The server probably never realizes and responds merrily. If you didn't want it, you should have sent a STOP_SENDING.
    • ...but before the server has read all the data out of the buffer. Depends on implementation; transport might discard unread data or deliver all the data anyway. Either way, this is identical to one of the two previous cases.

We already discuss cancelling requests, so I added a suggestion that clients go ahead and include a STOP_SENDING if they reset their request stream.

@MikeBishop MikeBishop added design An issue that affects the design of the protocol; resolution requires consensus. -http labels Aug 9, 2018
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something about the special rules here seems just wrong. I have an alternative approach for this that I think is neater.

discard responses at their discretion for other reasons.

Servers MUST NOT abort a response in progress as a result of receiving a
solicited RST_STREAM (i.e. with error code STOPPING). If the request stream
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing comma after "i.e."

abort a response in progress as a result of receiving a solicited RST_STREAM.
discard responses at their discretion for other reasons.

Servers MUST NOT abort a response in progress as a result of receiving a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the way to do this is to say:

Cancellation of a request stream does not prevent a server from sending a response. A client that does not want a response SHOULD send a STOP_SENDING frame. A server does not need to take receipt of a RST_STREAM as a signal that it cannot provide a response, though it might be unable to do so. If a stream is reset by the client and the server is unable to respond as a result, it can treat that as a stream error of type HTTP_INCOMPLETE_REQUEST.

This is a different structure - it says that any STOP_SENDING and RST_STREAM on the request side of a stream only cancel the request and not the response. But I think that this is neater - it avoids all of the coupling you have going on here.

Copy link
Contributor Author

@MikeBishop MikeBishop Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's basically what I was going for (the error at the server is that it doesn't have a complete request, not that the RST occurred), but that's a clearer way to state it.

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is good. Mostly just nits. The one of fully-formed might be a deal-breaker if you don't fix it.

(regardless of error code), do not affect the state of the server's response.
Servers MUST NOT abort a response in progress solely due to a state change on
the request stream. However, if the request stream terminates without containing
a fully-formed HTTP request, the server SHOULD abort its response with the error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/fully-formed/usable/ perhaps.

A server can often produce a response based purely on header fields. That's the case that we specifically want to support with STOP_SENDING (a 404 on a POST). This implies that the message needs to be complete, which isn't necessary.


Changes to the state of a request stream, including receiving a RST_STREAM
(regardless of error code), do not affect the state of the server's response.
Servers MUST NOT abort a response in progress solely due to a state change on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this is a normative requirement on reflection. Maybe just "Servers do not abort a response..." or "Servers are not expected to abort a response..."

discard responses at their discretion for other reasons.

Changes to the state of a request stream, including receiving a RST_STREAM
(regardless of error code), do not affect the state of the server's response.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/(regardless of error code)/with any error code/

@MikeBishop MikeBishop merged commit 2ad9b81 into master Aug 14, 2018
@martinthomson martinthomson deleted the http/incomplete_request branch October 26, 2018 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-http design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants