code review 4180044: sync: check Unlock of unlocked Mutex

sync: check Unlock of unlocked Mutex

[rsc, 2011-02-11 21:37:08]

Hello r (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg

[adg, 2011-02-11 22:15:16]

LGTM

> +               // (or there are 4 billion goroutines waiting)

hah

On 11 February 2011 16:37,  <rsc@golang.org> wrote:
> Reviewers: r,
>
> Message:
> Hello r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
> sync: check Unlock of unlocked Mutex
>
> Please review this at http://codereview.appspot.com/4180044/
>
> Affected files:
>  M src/pkg/sync/mutex.go
>  M src/pkg/sync/mutex_test.go
>
>
> Index: src/pkg/sync/mutex.go
> ===================================================================
> --- a/src/pkg/sync/mutex.go
> +++ b/src/pkg/sync/mutex.go
> @@ -53,9 +53,14 @@
>  // It is allowed for one goroutine to lock a Mutex and then
>  // arrange for another goroutine to unlock it.
>  func (m *Mutex) Unlock() {
> -       if xadd(&m.key, -1) == 0 {
> +       switch v := xadd(&m.key, -1); {
> +       case v == 0:
>                // changed from 1 to 0; no contention
>                return
> +       case int32(v) == -1:
> +               // changed from 0 to -1: wasn't locked
> +               // (or there are 4 billion goroutines waiting)
> +               panic("sync: unlock of unlocked mutex")
>        }
>        runtime.Semrelease(&m.sema)
>  }
> Index: src/pkg/sync/mutex_test.go
> ===================================================================
> --- a/src/pkg/sync/mutex_test.go
> +++ b/src/pkg/sync/mutex_test.go
> @@ -89,3 +89,16 @@
>        <-c
>        <-c
>  }
> +
> +func TestMutexPanic(t *testing.T) {
> +       defer func() {
> +               if recover() == nil {
> +                       t.Fatalf("unlock of unlocked mutex did not panic")
> +               }
> +       }()
> +
> +       var mu Mutex
> +       mu.Lock()
> +       mu.Unlock()
> +       mu.Unlock()
> +}
>
>
>

[rsc, 2011-02-11 22:16:06]

i wrote the code to check in Lock for the 2^32'th goroutine
but that seemed like overkill so i took it out

[r, 2011-02-11 22:42:56]

LGTM

[rsc, 2011-02-11 22:47:21]

*** Submitted as a8d5d8783edd ***

sync: check Unlock of unlocked Mutex

R=r, adg
CC=golang-dev
http://codereview.appspot.com/4180044