Make global loggers safe for concurrent use #316
Conversation
|
@akshayjshah, thanks for your PR! By analyzing the history of the files in this pull request, we identified @peter-edge to be a potential reviewer. |
|
are you sure even writing to a pointer is safely atomic? sync.Value seems to be there specifically for this case. |
|
Replacing a pointer is atomic, since it's a single word - readers will see either the old or new value, but not a mix of the two. I'm certainly not a deep expert here - most of my knowledge comes from Russ Cox's blog posts on Go interfaces and data races. |
|
LGTM |
There was a problem hiding this comment.
While writing a pointer is safe on x86 and many other architectures, it may not be safe on all other architectures.
The spec provides no guarantees of this, and reading/writing to a pointer from concurrent goroutines is considered a data race, even if the value is a pointer.
See a couple of golang-nuts threads for more information:
https://groups.google.com/d/msg/golang-nuts/ez9RiiLF0t0/VRIjR5stBQAJ
https://groups.google.com/d/msg/golang-nuts/AULpS8XKDk0/HyzcRloDBgAJ
The data race detector did not flag this since the test has the condition wrong -- it never actually runs the replacement/logging logic.
Once the conditions are flipped, the data race detector should flag this as a race.
global_test.go
Outdated
| for i := 0; i < 100; i++ { | ||
| wg.Add(2) | ||
| go func() { | ||
| for stop.Load() { |
There was a problem hiding this comment.
stop is going to false by default, so we're never running this loop, or the loop below. condition needs to be flipped.
There was a problem hiding this comment.
Well, that's embarrassing. 😬
|
regardless how this works out, this has been an extremely edifying pull request😀 I've been using sync.Value for a similar use case. the performance impact seems to be negligible. like truly negligible. a handful of nanos |
|
Ya I can't lie, the links from this PR have pointed out to me how much I've (amateurishly) glossed over some golang details during my time working with the language. Instead of learning about some key details, I did hacks like this https://github.com/peter-edge/pkg-go/blob/master/sync/volatile_bool.go to make sure I wouldn't run into issues, and @prashantv is reminding me why taking shortcuts like this does nothing for your engineering development. I'm starring this PR in my bookmarks. |
akshayjshah
force-pushed
the
ajs-locks
branch
from
9b42938 to
9e66120
Compare
akshayjshah
changed the title
There was a problem hiding this comment.
Code looks fine -- it's unfortunate that the call site is a little more verbose now.
If we really need to make L and S concurrent safe, then I think this is the right approach. I'd still prefer if the caller set L and S before they started using it to avoid the race.
global_test.go
Outdated
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/assert" |
global.go
Outdated
| L = logger | ||
| S = logger.Sugar() | ||
| return func() { ReplaceGlobals(&prev) } | ||
| // This doesn't require synchronization to be safe, since pointers are a |
There was a problem hiding this comment.
can delete stale comment
I'd originally intended zap's global loggers to be a consenting-adults sort of API in which easily-avoided data races are okay. After just a few migrations, it's become clear just how naive that plan was. This PR adds tests to verify that the global loggers are safe to use and reconfigure concurrently, and it converts the global variables to (short) functions.
akshayjshah
force-pushed
the
ajs-locks
branch
from
9e66120 to
1e69a29
Compare
|
Agreed about the verbosity of the call sites; I was also hoping that users of this API could just take care to replace the global loggers before they're used, but it's become clear to me that I was hopelessly naive. |
This PR adds tests to verify that the global loggers are safe to use and
reconfigure concurrently, and it converts the global variables to (short)
functions.
Users can update their code with a simple gofmt rewrite (which I'll
include in the RC 2 release notes).