fix: use a sync.Map to prevent "concurrent map read and map write" fa… #1923
Conversation
…tal error when using "output: prefixed"
| @@ -85,11 +86,11 @@ func (pw *prefixWriter) writeLine(line string) error { | |||
| line += "\n" | |||
| } | |||
|
|
|||
| 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.
|
|
||
| "github.com/go-task/task/v3/internal/logger" | ||
| "github.com/go-task/task/v3/internal/templater" | ||
| ) | ||
|
|
||
| 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.
…tal error when using "output: prefixed"
fixes #1922