Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write flag is turned on when deleting a key from a map, even if the key doesn't exist #40327

Closed
amit-davidson opened this issue Jul 21, 2020 · 2 comments

Comments

@amit-davidson
Copy link

@amit-davidson amit-davidson commented Jul 21, 2020

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

go1.13 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/amdavidson/Library/Caches/go-build"
GOENV="/Users/amdavidson/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/amdavidson/dev/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/3p/ydb2x5ls4b94ysv8bw2m8_48fvsncb/T/go-build154927085=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you expect to see?

map write flag is shouldn't be set if the key in the map doesn't exist.

What did you see instead?

map write flag is set when deleting an item even if the key doesn't exist in the map.

What did you do?

The issue arises when 2 goroutines access the map at the same time. The first reads an item from the map, and the second deletes a key from it. Even if no key is found and no write was made, the reading goroutine will suffer concurrent read-write since the write flag is turned on before we check to see if the key exists.

h.flags ^= hashWriting

Few lines before it is mentioned the write flag is turned on only after t.hasher ran since it may panic and no commit (write) will be made. This problem I suggested is similar.

@mvdan
Copy link
Member

@mvdan mvdan commented Jul 21, 2020

The issue arises when 2 goroutines access the map at the same time. The first reads an item from the map, and the second deletes a key from it. Even if no key is found and no write was made

Why would you want to do this in the first place, though? At best, it makes no sense. At worst, it's considered a data race, which is why the runtime panics.

@martisch
Copy link
Contributor

@martisch martisch commented Jul 21, 2020

Concurrently reading and attempting to write or delete a key from a map is a data race. The data race detection during normal runtime is best effort and might not trigger for all data races.
https://golang.org/doc/faq#atomic_maps

The runtime race detection is a tradeoff between simplicity, performance and race detection. There are cases e.g. for uninitialised maps were the map runtime race detection might not trigger however if run with race instrumentation those cases might still be detected as data races.
https://golang.org/doc/articles/race_detector.html

If the goroutine attempting deletion is known always to not delete a key to goroutine does not need to try to delete the key. If the goroutine sometimes actually deletes a key then this will trigger a data race warning even if the map implementation is changed to not set the hashWriting flag when no actual internal structure is changed.

In the current implementation there might not be a write to internal data structures but in the future there might be e.g. for bookkeeping and internal reorgantzaiion of the map for shrinking (even if no key was found).

@martisch martisch closed this Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.