Stop a completed run from reporting that it was interrupted - #32
Merged
Conversation
Every pass ended by printing that it was draining after a signal, whether or not anybody signalled it. The deferred stop cancels the context, the goroutine waiting on ctx.Done wakes, and the line goes out. A person reading the log afterwards cannot tell a run that read every document from one somebody abandoned, which is the only thing the line was there to say. The two ways out are separate channels now, and the goroutine is told which one happened. The notice goes to a writer the caller names because a goroutine reading os.Stderr is a thing a test cannot get underneath without racing it, and the race detector says so. Both the breadth passes and the norm run had this. It showed up on a real tax campaign that finished all 400 provisions and signed off as if it had been cut short.
…s is a coin flip The normal return closes done and then cancels the context, so a goroutine that had not reached the select by then found both ready and chose between them at random. That came back heads on Linux and macOS and tails on Windows, which is how CI caught it. The signal branch now asks whether the run had already returned. It can trust the answer: the cancel it observed can only have come from the stop function, and the close happened before it. The test runs the case two hundred times rather than once, since one iteration of a coin flip is what passed on two platforms out of three.
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.
Found while running M14 (#18). Every pass ends by printing this, whether or not anybody signalled it:
Nobody signalled that run. It read all 400 provisions and then announced that it was being cut short.
The cause is the shape the signal handling was written in.
signal.NotifyContextwith a deferredstopand a goroutine waiting onctx.Donemeans the normal return cancels the context too, the goroutine cannot tell the two apart, and the line goes out on the way past. The effect is that a log cannot distinguish a completed pass from an abandoned one, which is the only thing that line was ever for, and on passes that take hours the log is usually all anybody has.The two ways out are separate channels now and the goroutine is told which one happened. The notice also goes to a writer the caller names rather than to
os.Stderrdirectly: a goroutine reading a package level variable is a thing a test cannot get underneath, and the race detector says so rather than letting the test look like it passes.Both the breadth front end and the older norm run had it, from the same copied block, so both are fixed and they share the helper now.
Two tests. The one that matters runs everywhere, because it is the case an ordinary passing run exercises. The one that sends a real SIGTERM is built out on Windows rather than skipped, since there is no signal there to send.
gofmtclean,go vetclean,golangci-lint0 issues,go test -race ./...green, andGOOS=windows go buildandgo vetboth clean.