Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use a sync.Map to prevent "concurrent map read and map write" fa… #1923

Closed
wants to merge 1 commit into from
Closed

fix: use a sync.Map to prevent "concurrent map read and map write" fa… #1923

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions internal/output/prefixed.go
View file
Open in desktop
Copy link
Contributor

@alexandear alexandear Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we need to add a test case to ensure that this PR fixes an issue.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to write one that is deterministic, but since this is a race condition i'm not sure that's possible.

Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,23 @@ import (
"fmt"
"io"
"strings"
"sync"

"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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simply use a mutex for protecting the seen map? It's actively used in this project: 1, 2.

Suggested change
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/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

counter *uint
}

func NewPrefixed(logger *logger.Logger) Prefixed {
var counter uint

return Prefixed{
seen: make(map[string]uint),
seen: &sync.Map{},
counter: &counter,
logger: logger,
}
Expand Down Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps LoadOrStore() is better than Load() and then Store()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, that would be neater. But I will await the below discussion about sync.Map.


if !ok {
idx = *pw.prefixed.counter
pw.prefixed.seen[pw.prefix] = idx
pw.prefixed.seen.Store(pw.prefix, idx)

*pw.prefixed.counter++
}
Expand All @@ -98,7 +99,7 @@ func (pw *prefixWriter) writeLine(line string) error {
return nil
}

color := PrefixColorSequence[idx%uint(len(PrefixColorSequence))]
color := PrefixColorSequence[idx.(uint)%uint(len(PrefixColorSequence))]
pw.prefixed.logger.FOutf(pw.writer, color, pw.prefix)

if _, err := fmt.Fprint(pw.writer, "] "); err != nil {
Expand Down
Loading