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

add deadlock check with duplicate taking a lock in a goroutine #7

Merged
merged 2 commits into from
Nov 30, 2017

Conversation

smileusd
Copy link
Contributor

@smileusd smileusd commented Nov 30, 2017

fix #5


This change is Reviewable

@sasha-s
Copy link
Owner

sasha-s commented Nov 30, 2017

:lgtm:


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@sasha-s sasha-s merged commit b09a884 into sasha-s:master Nov 30, 2017
@a-robinson
Copy link

Was it intended that this also fires for calling RLock() twice in the same goroutine? Based on the README update and the test I'd say it looks unintended, but want to double check before changing it to not fire for such situations.

@sasha-s
Copy link
Owner

sasha-s commented Feb 7, 2018

This was unintentional, but it might be a good thing.
As far as I remember, doing calling RLock() twice from the same goroutine is dangerous:
it could deadlock if another goroutine tries to grab a regular (write) lock, while the first one is holding an RLock.
There was some discussion here:
#4 (comment)
To be more specific,
this code occasionally deadlocks:

var a sync.RWMutex
    var wg sync.WaitGroup

    sleep := func() {
      time.Sleep(time.Duration((rand.Intn(20))) * time.Millisecond)
    }

    rlockTwice := func() {
      defer wg.Done()
      sleep()
      a.RLock()
      sleep()
      a.RLock()
      sleep()
      a.RUnlock()
      sleep()
      a.RUnlock()
    }

    lock := func() {
      defer wg.Done()
      sleep()
      a.Lock()
      sleep()
      a.Unlock()
    }

    for i := 0; i < 10; i++ {
      wg.Add(2)
      go rlockTwice()
      go lock()
    }

-- with go version go1.9.3 darwin/amd64

What do you think?

@a-robinson
Copy link

Looks good, thanks @sasha-s!

@sasha-s
Copy link
Owner

sasha-s commented Feb 7, 2018

I updated the readme, thanks for bringing it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add deadlcok check of taking a lock twice in a goroutine
3 participants