[RE-5863] Resolve potential race condition in log fields #185
Conversation
Codecov Report
@@ Coverage Diff @@
## master #185 +/- ##
==========================================
- Coverage 58.49% 57.97% -0.52%
==========================================
Files 13 13
Lines 559 564 +5
==========================================
Hits 327 327
- Misses 225 230 +5
Partials 7 7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| }(i) | ||
| } | ||
| wgRead.Wait() | ||
| wgWrite.Wait() |
There was a problem hiding this comment.
it seems we could use single wait group here, how about using the same wg?
jalacardio
changed the title
plf is already a pointer thus no need to refetch from ctx and cast again
|
shouldn't you just change logfields to sync.Map instead ? something along the lines of - https://github.com/go-coldbrew/log/pull/5/files |
|
That is in fact a more concise way to dealing with it, thanks for the info and will look into it. |
| } | ||
| } | ||
|
|
||
| func BenchmarkFromAddToLogContext(b *testing.B) { |
There was a problem hiding this comment.
function name should be BenchmarkAddToLogContext
| @@ -12,6 +13,10 @@ var ( | |||
|
|
|||
| //LogFields contains all fields that have to be added to logs | |||
| type LogFields map[string]interface{} | |||
| type protectedLogFields struct { | |||
| content LogFields | |||
| mtx sync.RWMutex | |||
There was a problem hiding this comment.
As @ankurs suggested, sync.Map is indeed a better option.
According to https://pkg.go.dev/sync#Map, it optimizes for our use case:
when the entry for a given key is only ever written once but read many times, as in caches that only grow
Description
In current version of logFields, we discovered that there is a potential risk of having racing condition when multiple goroutine try to access logFields under an identical context (ctx). As you may see from the report below, when running parallelism test on the current implementation, a race condition is emerged even with a very little sample rate.
Therefore, in this PR, we are turning the old logFields into protected logFields by applying read/write lock. Now when logFields being accessed, it will first check if some other instance is also accessing it, if yes, it will wait until the access is freed up so no simultaneous write will happen.
In effects, FromContext doesn't seems to have it's performance decreased, while AddToLogContext increased about 10% of it's process time, and it's all being said with race condition is no longer a concern.
Review Guideline
FromContext
Applied with read lock as it's a public method, now we are return a copy of the map instead of giving access directly to logFields
fromContext
Same implementation as before, but now it's only for private access
AddToLogContext
Now write access is locked when writing new data to logFields
How to Test
Parallelism Test
Previously
Now
Benchmark stat
Previously
Now
Checklist
go mod tidyandgo mod vendorScreenshots (Optional)
Related PRs (Optional)
Impacted Endpoints (Optional)