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

sync, x/sync/syncmap: misleading comment in map.go about entry type while p == nil #45429

Closed
pancl1411 opened this issue Apr 7, 2021 · 8 comments
Closed
Labels
FrozenDueToAge NeedsFix
Milestone

Comments

@pancl1411
Copy link
Contributor

@pancl1411 pancl1411 commented Apr 7, 2021

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

$ go version
go version go1.15.6 linux/amd64

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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/panchenglong01/.cache/go-build"
GOENV="/home/panchenglong01/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/panchenglong01/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/panchenglong01/go:/mnt/e/Code/leetcode"
GOPRIVATE=""
GOPROXY="https://goproxy.io"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
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-build350312319=/tmp/go-build -gno-record-gcc-switches"

What did you do?

1) the misleading comment i think:
the misleading comment line about entry type while p == nil, as follows:

// If p == nil, the entry has been deleted and m.dirty == nil.  

in entry type:

type entry struct {
	p unsafe.Pointer // *interface{}
}

2) the source code logic i understand:

  1. considering a status that sync.Map has some valid key in both read.m and dirty map (which is common, and means dirty != nil);
  2. then, call LoadAndDelete to delete one of these vaild key;
  3. the code logic as follows:
func (m *Map) LoadAndDelete(key interface{}) (value interface{}, loaded bool) {
	read, _ := m.read.Load().(readOnly)
	// (1) key exist in read.m,  ok == true
        e, ok := read.m[key]  
	// (2) will not querying dirty if read.m exist key ( dirty != nil )
	if !ok && read.amended {
		m.mu.Lock()
		read, _ = m.read.Load().(readOnly)
		e, ok = read.m[key]
		if !ok && read.amended {
			e, ok = m.dirty[key]
			delete(m.dirty, key)
			m.missLocked()
		}
		m.mu.Unlock()
	}
       // (3) delete this key
	if ok {
		return e.delete()                  
	}
	return nil, false
}

func (e *entry) delete() (value interface{}, ok bool) {
	for {
		p := atomic.LoadPointer(&e.p)
		if p == nil || p == expunged {
			return nil, false
		}
                // (4) set entry.p == nil, but this time has m.dirty != nil
		if atomic.CompareAndSwapPointer(&e.p, p, nil) {          
			return *(*interface{})(p), true
		}
	}
}

3) the diff between comment and code:

  1. in code comment, If p == nil with m.dirty == nil
// If p == nil, the entry has been deleted and m.dirty == nil.  
  1. but as mentioned above, sometimes p == nil with m.dirty != nil

What did you expect to see?

code comments should cover all cases, but this one it seems not, am i misunderstand?

What did you see instead?

in code comment, If p == nil with m.dirty == nil , but sometimes p == nil with m.dirty != nil.

Additional

I am new to use golang, and not sure if I understand this correctly, so let me know if i am wrong.

thanks a lot!

@dmitshur dmitshur changed the title sync: misleading comment in map.go about entry type while p == nil sync, x/sync/syncmap: misleading comment in map.go about entry type while p == nil Apr 7, 2021
@dmitshur dmitshur added the NeedsInvestigation label Apr 7, 2021
@dmitshur dmitshur added this to the Backlog milestone Apr 7, 2021
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Apr 7, 2021

I believe this comment was originally added in CL 37342, and it also exists in golang.org/x/sync/syncmap.

CC @bcmills in case this is still familiar to you.

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 7, 2021

LoadAndDelete itself was added recently (in CL 205899, with a subsequent fix in CL 250197).

At any rate, I think you are correct that it is possible for p == nil when m.dirty != nil. Perhaps the comment should read:

	// If p == nil, the entry has been deleted, and either m.dirty == nil or
	// m.dirty[key] is e.

@pancl1411, does that seem more correct to you?

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 7, 2021

CC @changkun

@changkun
Copy link
Member

@changkun changkun commented Apr 7, 2021

I just revisited the code. The described execution flow had already existed in the old Delete implementation before LoadAndDelete CLs. I think I didn't pay too much attention to the comments in entry while introducing LoadAndDelete.

Not entirely sure why the comment was in that way but I have a theory:
If the entry has been deleted, then the future m.read access of the key can lead to delete of that key in the dirty map, and trigger missLocked, which eventually have: m.dirty == nil before the next write (sync.Map is designed for high-read low-write).

If that is the case, then we don't need change anything; otherwise I think the proposed comments is the right fix. What do you think?

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 7, 2021

I think it's probably a good idea to update the comment. (It should describe invariants that are always satisfied, not just conditions that will eventually be satisfied.)

@pancl1411
Copy link
Contributor Author

@pancl1411 pancl1411 commented Apr 8, 2021

@changkun I totally agree with your thoery, it sounds reasonable, so we don't need change anything except update comments.

@bcmills the comment you update is good enough for me:

	// If p == nil, the entry has been deleted, and either m.dirty == nil or
	// m.dirty[key] is e.

thank you all for detailed analysis and promptly apply, so should I commit this as a pull request?

pancl1411 added a commit to pancl1411/go that referenced this issue Apr 8, 2021
… nil

As discussed in: golang#45429,  about entry
type comments, it is possible for p == nil when m.dirty != nil, so
update the commemt about it.

Fixes golang#45429
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 8, 2021

Change https://golang.org/cl/308292 mentions this issue: sync: update misleading comment in map.go about entry type

@dmitshur dmitshur removed this from the Backlog milestone Apr 8, 2021
@dmitshur dmitshur added this to the Go1.17 milestone Apr 8, 2021
@dmitshur dmitshur added NeedsFix and removed NeedsInvestigation labels Apr 8, 2021
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Apr 8, 2021

I noticed the comment in the golang.org/x/sync/syncmap package is in a pre_go19.go file, since after Go 1.9 it just does type Map = sync.Map. So it's probably not a big deal to leave it as is.

@golang golang locked and limited conversation to collaborators Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants