-
Notifications
You must be signed in to change notification settings - Fork 683
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
Remove data race from zerolog provider #487
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.
LGTM
Codecov Report
@@ Coverage Diff @@
## v2 #487 +/- ##
===========================================
- Coverage 84.01% 57.58% -26.44%
===========================================
Files 30 28 -2
Lines 932 1530 +598
===========================================
+ Hits 783 881 +98
- Misses 110 586 +476
- Partials 39 63 +24
Continue to review full report at Codecov.
|
Thanks for your contribution! |
Does this fix need to be ported to the v2 branch? |
@johanbrandhorst we can see this bug present in 2.0-rc2, it would be great to release rc3 :) |
@johanbrandhorst I think it's already merged into v2 (https://github.com/grpc-ecosystem/go-grpc-middleware/commits/v2), do I miss something? |
Oh, I see, you simply want another release candidate? Maybe @bwplotka has some plans for a new release soon? |
It's important that
With()
on the adapter callWith()
on zerolog each time, so that we get a copy of the zerolog logger.The zerolog logger isn't threadsafe by default
Here's an old test run showing the race:
https://github.com/authzed/spicedb/runs/5382249173?check_suite_focus=true
And a newer run (using a local copy of the file in this PR):
https://github.com/authzed/spicedb/runs/5450521332?check_suite_focus=true
Open to discussing other options to address this, but this seemed the most straightforward.