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

add tls.ClientHelloInfo.Conn for recursive GetConfigForClient calls #4016

Merged

Conversation

marten-seemann
Copy link
Member

Copy link
Contributor

@mholt mholt left a comment

Choose a reason for hiding this comment

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

@francislavoie was able to confirm that this fixes the reproducer he discovered, so that's good. Still awaiting further confirmation if anyone else in the original issue wants to try it out.

Thank you for working on this. 🙏

Looks like there may be a race condition as noted by the CI. Not clear to me, but it seems it may be specific to the integration test itself (the write to local2 on L154, but I'm not quite sure where the concurrent read is coming from, the stack trace just says ginkgo basically). So I dunno, maybe it's just an issue with the test case and not the actual production code.

Thank you again, sorry this one is so complicated.

@marten-seemann marten-seemann force-pushed the clienthelloinfo-recursive-getconfigforclient branch from 280054c to 3970c33 Compare August 4, 2023 17:39
@marten-seemann
Copy link
Member Author

I'm sorry for the trouble this is causing. But you're right, this is quite complex.

The race condition should be fixed now.

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #4016 (3970c33) into master (18d3846) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4016      +/-   ##
==========================================
+ Coverage   82.81%   82.91%   +0.10%     
==========================================
  Files         147      147              
  Lines       14764    14771       +7     
==========================================
+ Hits        12226    12247      +21     
+ Misses       2036     2024      -12     
+ Partials      502      500       -2     
Files Changed Coverage Δ
internal/handshake/crypto_setup.go 56.26% <100.00%> (+3.36%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Contributor

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Oh, no need to apologize -- you're going above and beyond to help with something that I should have been able to catch sooner in a no-stress way. :)

I'm waiting to get more confirmation that the issue is fixed (beyond Francis', which is probably reliable) but you can merge this whenever IMO.

@mholt
Copy link
Contributor

mholt commented Aug 4, 2023

We have another confirmation of the fix. 👍

@marten-seemann
Copy link
Member Author

Thanks for the review @mholt and thank you @francislavoie and everyone else involved in testing!

@marten-seemann marten-seemann merged commit 95ab7bd into master Aug 5, 2023
29 checks passed
@marten-seemann marten-seemann deleted the clienthelloinfo-recursive-getconfigforclient branch September 1, 2023 10:06
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.

None yet

2 participants