diff --git a/internal/output/output_test.go b/internal/output/output_test.go index ba03c9adfa..1ee9af0bce 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "sync" "testing" "github.com/fatih/color" @@ -153,6 +154,43 @@ func TestPrefixed(t *testing.T) { //nolint:paralleltest // cannot run in paralle }) } +// TestPrefixedConcurrentStdoutStderr is a regression test for +// https://github.com/go-task/task/issues/2945: WrapWriter returns the same +// *prefixWriter for both stdout and stderr, and os/exec copies command +// output to each of them from its own goroutine, so Write can legitimately +// be called concurrently from two goroutines for a single task. Since +// bytes.Buffer isn't safe for concurrent use, unsynchronized access to the +// shared buffer could corrupt its internal state and panic (observed as +// "slice bounds out of range"). Run with -race for a reliable signal; this +// also reproduces the panic reliably on an unfixed prefixWriter even +// without -race, given enough concurrent lines. +func TestPrefixedConcurrentStdoutStderr(t *testing.T) { + t.Parallel() + + l := &logger.Logger{Color: false} + var o output.Output = output.NewPrefixed(l) + stdOut, stdErr, cleanup := o.WrapWriter(io.Discard, io.Discard, "prefix", nil) + + const lines = 5000 + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + for i := range lines { + fmt.Fprintf(stdOut, "stdout line %d\n", i) + } + }() + go func() { + defer wg.Done() + for i := range lines { + fmt.Fprintf(stdErr, "stderr line %d\n", i) + } + }() + wg.Wait() + + require.NoError(t, cleanup(nil)) +} + func TestPrefixedWithColor(t *testing.T) { t.Parallel() diff --git a/internal/output/prefixed.go b/internal/output/prefixed.go index fd2a230570..f819709811 100644 --- a/internal/output/prefixed.go +++ b/internal/output/prefixed.go @@ -38,9 +38,18 @@ type prefixWriter struct { prefixed *Prefixed prefix string buff bytes.Buffer + mutex sync.Mutex } +// Write and close both run under pw.mutex because WrapWriter hands out the +// same *prefixWriter for both stdout and stderr, and os/exec copies each of +// them to its Write method from its own goroutine. bytes.Buffer isn't safe +// for concurrent use, so without this lock, concurrent stdout/stderr output +// races on pw.buff and can panic (e.g. "slice bounds out of range"). func (pw *prefixWriter) Write(p []byte) (int, error) { + pw.mutex.Lock() + defer pw.mutex.Unlock() + n, err := pw.buff.Write(p) if err != nil { return n, err @@ -50,6 +59,9 @@ func (pw *prefixWriter) Write(p []byte) (int, error) { } func (pw *prefixWriter) close() error { + pw.mutex.Lock() + defer pw.mutex.Unlock() + return pw.writeOutputLines(true) }