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

Jump to bottom

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

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

pancl1411 opened this issue Apr 7, 2021 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.17

Comments

@pancl1411
Copy link
Contributor

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 Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 7, 2021
@dmitshur dmitshur added this to the Backlog milestone Apr 7, 2021
@dmitshur
Copy link
Contributor

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
Contributor

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
Contributor

bcmills commented Apr 7, 2021

CC @changkun

@changkun
Copy link
Member

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
Contributor

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

@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
@pancl1411
sync: update misleading comment in map.go about entry type while p ==…
202c598 
… 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
@pancl1411 pancl1411 mentioned this issue Apr 8, 2021
Closed
@gopherbot
Copy link

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

@dmitshur dmitshur modified the milestones: Backlog, Go1.17 Apr 8, 2021
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 8, 2021
@dmitshur
Copy link
Contributor

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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.17
Development

Successfully merging a pull request may close this issue.

sync: update misleading comment in map.go about entry type pancl1411/go
5 participants
@dmitshur @bcmills @changkun @gopherbot @pancl1411