Skip to content

Data race on color.NoColor when commands run concurrently #1027

@highlyavailable

Description

@highlyavailable

What's wrong

initCommand in internal/temporalcli/commands.go uses the global color.NoColor from github.com/fatih/color as scratch space. It saves the current value into origNoColor (around line 476), the PersistentPreRunE closure overwrites it based on the --color flag and JSON output, and PersistentPostRun puts it back (line 513).

That's fine for the real CLI, where each invocation is its own process. But the tests run commands concurrently in one process, and then those saves, overwrites and restores race against each other. TestServer_StartDev_ConcurrentStarts is the one that hits it.

Repro

go test -run TestServer_StartDev_ConcurrentStarts -race ./internal/temporalcli/

Race trace

WARNING: DATA RACE
Read at 0x000127f6a36d by goroutine 36941:
  github.com/temporalio/cli/internal/temporalcli.(*TemporalCommand).initCommand()
      internal/temporalcli/commands.go:476
  github.com/temporalio/cli/internal/temporalcli.Execute()
      internal/temporalcli/commands.go:373

Previous write at 0x000127f6a36d by goroutine 68:
  github.com/temporalio/cli/internal/temporalcli.(*TemporalCommand).initCommand.func2()
      internal/temporalcli/commands.go:513

Notes

Found this while fixing #581. IDK if there is a quick fix since color.NoColor is a package global in a third-party library and concurrent commands can't each keep their own value. Either the pre-run/post-run color block needs serializing, or the color preference should live on CommandContext instead of the global.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions