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

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions