runtime: concurrent map iter+write fatal doesn't respect GOTRACEBACK

[bradfitz]

Go version

go version go1.22.4 linux/amd64

What did you do?

• Introduce a data race on a map (repro: https://go.dev/play/p/rQ1QQ7PMZ86)
• Don't have sufficient test coverage that runs concurrently to catch it in CI
• Ship it to prod
• Have over a million goroutines
• Wait an hour....

What did you see happen?

... we hit the race on the map in prod after an hour and 💣

The Go runtime then did its fatal:

fatal error: concurrent map iteration and map write

But then we got 3 GB of GOTRACEBACK stacks from the Go runtime for 1M+ stacks, about 3 GB more than we needed to find the bug, overwhelming our logging system in the process, causing a secondary outage.

GOTRACEBACK docs say:

The failure prints stack traces for all goroutines if there is no current goroutine or the failure is internal to the run-time.

And arguably a map race is internal to the run-time insofar as maps are implemented in the runtime.

But the Go runtime know it's the user's fault; note the "but is used when user code is expected to be at fault for the failure" bit here:

// fatal triggers a fatal error that dumps a stack trace and exits.
//
// fatal is equivalent to throw, but is used when user code is expected to be
// at fault for the failure, such as racing map writes.
//
// fatal does not include runtime frames, system goroutines, or frame metadata
// (fp, sp, pc) in the stack trace unless GOTRACEBACK=system or higher.
//
//go:nosplit
func fatal(s string) {
        // Everything fatal does should be recursively nosplit so it
        // can be called even when it's unsafe to grow the stack.
        systemstack(func() {
                print("fatal error: ")
                printindented(s) // logically printpanicval(s), but avoids convTstring write barrier
                print("\n")
        })

        fatalthrow(throwTypeUser)
}

What did you expect to see?

I'd expect fatalthrow(throwTypeUser) to be treated as a normal panic with GOTRACEBACK=single respected.

Currently it's ignored:

bradfitz@book1pro ~ % GOTRACEBACK=single go run maprace.go 2>&1 | wc -l
     618
bradfitz@book1pro ~ % GOTRACEBACK=none go run maprace.go 2>&1 | wc -l
       2
bradfitz@book1pro ~ % GOTRACEBACK=system go run maprace.go 2>&1 | wc -l
    1684

(At least none works, but that's too quiet, hiding the actual problem).

[gabyhelp]

Similar Issues

• runtime: some runtime throws print all goroutine stack dumps by default #46995
• runtime: concurrent map access detection in hashmap.go only catches one stack #26703
• runtime/race: document and fix "trap when race detected" #23577
• runtime: crash while deleting all keys in map while ranging over it with Go 1.1.2 #6216

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[MikeMitchellWebDev]

It says the following in runtime1.go

// gotraceback returns the current traceback settings.
//
// If level is 0, suppress all tracebacks.
// If level is 1, show tracebacks, but exclude runtime frames.
// If level is 2, show tracebacks including runtime frames.
// If all is set, print all goroutine stacks. Otherwise, print just the current goroutine.
// If crash is set, crash (core dump, etc) after tracebacking.

[mknyszek]

CC @prattmic I know some time ago you reorganized the fatal/panic/throw code in the runtime, I'm not sure how long ago it was, but possibly relevant? Also, would this still be a problem with the new map implementation?

Putting this into backlog in any case.

[prattmic]

Yes, the reorganization was in CL 390421, though I don't think it affected map fatal error behavior. The new maps will behave the same, they use the same fatal path.

All is forced for fatal calls (== throwTypeUser) here. I think it could be changed to not force all easily if we choose to do so.

FWIW, I think getting all goroutines for races is useful since you might want to try to find the other side of the race. But we should probably respect GOTRACEBACK.

[gopherbot]

Change https://go.dev/cl/649535 mentions this issue: runtime: respect GOTRACEBACK for user-triggered runtime panics