fix: use a sync.Map to prevent "concurrent map read and map write" fa…#1923
fix: use a sync.Map to prevent "concurrent map read and map write" fa…#1923tinmrn wants to merge 1 commit intogo-task:mainfrom
Conversation
…tal error when using "output: prefixed"
| } | ||
|
|
||
| idx, ok := pw.prefixed.seen[pw.prefix] | ||
| idx, ok := pw.prefixed.seen.Load(pw.prefix) |
There was a problem hiding this comment.
Perhaps LoadOrStore() is better than Load() and then Store()?
There was a problem hiding this comment.
You're right, that would be neater. But I will await the below discussion about sync.Map.
| type Prefixed struct { | ||
| logger *logger.Logger | ||
| seen map[string]uint | ||
| seen *sync.Map |
There was a problem hiding this comment.
Can we simply use a mutex for protecting the seen map? It's actively used in this project: 1, 2.
| seen *sync.Map | |
| muSeen sync.Mutex | |
| seen map[string]uint |
Additionally, sync.Map has drawbacks and rare use cases. See https://victoriametrics.com/blog/go-sync-map/
There was a problem hiding this comment.
From the go docs of sync.Map:
The Map type is optimized for two common use cases: (1) when the entry for a given
key is only ever written once but read many times, as in caches that only grow,
or (2) ...
I think that applies here, because potentially a lot of lines get written and only relatively few prefixes will be used.
I think speed is important here since task execution blocks waiting for output to be written. But I have no strong opinion about either way, and we would have to benchmark to check which is actually faster.
One drawback of sync.Map over a mutex is that sync.Map is not type safe, but I think its usage is easy to oversee in this case.
There was a problem hiding this comment.
Perhaps we need to add a test case to ensure that this PR fixes an issue.
There was a problem hiding this comment.
I can try to write one that is deterministic, but since this is a race condition i'm not sure that's possible.
|
Thanks for your PR. Another PR that fix this issue has been merged (#1974) |
4 participants
…tal error when using "output: prefixed"
fixes #1922