-
Notifications
You must be signed in to change notification settings - Fork 529
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
Don't send client CONNECTION_CLOSE on 1-RTT before handshake completion. #4145
Conversation
@@ -885,13 +894,11 @@ QuicTestCustomServerCertificateValidation( | |||
} | |||
TEST_EQUAL(AcceptCert, Client.GetIsConnected()); | |||
|
|||
if (AcceptCert) { // Server will be deleted on reject case, so can't validate. |
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 see how the server would get deleted. The new connection instance is held by the unique_ptr above and neither the AutoDelete bit nor ShutdownEventCallback is set, so the connection should be valid until the unique_ptr goes out of scope.
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 believe this was meant to handle a case where the listener could reject the connection, and then this pointer would never be set. That being said, I cannot see a path where that would happen in this test case (or the one below). And since the tests seem to pass just fine, I'm ok with these changes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4145 +/- ##
===========================================
- Coverage 87.38% 65.02% -22.36%
===========================================
Files 56 56
Lines 16984 15189 -1795
===========================================
- Hits 14841 9877 -4964
- Misses 2143 5312 +3169 ☔ View full report in Codecov by Sentry. |
@rzikim would it be possible to add (or modify) a test to validate #4132 is actually fixed? |
That is handled by the tests modified in this PR, without the fix in packet_builder.c, the async/reject case would've failed. This was not detected before because the test did not wait for server shutdown notification which would've timed out. |
Ah! I see. Ok, then let's ship it! |
Description
Fixes #4132.
Testing
Fixed tests so that they would detect the bug before the fix.
Documentation
No.