fix: synchronize prefixed output writer to fix concurrent-write panic - #2949
Open
ankit090701 wants to merge 1 commit into
Open
fix: synchronize prefixed output writer to fix concurrent-write panic#2949ankit090701 wants to merge 1 commit into
ankit090701 wants to merge 1 commit into
Conversation
WrapWriter in internal/output/prefixed.go returns the same *prefixWriter
instance for both a task's stdout and stderr:
pw := &prefixWriter{writer: stdOut, prefix: prefix, prefixed: p}
return pw, pw, func(error) error { return pw.close() }
os/exec copies a running command's stdout and stderr to their respective
writers from two separate goroutines, so for the "prefixed" output mode,
Write can legitimately be called concurrently on the same prefixWriter by
both the stdout- and stderr-copying goroutines of a single task.
prefixWriter.Write and writeOutputLines read and write pw.buff (a
bytes.Buffer) without any synchronization, and bytes.Buffer is not safe
for concurrent use, so this corrupts its internal state and can panic -
reported as "slice bounds out of range" - whenever a command produces
enough concurrent stdout/stderr output.
Add a mutex to prefixWriter and hold it for the duration of both Write
and close, matching the pattern already used for the separate
Prefixed.mutex that protects the shared prefix/counter map (which does
not, on its own, protect each writer's buffer).
Added TestPrefixedConcurrentStdoutStderr, which spawns two goroutines
writing thousands of lines to stdout and stderr concurrently through the
same prefixed writer. Verified under `go test -race`: without the fix,
this reliably reproduces the exact reported panic (and the underlying
data race); with the fix, it passes cleanly across repeated runs.
Fixes go-task#2945
Signed-off-by: ankit090701 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's the bug
With
output: prefixedenabled, Task can panic while a command writes to stdout and stderr concurrently:Reproduces with:
Root cause
WrapWriterininternal/output/prefixed.goreturns the same*prefixWriterinstance for both stdout and stderr:os/execcopies a running command's stdout and stderr to their respective writers from two separate goroutines (io.copyBufferinCmd.writerDescriptor), so for prefixed output,Writecan legitimately be called concurrently on the sameprefixWriterby both the stdout- and stderr-copying goroutines of a single task.prefixWriter.WriteandwriteOutputLinesread and writepw.buff(abytes.Buffer) without any synchronization, andbytes.Bufferis explicitly documented as not safe for concurrent use - concurrent access corrupts its internal length/offset bookkeeping, which is exactly what produces a "slice bounds out of range" panic.Note that
Prefixed.mutexalready exists and is held inwriteLine, but it only protects the struct shared across all prefixes (theseenmap andcounter) - it does nothing to protect an individualprefixWriter's own buffer from its two concurrent callers.The fix
Add a
sync.MutextoprefixWriterand hold it for the full duration of bothWriteandclose(both of which read/mutatepw.buff). This is a distinct, narrower lock thanPrefixed.mutex, so there's no risk of the two interacting or deadlocking -writeOutputLines(called while holdingprefixWriter.mutex) callswriteLine, which acquires the separatePrefixed.mutex.Testing
TestPrefixedConcurrentStdoutStderr(internal/output/output_test.go): spawns two goroutines writing 5000 lines each to stdout and stderr concurrently through the same prefixed writer, then callscleanup.go test -race: without the fix, this reliably reproduces both the underlying data race and the exact reported panic (slice bounds out of range) within a handful of runs. With the fix, it passes cleanly (no race, no panic) across repeated runs (-count=5, and again in isolation with-race).go build ./... && go vet ./... && go test ./...) - clean aside fromTestEvaluateSymlinksInPaths, which I confirmed is unrelated: it fails the same way with a filesystem/symlink error on an unmodified checkout too, due to how real NTFS symlinks don't survive a Windows→Linux Docker bind mount (I don't have a native Go toolchain on this machine, so I built/tested via Docker).Fixes #2945