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
add tls.ClientHelloInfo.Conn for recursive GetConfigForClient calls #4016
Conversation
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.
@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.
280054c
to
3970c33
Compare
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 Report
@@ 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
|
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.
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.
We have another confirmation of the fix. 👍 |
Thanks for the review @mholt and thank you @francislavoie and everyone else involved in testing! |
See caddyserver/caddy#5680 (comment).