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

closed streams #145

Closed
wants to merge 4 commits into from
Closed

Conversation

marten-seemann
Copy link
Contributor

I added two commits regarding the closing of streams.

The current version of the draft already states that a peer that received stream data beyond the final offset (received either in STREAM frame with the FIN flag set or in a RST_STREAM frame) MUST close the connection with a QUIC_STREAM_DATA_AFTER_TERMINATION error.
However, due to packet reordering, the packet carrying the final offset might be delayed, such that the peer first receives STREAM frames up to a certain offset, and then receives the final offset, and notices that the final offset is smaller than the offset it received before. In this case, it MUST also terminate the connection with a QUIC_STREAM_DATA_AFTER_TERMINATION error.

The second commit is concerned with what happens when a RST_STREAM is received. A common use case is the following: A client sends a POST request to a server and already begins sending data, but the Request URI doesn't accept data (e.g. there's an error 403 or 404). In those cases, the server will send a HTTP response (note that a 404 has a non-empty response body), and immediately after sending the last byte of the 404 response, it will send a RST_STREAM in order to stop the client from sending more data. In this case, QUIC needs to make sure the data sent by the server is delivered to the client (i.e. the server MUST retransmit any lost data, even after receiving the RST_STREAM response from the client), and the client MUST deliver the data received to the application, even if the packets containing the response body are delayed such that they arrive after the RST_STREAM sent by the server.

@marten-seemann
Copy link
Contributor Author

marten-seemann commented Jan 13, 2017

I realize there are cases where a retransmission of STREAM frames might not be necessary. In my example above only the stream data sent by the server needs to be reliably transmitted, but the stream data sent by the client is not read anyway and doesn't need retransmissions.
Is this maybe what the TODO at the end of the section is referring to? This might then be related to #19.

discard the frame, with one exception. If a STREAM frame carrying data beyond
the received final offset is received, the endpoint MUST close the connection
with a QUIC_STREAM_DATA_AFTER_TERMINATION error ({{error-handling}}).
An endpoint that receives a FIN flag or a RST_STREAM frames knows the final
Copy link
Member

Choose a reason for hiding this comment

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

"frame"

QUIC_STREAM_DATA_AFTER_TERMINATION error ({{error-handling}}).

An endpoint MUST close the connection with a QUIC_STREAM_DATA_AFTER_TERMINATION
error if it receives a RST_STREAM or a FIN flag with a lower final offset than
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use "RST_STREAM frame" like above?

the received final offset is received, the endpoint MUST close the connection
with a QUIC_STREAM_DATA_AFTER_TERMINATION error ({{error-handling}}).
An endpoint that receives a FIN flag or a RST_STREAM frames knows the final
offset for this stream. If a STREAM frame carrying data beyond the received
Copy link
Member

Choose a reason for hiding this comment

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

Personally (and YMMV) I find the new wording here a bit confusing, and I'd keep the previous one for this case (with your addition in the next paragraph).

@martinthomson
Copy link
Member

@marten-seemann, I think that the first change here is non-controversial. However, I'm concerned that the second is a fairly signficant change to the stream state machine. Can I request that you open a new pull request for that?

@marten-seemann
Copy link
Contributor Author

@martinthomson I reverted the controversial commit and fixed a few typos pointed out by @ghedo.

I still think that changes to the stream state machine are necessary (in one way or the other, but at the moment the behavior is not clearly defined). I'll prepare another PR for that.

@martinthomson
Copy link
Member

I agree with the need for changes to the state machine, but it's a fairly big change. We might want to consider other, more drastic changes here.

@mirjak
Copy link
Contributor

mirjak commented Jan 16, 2017

(I agree that resets and other control information should be re transmitted as quickly as possible. However, data frames could be retransmitted based on their frame priority.) Sorry typed this in the wrong window -> belongs to issue 114

@mirjak
Copy link
Contributor

mirjak commented Jan 16, 2017

I'm not sure that if the server sends a 404 that this automatically means it must close the stream. 404 is application logic and given you received the 404 message correctly the server transport connection works. You should just close the stream regularly or even the whole connection.

@marten-seemann
Copy link
Contributor Author

@mirjak: I already reverted the corresponding commit, since there are further changes needed to handle the resetting of streams correctly. For sure, some kind of stream resetting is needed in the case of a POST to a 404 (in order to keep the flow controllers synchronized). I'll soon open a new PR for that, but I'm still figuring out the details.

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.

I'm going to open a new PR for this one and I'll ask for another review, since it turned out to be a fairly big set of changes.


An endpoint MUST close the connection with a QUIC_STREAM_DATA_AFTER_TERMINATION
error if it receives a RST_STREAM frame or a FIN flag with a lower final offset
than the highest data offset it already received on that stream.
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified greatly:

If a STREAM frame carries data with an offset higher than the final offset, {{ERROR}}. If a RST_STREAM or a STREAM frame marked with a FIN flag indicates a different final offset, {{ERROR}}.

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

Successfully merging this pull request may close these issues.

4 participants