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: document that double RLock isn't safe #15418

Closed
ghost opened this issue Apr 23, 2016 · 11 comments
Closed

sync: document that double RLock isn't safe #15418

ghost opened this issue Apr 23, 2016 · 11 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ghost
Copy link

ghost commented Apr 23, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version go1.6 darwin/amd64
  2. What operating system and processor architecture are you using (go env)?
    GOARCH="amd64"
    GOBIN=""
    GOEXE=""
    GOHOSTARCH="amd64"
    GOHOSTOS="darwin"
    GOOS="darwin"
    GOPATH=""
    GORACE=""
    GOROOT="/usr/local/go"
    GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
    GO15VENDOREXPERIMENT="1"
    CC="clang"
    GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fno-common"
    CXX="clang++"
    CGO_ENABLED="1"
  3. What did you do?
package main

import (
    "fmt"
    "sync"
)

func main() {
    ch := make(chan int)
    testFunc := func() {
        weirdLock := sync.RWMutex{}
        go func() {
            weirdLock.RLock()
            go func() {
                weirdLock.RLock()
                for i := 0; i < 3; i++ {
                    func() {
                        weirdLock.RLock()
                        ch <- 1
                        weirdLock.RUnlock()
                    }()
                }
                weirdLock.RUnlock()
            }()
            weirdLock.RUnlock()
        }()
        go func() {
            weirdLock.Lock()
            ch <- 1
            weirdLock.Unlock()
        }()
    }
    runNum := 1000
    for i := 0; i < runNum; i++ {
        testFunc()
    }
    total := 0
    for i := 0; i < 4*runNum; i++ {
        total += <-ch
    }
    //Expect 4*runNum
    fmt.Println(total)
}

http://play.golang.org/p/UWIAmPCRCu

  1. What did you expect to see?
    output 4000
  2. What did you see instead?
    fatal error: all goroutines are asleep - deadlock!
    runNum more big,the probability of deadlock more often
@bronze1man
Copy link
Contributor

bronze1man commented Apr 23, 2016

I found a simpler case looks like this issues.

http://play.golang.org/p/pEhnPbsnDG

package main

import (
    "sync"
    "time"
)

func main() {
    /*
    TimeLine:
    * Thread A locker.RLock() return
    * Thread B locker.Lock() start
    * Thread A locker.RLock() start
    * Thread A and Thread B are deadlock
     */
    locker:=sync.RWMutex{}
    locker.RLock()
    go func(){
        locker.Lock()
        locker.Unlock()
    }()
    time.Sleep(10*time.Millisecond)
    locker.RLock()
    locker.RUnlock()
    locker.RUnlock()
    time.Sleep(10*time.Millisecond)
}

I found another case which is not deadlock.
http://play.golang.org/p/cZeBkt11Kv

package main

import (
    "sync"
    "time"
)

func main() {
    locker:=sync.RWMutex{}
    locker.RLock()
    go func(){
        locker.Lock()
        locker.Unlock()
    }()
    go func(){
        time.Sleep(10*time.Millisecond)
        locker.RLock()
        locker.RUnlock()
    }()
    time.Sleep(20*time.Millisecond)
    locker.RUnlock()
}

So It looks like that you should not use RLock twice in the same thread from those examples.
If this is an implement limit,you should add a document to the RWMutex.

@bradfitz
Copy link
Contributor

Yes, it's not safe to recursively acquire an RLock. It is documented a bit already, indirectly:

To ensure that the lock eventually becomes available, a blocked Lock call excludes new readers from acquiring the lock.

@bradfitz bradfitz added this to the Go1.7Maybe milestone Apr 23, 2016
@bradfitz bradfitz changed the title recursive RWMutex calls deadlock sync: document that double RLock isn't safe Apr 23, 2016
@bronze1man
Copy link
Contributor

bronze1man commented Apr 23, 2016

