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

require the application to handle 0-RTT rejection #3066

Merged
merged 5 commits into from
Mar 11, 2021

Conversation

marten-seemann
Copy link
Member

@marten-seemann marten-seemann commented Mar 4, 2021

Fixes #2067. Depends on #3073.

Implementing the proposal made in this issue, without the wrapper for the easy / common case.

@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #3066 (f8313d8) into master (eb3e100) will decrease coverage by 0.19%.
The diff coverage is 68.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3066      +/-   ##
==========================================
- Coverage   86.04%   85.85%   -0.19%     
==========================================
  Files         132      132              
  Lines        9435     9513      +78     
==========================================
+ Hits         8118     8167      +49     
- Misses        955      981      +26     
- Partials      362      365       +3     
Impacted Files Coverage Δ
internal/ackhandler/sent_packet_handler.go 77.69% <ø> (-0.17%) ⬇️
session.go 75.92% <0.00%> (-0.68%) ⬇️
internal/flowcontrol/connection_flow_controller.go 95.35% <71.43%> (-4.65%) ⬇️
streams_map.go 84.97% <72.73%> (-8.37%) ⬇️
framer.go 97.50% <88.24%> (-2.50%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb3e100...f8313d8. Read the comment docs.

@marten-seemann marten-seemann force-pushed the zero-rtt-rejection branch 5 times, most recently from 0afe8b8 to a16e170 Compare March 8, 2021 04:00
framer.go Outdated Show resolved Hide resolved
internal/flowcontrol/connection_flow_controller.go Outdated Show resolved Hide resolved
streams_map.go Outdated
@@ -53,6 +58,8 @@ type streamsMap struct {
outgoingUniStreams *outgoingUniStreamsMap
incomingBidiStreams *incomingBidiStreamsMap
incomingUniStreams *incomingUniStreamsMap

resetErr utils.AtomicError // can be nil, otherwise must be an error
Copy link
Member

Choose a reason for hiding this comment

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

I find it hard to understand which parts of the streamsMap are expected to be thread safe, probably some comments would help.

That said, this looks weird to me: The error is set from ResetFor0RTT and read from the Open* / Accept* fns. So those are called from different threads? But then, they read the maps, which are set from initMaps, called from ResetFor0RTT? Why does the error need to be protected, but not those maps?

Copy link
Member Author

Choose a reason for hiding this comment

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

Open and Accept is called by the application. When 0-RTT is reset, these calls should return Err0RTTRejected, until the application calls NextSession.
initMaps is called after the error is set, so we don't need introduce a mutex for Open and Accept. Note that ResetFor0RTT also closes the maps, which makes them return an Err0RTTRejected if Open or Accept is called concurrently.

Copy link
Member

@lucas-clemente lucas-clemente Mar 9, 2021

Choose a reason for hiding this comment

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

So ResetFor0RTT and Open* are called from different threads? Then I don't think this is safe. Imagine the thread calling Open* pauses after the err != nil if clause, then ResetFor0RTT is called, starts setting the new map, gets interrupted and the first thread resumes – concurrent access to the map without protection?

@marten-seemann
Copy link
Member Author

@lucas-clemente I introduced a mutex in the streams_map. PTAL.

@marten-seemann marten-seemann merged commit b2c2e49 into master Mar 11, 2021
@marten-seemann marten-seemann deleted the zero-rtt-rejection branch March 11, 2021 13:15
@aschmahmann aschmahmann mentioned this pull request May 14, 2021
71 tasks
@kaiyuhou
Copy link

kaiyuhou commented Aug 20, 2021

Hi, I noticed the current http3 client would not handle the "0-RTT rejected" correctly. The http3 client will only raise the error to the user if it received this error and will not resend the request.

Or do I have the wrong expectation for the http3 client? Should It be defined as an application? If it shouldn't be considered as an application, as an end-user when I received the "0-RTT rejected" error from the http3 client, should I manually call the "Do" function again?

In this case, will the first data packet be the "1-RTT" case, or actually the "2-RTT" case?

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.

invalidate sent data when 0-RTT is rejected
3 participants