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.

Sign up for GitHub

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

Jump to bottom

Rework idle timeout definition #2346

Merged
merged 3 commits into from Jan 22, 2019
Merged

Rework idle timeout definition #2346

merged 3 commits into from Jan 22, 2019

Conversation

martinthomson
Copy link
Member

Closes #2149.

@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels Jan 18, 2019
dtikhonov
dtikhonov reviewed Jan 18, 2019
View reviewed changes
draft-ietf-quic-transport.md Outdated
not been acknowledged or declared lost. Packets that contain only ACK or
PADDING frames are not acknowledged until an endpoint has other frames to send,
so they could prevent the timeout from being refreshed.
Each endpoint advertises its own idle timeout to its peer. Endpoints restart
Copy link
Member

Choose a reason for hiding this comment

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

I'd switch to the singular number:

An enpdpoint restarts any timer it maintains when a packet from its peer....

As it stands, it sounds as if several endpoints had a single peer.

ianswett
ianswett approved these changes Jan 19, 2019
View reviewed changes
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.

I think this looks good, but @RyanatGoogle has more familiarity with this code than I do, so I'm adding him as well.

@martinthomson martinthomson merged commit 552985f into master Jan 22, 2019
@martinthomson martinthomson deleted the idle-restart-time branch January 22, 2019 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtikhonov dtikhonov dtikhonov left review comments

@ianswett ianswett ianswett approved these changes

@RyanTheOptimist RyanTheOptimist Awaiting requested review from RyanTheOptimist

Assignees
No one assigned
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@martinthomson @dtikhonov @ianswett