-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Unit test to reproduce data race when reloading TLS config #6213
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: chahatsagarmain <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6213 +/- ##
===========================================
- Coverage 96.50% 48.76% -47.75%
===========================================
Files 354 179 -175
Lines 20127 10803 -9324
===========================================
- Hits 19424 5268 -14156
- Misses 520 5092 +4572
- Partials 183 443 +260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Signed-off-by: chahatsagarmain <[email protected]>
Have you observe this new test fail? It doesn't look like it's testing race condition deterministically. |
Signed-off-by: chahatsagarmain <[email protected]>
In the previous test, the race condition occurred because the mutex was not used. Adding the mutex resolved the issue. However, in the updated test, new TLS configurations are being applied and its seems to be causing data race during client connection and TLS handshake error . |
so I see this test is failing on detected race condition, but the stack traces indicate that the race is caused by the test itself (the Write part), not by the production code:
|
Signed-off-by: chahatsagarmain <[email protected]>
@chahatsagarmain an alternative to fixing the race condition from reloading is to eliminate the use of this package altogether. We already migrated several endpoints to use OTEL helpers which internally handle TLS reloading differently (on a timeout rather than on file change). It would be interesting to see which parts of Jaeger still use the |
@yurishkuro So i can use |
yes, that would be good. You can start small, e.g. can we remove |
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test