fix: harden concurrent transfers and terminal output - #123
Open
BKPepe wants to merge 4 commits into
Open
Conversation
Import order, --simple block indentation and const alignment. No behaviour change; split out to keep the next commit's diff readable.
- BytesCounter.total mixed atomic and plain access; now an atomic.Uint64. - Download/Upload leaked in-flight goroutines past "break Loop"; the done signal is now ctx-aware and both wait on a WaitGroup before reading the counter. - Results rode on spinner.FinalMSG, which never prints without a TTY, so piping produced no output at all. Printed via output.WriteUI now, and the spinner moved off stdout onto stderr. - Server-supplied strings were printed raw, allowing ANSI escapes and a forged --list entry. output.Sanitize strips C0/DEL/C1 at every display site; url.Parse only rejects ASCII controls, so C1 still needs stripping. - BytesCounter.Read and its reader/pos/lock fields became unreachable when Upload started building a body reader per request; dropped.
- Upload's response-body copy now filters context.Canceled/DeadlineExceeded the way the download path already did, so ending the test no longer logs a spurious debug failure. - The two raw-payload debug dumps use %q instead of Sanitize: newlines are legitimate in a response body, and quoting escapes them without losing them. - TestSanitize covers C0, DEL, C1 and the printable boundaries, exhaustively rather than by example. - TestBytesCounterConcurrentAccess reproduces the spinner-vs-transfer access pattern. Verified: it reports DATA RACE against the pre-fix counter and passes against the fixed one. - Download and Upload are exercised against a server that never ends a request, so a hanging wg.Wait() fails the test rather than a release.
15s was arbitrary. A healthy run measures 0.90-0.94s (2 requests x 200ms spawn interval + the 500ms test duration; the unwind after cancel() is 10-40ms, with or without -race), so the budget is now derived from those parts plus 2s of slack, giving 2.9s. Verified both ways: the tests still pass, and an artificially stalled goroutine still fails them - at 2.91s instead of 15s.
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.
There were two bugs in the transfer path: the byte counter mixed atomic and plain access to
total(the spinner polls it while transfers write to it), and Download/Upload never waited for their goroutines, so the final number could be read while requests were still in flight.Piping also produced no output — every result line depended on
spinner.FinalMSG, which never prints when stderr isn't a terminal. Results go throughoutput.WriteUInow, while the spinner writes to stderr.Server names and the getIP response were also printed unescaped, allowing a backend to inject ANSI sequences into the terminal or smuggle a newline into
--listoutput.Tests are new. The counter test is the interesting one: it reports a DATA RACE against the old implementation, so it reproduces the original bug rather than just exercising the new code.
go test -race ./...is clean.