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 RWMutex.RLock shouldn't be used recursively #7576

Closed
dvyukov opened this issue Mar 18, 2014 · 12 comments
Closed

sync: document RWMutex.RLock shouldn't be used recursively #7576

dvyukov opened this issue Mar 18, 2014 · 12 comments

Comments

@dvyukov
Copy link
Member

dvyukov commented Mar 18, 2014

Detected by experimental deadlock detector in cl/77450043.

==================
WARNING: DEADLOCK
Goroutine 999 lock mutex 35 while holding mutex 35:
  sync.(*RWMutex).RLock()
      src/pkg/sync/rwmutex.go:42 +0xa2
  sync.(*rlocker).Lock()
      src/pkg/sync/rwmutex.go:133 +0x34
  sync_test.func·027()
      src/pkg/sync/rwmutex_test.go:132 +0xb6

Mutex 35 was previously locked here:
  sync.(*RWMutex).RLock()
      src/pkg/sync/rwmutex.go:42 +0xa2
  sync.(*rlocker).Lock()
      src/pkg/sync/rwmutex.go:133 +0x34
  sync_test.func·027()
      src/pkg/sync/rwmutex_test.go:133 +0xda
==================

==================
WARNING: DEADLOCK
Goroutine 999 lock mutex 16 while holding mutex 16:
  sync.(*RWMutex).RLock()
      src/pkg/sync/rwmutex.go:42 +0xa2
  sync_test.func·028()
      src/pkg/sync/rwmutex_test.go:166 +0x88
  testing.func·002()
      src/pkg/testing/benchmark.go:416 +0x17d

Mutex 16 was previously locked here:
  sync.(*RWMutex).RLock()
      src/pkg/sync/rwmutex.go:42 +0xa2
  sync_test.func·028()
      src/pkg/sync/rwmutex_test.go:167 +0x96
  testing.func·002()
      src/pkg/testing/benchmark.go:416 +0x17d
==================

Tests lock RWMutex recursively. RWMutex is not recursive.
@gopherbot
Copy link

Comment 1 by 2852914@wessie.info:

This looks like a false positive to me. The test locks the `rlocker` multiple times,
which is the read lock from `RWMutex.RLocker()`. By design the read lock from a RWMutex
is allowed to be acquired multiple times.

@dvyukov
Copy link
Member Author

dvyukov commented Mar 18, 2014

Comment 2:

By what design?
At least currently implementation does not support recursive read locking, and such code
indeed deadlocks.

@gopherbot
Copy link

Comment 3 by 2852914@wessie.info:

The documentation http://golang.org/pkg/sync/#RWMutex tells you that "The lock can be
held by an arbitrary number of readers or a single writer". It's also easily
demonstrated by a small example such as http://play.golang.org/p/TteARoVJeh

@dvyukov
Copy link
Member Author

dvyukov commented Mar 18, 2014

Comment 4:

it does not say that the mutex can be locked several times by a single reader
this is a common thing for rw mutexes, you can see that SRWLOCK provides the same
guarantee:
"SRW locks cannot be acquired recursively"
http://msdn.microsoft.com/en-us/library/windows/desktop/aa904937(v=vs.85).aspx

@bradfitz
Copy link
Contributor

Comment 5:

How do you know who the reader is?
Goroutine? You can RLock in one goroutine and release in another, right? If that's
valid, I see no way for you to prohibit multiple RLocks by the "same" reader, however
that's (not) defined.

@gopherbot
Copy link

Comment 6 by 2852914@wessie.info:

So this can still count as a potential deadlock unless the documentation is updated to
guarantee the current behavior. Which is currently implicitly given by the fact that
RLock and RUnlock don't have to be paired in the same goroutine.

@dvyukov
Copy link
Member Author

dvyukov commented Mar 18, 2014

Comment 7:

answered on golang-dev@ mailing list

@rsc
Copy link
Contributor

rsc commented May 21, 2014

Comment 8:

I think the test is fine. I agree that RLock cannot be called recursively if someone
else might be trying to Lock at the same time. But in these two instances the test has
been written so that there are no concurrent calls to Lock. In that Lock-free context,
duplicate RLocks are safe and I think should be expected to work.

Status changed to WorkingAsIntended.

@dvyukov
Copy link
Member Author

dvyukov commented May 21, 2014

Comment 9:

We will need it if/when we deploy deadlock detector.
Locking a mutex when nobody else locks it is pointless, so even if this scenario works,
it is not testing anything useful. And also demonstrates a bad example to readers.

Labels changed: added release-none.

Owner changed to @dvyukov.

Status changed to Accepted.

@rsc
Copy link
Contributor

rsc commented May 21, 2014

Comment 10:

I don't think the test is pointless as written. It is testing that two RLocks can
coexist.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@bradfitz
Copy link
Contributor

@dvyukov, should we repurpose this into a bug to clarify some documentation on https://golang.org/pkg/sync/#RWMutex.RLock to say that it must not be used recursively by the same caller?

@bradfitz bradfitz modified the milestones: Go1.9, Unplanned Jan 31, 2017
@bradfitz bradfitz changed the title sync: potential deadlocks in tests sync: document RWMutex.RLock shouldn't be used recursively Jan 31, 2017
@dvyukov
Copy link
Member Author

dvyukov commented Feb 1, 2017

The documentation already says this:

If a goroutine holds a RWMutex for reading, it must not expect this or any other goroutine to be able to also take the read lock until the first read lock is released. In particular, this prohibits recursive read locking. This is to ensure that the lock eventually becomes available; a blocked Lock call excludes new readers from acquiring the lock.

@rsc rsc closed this as completed Feb 1, 2017
@golang golang locked and limited conversation to collaborators Feb 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants