[Backend][Reentrant] Introduce Reentrant Backend #96
Conversation
atomic.SwapPointer() is the only way to retrieve the old value atomically.
Level and memory backend are read mostly, so atomic value can a very limited effect on performance; Only write map acces need to be done under Mutex. Now Backend loggers could be called from multiple goroutine. Tests validated with : go test -v -race ./...
|
It's seems you testing your code with very old golang version, atomic.Value request Golang >= 1.4 |
|
@op : Did you have time to look my patch ? |
|
@nplanel Sorry for the silence. Been very busy lately. Will try to get some time. |
|
@op ok no pb, If you look at the commit, one is a bugfix that you can apply/cherry-pick today. |
|
@op Any update ? |
| levels := l.levels.Load().(moduleLeveledMap) | ||
| l.lock.Lock() | ||
| levels[module] = level | ||
| l.lock.Unlock() |
There was a problem hiding this comment.
I could be wrong, but looks like the levels map is being modified, yet the reader in GetLevel is not underneath a lock so this can race the read and the write.
Maybe there's a way to even get rid of the map lookup in regular logging. Maybe GetLevel needs to return a pointer or sync.Value. And the IsEnabledFor can cache that (it is never removed)
|
I thought I'd help out with some reviewing. Really would love to be able to set levels for all the loggers from a single routine. I'm thinking reloading configuration to enable debug logging - that would make diagnostics so much easier when a problem is happening. I did have some concern with the handling though as it seems there may still be a race on the |
| sync.end.Add(10) | ||
| sync.start.Add(10) | ||
| for i := 0; i < 10; i++ { | ||
| go testConcurrent_Log(i, sync, &lvlBackend, log) |
There was a problem hiding this comment.
Concurrent use of GetLevel with SetLevel is not under test here I think. Thus why the go test -race probably isn't picking up the concurrent read/write access to the levels map
Level and memory backend are read mostly, so atomic value can a very
limited effect on performance; Only write map acces need to be done under
Mutex.
Now Backend loggers could be called from multiple goroutine.
Tests validated with : go test -v -race ./...