@bradfitz
Is it ok for you to do following things in the following order by defination?

  • Thread A Rlock call return
  • Thread B Lock call start
  • Thread C RLock call start
  • Thread A RUnLock call return
  • Thread C RLock call return
  • Thread C RUnLock call return
  • Thread B Lock call return

@bradfitz
Copy link
Contributor

@bronze1man the goroutine doesn't matter. Any RLock caller cannot depend on a future RLock succeeding in any goroutine as long as the first RLock is held.

@abligh
Copy link

abligh commented May 2, 2016

That's a pretty significant limitation given the reason to use an RWMutex over a Mutex might precisely be because you need to have more than one RLock held at once (as opposed to merely you don't mind if it happens occasionally).

The docs currently say:

An RWMutex is a reader/writer mutual exclusion lock. The lock can be held by an arbitrary number of readers or a single writer. RWMutexes can be created as part of other structures; the zero value for a RWMutex is an unlocked mutex.

Clearly current behaviour does conform to the above (unless you read the bit in bold as 'the lock might be able to be held by an arbitrary number of readers' which I would suggest is an unnatural interpretation).

At the least this is a documentation problem, but given the fact that RWMutex's have well known behaviour outside go, one could argue this is in fact a bug.

@cznic
Copy link
Contributor

cznic commented May 2, 2016

IMO, the docs are correct. Any numbers of goroutines may simultaneously acquire a RLock of the same RWMutex. What does not work is when the same goroutine calls RLock on a RWMutex when that goroutine already did that without calling RUnlock first.

It's a general principle, applicable to all mutexes provided by the stdlib. The same goroutine cannot lock any mutex if it already did that and did not yet unlocked it.

@abligh
Copy link

abligh commented May 2, 2016

@cznic according to @bradfitz , the goroutine does not matter:

@bronze1man the goroutine doesn't matter. Any RLock caller cannot depend on a future RLock succeeding in any goroutine as long as the first RLock is held.

(my emphasis)

What he is saying (I think) is that if goroutine A takes an RLock, it cannot rely on goroutine B being able to acquire an RLock() before A drops its RLock.

@bronze1man
Copy link
Contributor

bronze1man commented May 2, 2016

@abligh I know the goroutine does not matter.
But you need the goroutine to get the buggy example.

I think @bradfitz says that you can not have one thread call RLock twice and call RUnlock twice in sequence,and have an other thread call Lock at the same time.It will deadlock with little possibility.(It just look like the possibility of the data race has happend.)
The problem is that the sync.RWMutex may deadlock with little possibility in that case.
You can write a test case for that case , but you will pass the test case most of time.
So I think that is the reason why this forbids using the case which should be documented as soon as possible.

By the way,I think using the case in the way that i have found is better than the sentence "Any RLock caller cannot depend on a future RLock succeeding in any goroutine as long as the first RLock is held".

@bradfitz bradfitz modified the milestones: Go1.7, Go1.7Maybe May 20, 2016
@bradfitz
Copy link
Contributor

I've seen so much confusion about this in the past few months that I've promoted this to a Go1.7 documentation issue, instead of a "Maybe". I do think it needs to be documented well. I know we generally tell people to stay out of the sync package (or atomic package too) if they don't know what they're doing, but people still use these things, so warnings should be on the label.

@dvyukov, can you maybe write some docs here?

@dvyukov
Copy link
Member

dvyukov commented May 21, 2016

How about this?

If a goroutine has locked RWMutex for reading, it must not expect this,
or any other goroutine to be able to lock RWMutex for reading until the
first read lock is released. In particular, this prohibits recursive read
locking. The restriction is due to fairness properties: to ensure that
the lock eventually becomes available, a blocked Lock call excludes new
readers from acquiring the lock.

I would put such important notes on the type rather than on methods.

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label May 26, 2016
@adg adg self-assigned this May 30, 2016
@gopherbot
Copy link

CL https://golang.org/cl/23570 mentions this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants