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

Optional Idle Timeout #1765

Merged
merged 3 commits into from
Sep 19, 2018
Merged

Optional Idle Timeout #1765

merged 3 commits into from
Sep 19, 2018

Conversation

nibanks
Copy link
Member

@nibanks nibanks commented Sep 18, 2018

Idle timeout is currently the last required transport parameter. This makes the parameter optional, defaulting to 0 or disabled if not present. It also removes the 10 minute maximum value. For other non-http protocols over QUIC, a 10 minute max doesn't really make sense.

@kazuho
Copy link
Member

kazuho commented Sep 18, 2018

Would it make sense to use varint to allow timeouts longer than 18 hours (e.g. 1 day)?


: The idle timeout is a value in seconds that is encoded as an unsigned 16-bit
integer. There is no maximum value besides the maximum encodable value of
0xFFFF (about 18 hours). If this parameter is abset or zero then the idle
Copy link
Member

Choose a reason for hiding this comment

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

"abset" -> "absent"

@marten-seemann
Copy link
Contributor

@kazuho: I suggested that in #1608.

@nibanks
Copy link
Member Author

nibanks commented Sep 18, 2018

Personally, I'd prefer not to use variable length integers here, but either way, I think we should leave that discussion to #1608 and not make that change as part of this PR. If you want an explicit idle timeout larger than 18 hours, then we should make this a 4-byte value instead of 2.

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 really like that we no longer have any mandatory transport parameters.

@@ -1666,6 +1654,18 @@ ack_delay_exponent (0x0007):
value is also used for ACK frames that are sent in Initial and Handshake
packets. Values above 20 are invalid.

initial_max_uni_streams (0x0008):
Copy link
Member

Choose a reason for hiding this comment

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

Please don't move this. The numbers are all out of order, but I do hope to fix that eventually.

A connection that remains idle for longer than the advertised idle timeout (see
{{transport-parameter-definitions}}) is closed. A connection enters the
draining state when the idle timeout expires.
If the idle timeout is enabled, a connection that remains idle for longer than
Copy link
Member

Choose a reason for hiding this comment

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

This change isn't needed. There is always an idle timeout, it's just that it is sometimes infinite.

Copy link
Member Author

Choose a reason for hiding this comment

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

We didn't quite state it that way above. It says the idle timeout is disabled, not infinite.

server.
: The idle timeout is a value in seconds that is encoded as an unsigned 16-bit
integer. There is no maximum value besides the maximum encodable value of
0xFFFF (about 18 hours). If this parameter is absent or zero then the idle
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't bother with the maximum sentence thing.

integer. There is no maximum value besides the maximum encodable value of
0xFFFF (about 18 hours). If this parameter is absent or zero then the idle
timeout is disabled. In this case, the transport never closes the connection
in response to being idle.
Copy link
Member

Choose a reason for hiding this comment

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

drop this last sentence too

@martinthomson martinthomson merged commit fbac6ec into quicwg:master Sep 19, 2018
@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Sep 19, 2018
@martinthomson
Copy link
Member

Thanks

@nibanks nibanks deleted the pr/optional-idle-timeout branch September 19, 2018 14:48
dtikhonov pushed a commit to dtikhonov/base-drafts that referenced this pull request Oct 17, 2018
Ns 1520 and 1521 refer to artefacts in lucas-clemente/quic-go GitHub
repository.
martinthomson added a commit that referenced this pull request Oct 17, 2018
…-15-changelog

Fix ID-15 changelog: idle timeout was made optional by PR #1765
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants