Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Change idle_timeout to max_idle_timeout #3099
Change idle_timeout to max_idle_timeout #3099
Changes from 11 commits
61a3725
dc5b220
2537a1a
61dcaad
6fd0ac8
e4dd819
2234623
11d8da8
508413d
e0d76ce
48f04d6
a7232f1
ec80c86
cd47d62
7a6d513
0bfcf11
210821b
07d63d9
e74856d
581a368
875de3f
774bc68
c559540
a203af6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this sentence... what does it mean for an endpoint to initiate an immediate close if it abandons the connection prior to the min? I thought that the endpoint was to initiate an immediate close at the min. Why are we talking about abandoning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write some rationale for these restarting rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also pre-existing text, so maybe in a different PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're going to explain why you restart when sending, you might need to explain why you don't restart when sending subsequent packets. Perhaps "when sending one or more packets" might help point this explanation at bursts, rather than at individual packets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also pre-existing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a great construction: "If a peer could time out within a Probe Timeout". It's also confusing ... maybe change this to "When an endpoint is about to send data that cannot be retried safely, it is possible that the peer may have reached the end of its idle timeout period before this data arrives at the peer. To avoid such situations, it is recommended that an endpoint test for liveness when it expects that the peer might be close, within a Probe Timeout (PTO; see Section ...) period for instance, to the end of its idle timeout period."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I'll file an issue to address your and Mike's comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the text should be fixed here... why introduce text here and then fix it in a new PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not introducing this text, it just moved, hence the different PR, which is editorial.