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

Tweaks for the draining period #870

Merged
merged 3 commits into from Oct 25, 2017
Merged

Conversation

martinthomson
Copy link
Member

This makes the 3RTO timer advisory (SHOULD).

It also adds a note about leaving early. If you can be sure that the other
side isn't going to send you more packets indefinitely, then you can end early
if you are OK with receiving reordered or delayed packets.

I also removed a residual mention of sending ACK frames in this state.

Closes #869.

This makes the 3RTO timer advisory (SHOULD).

It also adds a note about leaving early.  If you can be sure that the other
side isn't going to send you more packets indefinitely, then you can end early
if you are OK with receiving reordered or delayed packets.

I also removed a residual mention of sending ACK frames in this state.
Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

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

LGTM

@ekr
Copy link
Collaborator

ekr commented Oct 13, 2017

This just seems like a hack for failing to acknowledge that there are times when draining is required and times when it's not. I'd like to see this whole section revisited rather than continuing to add epicycles.

@martinthomson
Copy link
Member Author

It sounds like you have higher-level concerns than this. If you could write them up in an issue, that would be appreciated.

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

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

a nit, but I'll approve, assuming that you'll address the comment.

disposing of connection state could result in delayed or reordered packets to be
handled poorly. For endpoints that have some alternative means to ensure that
that late arriving packets on the connection are discarded, such as closing the
UDP port, an abbreviated draining period can allow for faster resource recovery.
Copy link
Contributor

Choose a reason for hiding this comment

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

(i) Closing the "port" makes no sense, you probably meant to say "socket".
(ii) Closing the port doesn't ensure that they;re discarded -- this can result in an ICMP message being generated in response. Perhaps say something like "to ensure that late-arriving packets on the connection do not create QUIC state, such as by closing the UDP socket"

that late arriving packets on the connection are discarded, such as closing the
UDP port, an abbreviated draining period can allow for faster resource recovery.
Servers that retain an open port for accepting new connections SHOULD NOT exit
the draining period early.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is good.

@martinthomson martinthomson merged commit 1ca1af9 into master Oct 25, 2017
@martinthomson martinthomson deleted the draining-applicability branch October 25, 2017 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants