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.

Sign up for GitHub

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/cond: redundant code in copyChecker.check #34516

Closed
consensyx opened this issue Sep 25, 2019 · 8 comments
Closed

sync/cond: redundant code in copyChecker.check #34516

consensyx opened this issue Sep 25, 2019 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@consensyx
Copy link

consensyx commented Sep 25, 2019 •
edited by bradfitz

the copy chcker implementation (src/sync/cond.go,line :81)

func (c *copyChecker) check() {
	if uintptr(*c) != uintptr(unsafe.Pointer(c)) &&
		!atomic.CompareAndSwapUintptr((*uintptr)(c), 0, uintptr(unsafe.Pointer(c))) &&
		uintptr(*c) != uintptr(unsafe.Pointer(c)) {
		panic("sync.Cond is copied")
	}
}

the code uintptr(*c) != uintptr(unsafe.Pointer(c)) have twice in same if branch.
@bradfitz bradfitz changed the title The copyChecker in sync/cond have redundant code sync/cond: redundant code in copyChecker.check Sep 25, 2019
@av86743
Copy link

av86743 commented Sep 25, 2019

Reviewed on https://codereview.appspot.com/11573043 .

@ALTree
Copy link
Member

ALTree commented Sep 25, 2019

Note that A && f( ) && A is not equivalent to A && f( ) if f changes a memory location involved in the A expression. So this may be intentional.

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 25, 2019
@ALTree ALTree added this to the Unplanned milestone Sep 25, 2019
@av86743
Copy link

av86743 commented Sep 25, 2019

This is how it works:

First A - quick check that either pointer is not initialized (zero) or initialized to address different from its own (Cond was copied). On fastpath it fails, so it's all right for && f( ) && A to be "slow".
f( ) - initializes pointer with its own address if not initialized yet.
Second A - now that pointer is initialized (not zero), verifies that pointer contains its own address.

@ALTree
Copy link
Member

ALTree commented Sep 26, 2019

I suspect it's something like that, and the code should be left as it is. cc @dvyukov

@av86743
Copy link

av86743 commented Sep 26, 2019

Originator's reasoning is as follows:

  • second A computes only if f( ) holds, that is if CompareAndSwapUintptr returns false, that is when pointer is not changed;
  • if pointer is not changed by f( ), second A should be the same as first A (that is, true.) Hence redundancy.

@av86743
Copy link

av86743 commented Sep 28, 2019

However, consider the following scenario:

Two or more goroutines are using an uninitialized Cond so that their f( ) compute concurrently. Only one f( ) will succeed to initialize pointer; all other will not. Without "redundant" second A, these latter would all unnecessarily panic.

@ALTree ALTree mentioned this issue Aug 20, 2020
Closed
@ALTree
Copy link
Member

ALTree commented Aug 20, 2020

As suspected this is working as intended. Let's use #40924 to track the possibility of adding a comment.

@ALTree ALTree closed this as completed Aug 20, 2020
robinbraemer added a commit to robinbraemer/go that referenced this issue Sep 7, 2020
@robinbraemer
add explanation comment in sync/cond.go
fe392ce 
It adds two internal comments explaining why `c` is checked twice in a row and is intentional.

There has been confusion about why this was done, confusion should not occur anymore:
- golang#34516
- golang#40924
@robinbraemer robinbraemer mentioned this issue Sep 7, 2020
Closed
@gopherbot
Copy link

Change https://golang.org/cl/253378 mentions this issue: sync/cond.go: add explanation comments to check func

@golang golang locked and limited conversation to collaborators Sep 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@av86743 @consensyx @ALTree @gopherbot