go: fmt: improve or avoid panic when printing map that is changing as it is being printed

What version of Go are you using (`go version`)?

$ go version
1.12

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (`go env`)?

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/yaotianjia/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/yaotianjia/goworkspace"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build822647916=/tmp/go-build -gno-record-gcc-switches"

What did you do?

using fmt.Sprintf(“%+v”,arg) to print a map, and it panics

What did you expect to see?

It should shows the content of the map

What did you see instead?

runtime panic: runtime error: index out of range

runtime panic: runtime error: index out of range
......
panic(0x320a360, 0x6a3f2b0)
    /usr/local/go/src/runtime/panic.go:522 +0x1b5
internal/fmtsort.Sort(0x31d94a0, 0xc01b3a7ab0, 0x195, 0x1)
    /usr/local/go/src/internal/fmtsort/sort.go:60 +0x34f
fmt.(*pp).printValue(0xc03c81dc80, 0x31d94a0, 0xc01b3a7ab0, 0x195, 0x76, 0x2)
    /usr/local/go/src/fmt/print.go:759 +0xd20
fmt.(*pp).printValue(0xc03c81dc80, 0x360b300, 0xc01b3a7680, 0x199, 0xc000000076, 0x1)
    /usr/local/go/src/fmt/print.go:796 +0x1b52
fmt.(*pp).printValue(0xc03c81dc80, 0x30f0ba0, 0xc01b3a7680, 0x16, 0xc000000076, 0x0)
    /usr/local/go/src/fmt/print.go:866 +0x1956
fmt.(*pp).printArg(0xc03c81dc80, 0x30f0ba0, 0xc01b3a7680, 0x76)
    /usr/local/go/src/fmt/print.go:702 +0x2ba
fmt.(*pp).doPrintf(0xc03c81dc80, 0x3757e58, 0x3c, 0xc00fa93cf8, 0x2, 0x2)
    /usr/local/go/src/fmt/print.go:1016 +0x158
fmt.Sprintf(0x3757e58, 0x3c, 0xc00fa93cf8, 0x2, 0x2, 0x1e, 0x363fa6c)
    /usr/local/go/src/fmt/print.go:214 +0x66
......

the reason of the panic

https://github.com/golang/go/blob/release-branch.go1.12/src/internal/fmtsort/sort.go#L59

the built-in function is shown as follow

func Sort(mapValue reflect.Value) *SortedMap {
	if mapValue.Type().Kind() != reflect.Map {
		return nil
	}
	key := make([]reflect.Value, mapValue.Len())
	value := make([]reflect.Value, len(key))
	iter := mapValue.MapRange()
	for i := 0; iter.Next(); i++ {
		key[i] = iter.Key()
		value[i] = iter.Value()
	}
	sorted := &SortedMap{
		Key:   key,
		Value: value,
	}
	sort.Stable(sorted)
	return sorted
}

The variable key and value is pre-calculated and then the code iterates by iter, when another goroutine add a key to map, the length of key is shorter than the iter’s range. Therefore, it panics.

My solutions

I think the code should consider the range of key, maybe it’s better looks like this:

for i := 0; iter.Next() && i < len(key) ; i++ {
    ...
}

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 29 (9 by maintainers)

Commits related to this issue

internal/fmtsort: restrict the for-range by len(key) this modification prevents the fmt panicking with reason index out of range Fixes #33275 Change-Id: I81fbd1997b8cd1f5f30549eb4640b516105d1dcc — committed to J-CIC/go by J-CIC 5 years ago
internal/fmtsort: don't out-of-bounds panic if there's a race condition Raising an out-of-bounds panic is confusing. There's no indication that the underlying problem is a race. The runtime already ... — committed to tomocy/go by randall77 5 years ago
internal/fmtsort: don't out-of-bounds panic if there's a race condition Raising an out-of-bounds panic is confusing. There's no indication that the underlying problem is a race. The runtime already ... — committed to t4n6a1ka/go by randall77 5 years ago

Most upvoted comments

@av86743 Please avoid saying things like “then you fail”. That is unfriendly. The same sentiment could be phrased in a less destructive way, for example by saying “unfortunately we are unlikely to accept such a change without a test”. Please see https://golang.org/conduct. Thanks.

ianlancetaylor on Jul 25, 2019

… It’s programmer’s duty to ensure the program won’t encouter a race condition. …

Please explain yourself. My google translate does not work for this.

av86743 on Jul 26, 2019

I do think there is something to be fixed here. At the very least, the panic could be improved. I see the options as

Leave the code as is (the status quo)
Panic, but with a better error message
Don’t panic, but add something to the string output to indicate a data race happened
Ignore the error and proceed

Note that we do 4 in reflect.MapKeys. Maybe that’s not the right choice, but it is precedent.

I’d argue that fmt is kind of special and warrants considering doing either 3 or 4. Unlike reflect.MapKeys, for example, the results of fmt.Printf probably aren’t being used programmatically, but printed or logged. In that case, it’s almost certainly better to allow the print to proceed without a panic. The print/log might be happening as part of handling a panic, and triggering a panic while handling a panic is almost always a bad idea.

randall77 on Jul 25, 2019

I agree that we should handle correctly on concurrent behavior on map, that’s how I fixed the program. But this panic message(“index out of range”) may confuse other programmer ?

The main way to detect these problems is to run tests and some production workload with the race detector that is meant to find these problems. The proposed change hides this problem from the programmer as no panic would trigger anymore. I do not think hiding the problem of the race that can surface in multiple other ways would be preferred. Racy programs can fail in many ways many are confusing and actually triggering a panic is one of the better observable failures vs silent data corruption.

If you have a suggestion for an improvement that helps in a non racy program or a way to better detect races with the race detector please add a new issue. Otherwise I would suggest to close this issue as I do think It is not clear what the proposed addition would improve and its based on working around a non well formed go program.

martisch on Jul 25, 2019

And how it would possible to print/log map “as part of handling a panic”? As your intention concerns removing only map-related panic.

func f(m map[int]int) {
    defer func() {
        fmt.Printf("f failed on map %v\n", m)
    }()
    ... do some complex calculation with m...
}

Then if the complex calculation panics for whatever reason, we can print (or log) the map used for that calculation.