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 RST-like behavior for CONNECTION_CLOSE. Fixes #330, #328 #705

Merged
merged 4 commits into from
Jul 28, 2017

Conversation

ekr
Copy link
Collaborator

@ekr ekr commented Jul 27, 2017

No description provided.

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me.

@siyengar
Copy link

lgtm, maybe could use some guidance on how exactly the server could implement such a mechanism, however we can always add that later once we have more implementation experience.

@mikkelfj
Copy link
Contributor

mikkelfj commented Jul 27, 2017

This is not entirely clear to me. If there is a requirement for backing off, but no requirement for replying to a connection close (is there?), then state will just be tied up for no good reason. If you have 100K connections per minute, and you need to hold a connection for 10 minutes to make sure you can back off, well ...

@ekr
Copy link
Collaborator Author

ekr commented Jul 27, 2017

See my email on this topic to the list where I lay out the tradeoffs.

implementations MUST NOT send any non-closing packets on that connection. If
additional packets are received after this time, implementations
SHOULD respond to them by sending either a CONNECTION_CLOSE frame or a
Public Reset packet, either of which may just be a duplicate of a
Copy link
Member

Choose a reason for hiding this comment

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

I recommend that you use CONNECTION_CLOSE here and instead note that an endpoint MAY send a Public Reset instead if it doesn't want to save the state necessary to generate CONNECTION_CLOSE.

There's a sliding scale here:

  1. maintain full state and generate a new CONNECTION_CLOSE in a new packet
  2. only keep the packet that holds CONNECTION_CLOSE and retransmit that unmodified
  3. drop state and rely on Public/Stateless Reset

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm happy to revise this if that's the consensus. I was trying to be flexible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that 2 and 3 are a sliding scale and that's why SHOULD makes sense to me. I haven't come up with any reasons why option 1 is preferable to 2, so I would not suggest it, but maybe there are some reasons I haven't come up with?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Ian that we don't need the endpoint to maintain complete state, so 2 and 3 make sense. I'd suggest that we add a suggestion about how long a peer resends CONNECTION_CLOSE after sending it once (basically how long state should be kept in option (2)), and I propose idle_timeout as a time period that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

I think that we should allow all three, though gently discourage 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the argument for allowing 1?

Public Reset packet, either of which may just be a duplicate of a
previous packet. Implementations SHOULD throttle these responses, for
instance by exponentially backing off the number of packets which must
be received before sending a 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/must be/are

@janaiyengar
Copy link
Contributor

lgtm. I have a non-blocking suggestion in my inline comment.

@ekr
Copy link
Collaborator Author

ekr commented Jul 28, 2017

How's this look?

@janaiyengar
Copy link
Contributor

LGTM. Not a major issue, but the line breaks are at odd places. If you are able to, please format them to 80 cols.

@janaiyengar
Copy link
Contributor

I'd like to merge this. martinthomson@, if you think we should make the text more accommodating, let's do that separately.

@janaiyengar janaiyengar merged commit 72db13a into quicwg:master Jul 28, 2017
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.

6 participants