sasha-s / go-deadlock

Online deadlock detection in go (golang)
Apache License 2.0
1.03k stars 75 forks source link

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

Closed smileusd closed 6 years ago

smileusd commented 6 years ago

fix #5


This change is Reviewable

sasha-s commented 6 years ago
:lgtm:

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


Comments from Reviewable

a-robinson commented 6 years ago

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 commented 6 years ago

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: https://github.com/sasha-s/go-deadlock/issues/4#issuecomment-325582598 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 commented 6 years ago

Looks good, thanks @sasha-s!

sasha-s commented 6 years ago

I updated the readme, thanks for bringing it up.