Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 38 additions & 0 deletions internal/output/output_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"io"
"sync"
"testing"

"github.com/fatih/color"
Expand Down Expand Up @@ -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()

Expand Down
12 changes: 12 additions & 0 deletions internal/output/prefixed.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,18 @@ type prefixWriter struct {
prefixed *Prefixed
prefix string
buff bytes.Buffer
mutex sync.Mutex

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Its quite often abbreviated to 'mu' rather than mutex. I don't mind personally, just mentioning.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@C:\Users\AnkitPrasad\Desktop\Test\scratchpad_mu_reply.md

}

// 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
Expand All @@ -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)
}

Expand Down