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

Repair rules for NEW_TOKEN #2961

Merged
merged 2 commits into from
Aug 19, 2019
Merged

Repair rules for NEW_TOKEN #2961

merged 2 commits into from
Aug 19, 2019

Conversation

martinthomson
Copy link
Member

Closes #2871.

@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels Aug 9, 2019
marten-seemann
marten-seemann approved these changes Aug 9, 2019
View reviewed changes
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

LGTM, but do we need some text saying that when receiving a NEW_TOKEN frame, you MUST make sure that it's not a duplicate? Otherwise, a client might just save the token twice, and effectively reuse the token.

@martinthomson
Copy link
Member Author

Is that not implied by the text here?

@marten-seemann
Copy link
Contributor

Kind of, but this text talks about the sender's perspective.
Furthermore, checking for duplicates is inherently racy if you're using tokens for new connections while the old connection is still alive.

@martinthomson
Copy link
Member Author

You know, I think that I put some proposed fixes for this in c016639 with rather than this branch. Moved it over here. @marten-seemann, you approved #2962, so I hope that moving that commit over here works for you.

@martinthomson martinthomson merged commit 86bca1d into master Aug 19, 2019
@martinthomson martinthomson deleted the retransmit-new-token branch August 19, 2019 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianswett ianswett ianswett approved these changes

@marten-seemann marten-seemann marten-seemann approved these changes

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.

Retransmission Instructions do not include NEW_TOKEN
3 participants
@martinthomson @marten-seemann @ianswett