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

Explicitly suggest FIN or RST_STREAM #1893

Closed
wants to merge 5 commits into from
Closed

Explicitly suggest FIN or RST_STREAM #1893

wants to merge 5 commits into from

Conversation

martinduke
Copy link
Contributor

For issue #1878. I was apparently the only one confused, but since this is different from HTTP/2 it may be good to make it crystal clear.

For issue #1878. I was apparently the only one confused, but since this is different from HTTP/2 it may be good to make it crystal clear.
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Is it worth commenting that the receiver of the RST_STREAM frame MUST eventually send a FIN or a RST_STREAM in order for the stream to close completely?

@marten-seemann
Copy link
Contributor

@RyanatGoogle That applies to every stream, not just streams that a RST_STREAM was received for, doesn't it?

@RyanTheOptimist
Copy link
Contributor

@marten-seemann Yes, definitely. But I understood that the point of this CL was to help clarify the QUIC RST_STREAM behavior which is different from HTTP/2. Since the proposed text says, SHOULD, I wondered if some reader might not realize that eventually they MUST. But I'm happy with this PR as is if nobody else is concerned :)

@kazuho
Copy link
Member

kazuho commented Oct 19, 2018

I agree with @RyanatGoogle that the proposed text (quoted below) is confusing.

It will often be the case that upon receipt of a RST_STREAM on a bidirectional stream, an endpoint will choose to stop sending data to the peer on that stream. If so, it SHOULD send a FIN or RST_STREAM for that stream to close the peer's receive stream state.

My view is that the text can be interpreted as allowing an endpoint to stop sending data on that stream without sending FIN or RST_STREAM.

I think that it would be beneficial to either avoid use of SHOULD (by for example replacing it to "needs to"), or change it to MUST.

The reviews expired a rewrite. I think it's even clearer now, and avoids some of the bad inferences of the previous version.
@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels Oct 21, 2018
@martinthomson
Copy link
Member

Merged manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants