sync: document that double RLock isn't safe

[ghost]

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]

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]

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.

[bronze1man]

@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]

@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]

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]

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]

@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]

@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]

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]

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.

[gopherbot]

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