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

save the RTT in non-0-RTT session tickets #4042

Merged
merged 10 commits into from
Sep 11, 2023

Conversation

tanghaowillow
Copy link
Contributor

@tanghaowillow tanghaowillow commented Aug 22, 2023

Save the RTT in non-0RTT session tickets.
Fixes #3853 for go 1.21.

@tanghaowillow tanghaowillow marked this pull request as draft August 22, 2023 06:05
@tanghaowillow tanghaowillow changed the title also send session ticket when Allow0RTT is disabled save the RTT in non-0RTT session tickets Aug 23, 2023
@tanghaowillow tanghaowillow marked this pull request as ready for review August 23, 2023 02:40
internal/handshake/session_ticket.go Outdated Show resolved Hide resolved
internal/handshake/session_ticket.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #4042 (b727b88) into master (2797f85) will increase coverage by 0.18%.
Report is 11 commits behind head on master.
The diff coverage is 58.54%.

@@            Coverage Diff             @@
##           master    #4042      +/-   ##
==========================================
+ Coverage   83.29%   83.47%   +0.18%     
==========================================
  Files         147      147              
  Lines       14908    14904       -4     
==========================================
+ Hits        12417    12441      +24     
+ Misses       1990     1967      -23     
+ Partials      501      496       -5     
Files Changed Coverage Δ
internal/qtls/go120.go 28.17% <0.00%> (ø)
internal/qtls/go121.go 56.84% <11.11%> (+1.74%) ⬆️
internal/handshake/crypto_setup.go 57.73% <73.33%> (+0.55%) ⬆️
internal/handshake/session_ticket.go 100.00% <100.00%> (ø)

... and 12 files with indirect coverage changes

@marten-seemann marten-seemann changed the title save the RTT in non-0RTT session tickets save the RTT in non-0-RTT session tickets Aug 26, 2023
}
t.Parameters = &tp
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a check here that the entire ticket has been consumed (r.Len() == 0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think maybe it is only needed for unmarshaling non-0-RTT session ticket. A 0-RTT session ticket is end with transport parameters and unmarshaling transport parameters from session ticket always consumes all bytes of the session ticket(extra bytes would either be ignored as unknwon parameter IDs or cause an EOF error).

internal/handshake/session_ticket.go Show resolved Hide resolved
internal/handshake/crypto_setup_test.go Show resolved Hide resolved
Copy link
Member

@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.

Looks good, just one small nit, and two suggestions how to improve the tests.

internal/handshake/session_ticket.go Outdated Show resolved Hide resolved
internal/handshake/session_ticket_test.go Show resolved Hide resolved
internal/handshake/session_ticket_test.go Show resolved Hide resolved
@marten-seemann
Copy link
Member

Thanks for all your work @tanghaowillow!

@marten-seemann marten-seemann merged commit d1f6ea9 into quic-go:master Sep 11, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

save the RTT with the session ticket, for non-0-RTT tickets
2 participants