sync: throw, not panic, for unlock of unlocked mutex

The panic leaves the lock in an unusable state.
Trying to panic with a usable state makes the lock significantly
less efficient and scalable (see early CL patch sets and discussion).

Instead, use runtime.throw, which will crash the program directly.

In general throw is reserved for when the runtime detects truly
serious, unrecoverable problems. This problem is certainly serious,
and, without a significant performance hit, is unrecoverable.

Fixes #13879.

Change-Id: I41920d9e2317270c6f909957d195bd8b68177f8d
Reviewed-on: https://go-review.googlesource.com/31359
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/runtime/panic.go b/src/runtime/panic.go
index 9e108cc437b475ce7edb6e8628a2c7f96d30258a..f78e67f9bb2084c75d9c56204d68f5f2f80404b9 100644 (file)
--- a/src/runtime/panic.go
+++ b/src/runtime/panic.go
@@ -576,6 +576,11 @@ func dopanic(unused int) {
        *(*int)(nil) = 0
 }
 
+//go:linkname sync_throw sync.throw
+func sync_throw(s string) {
+       throw(s)
+}
+
 //go:nosplit
 func throw(s string) {
        print("fatal error: ", s, "\n")

diff --git a/src/sync/mutex.go b/src/sync/mutex.go
index 90892793f0a99cfa112e3f7f386cc24876aef695..717934344e4ed703333fa56daa2accb4290654dd 100644 (file)
--- a/src/sync/mutex.go
+++ b/src/sync/mutex.go
@@ -16,6 +16,8 @@ import (
        "unsafe"
 )
 
+func throw(string) // provided by runtime
+
 // A Mutex is a mutual exclusion lock.
 // Mutexes can be created as part of other structures;
 // the zero value for a Mutex is an unlocked mutex.
@@ -74,7 +76,7 @@ func (m *Mutex) Lock() {
                        // The goroutine has been woken from sleep,
                        // so we need to reset the flag in either case.
                        if new&mutexWoken == 0 {
-                               panic("sync: inconsistent mutex state")
+                               throw("sync: inconsistent mutex state")
                        }
                        new &^= mutexWoken
                }
@@ -108,7 +110,7 @@ func (m *Mutex) Unlock() {
        // Fast path: drop lock bit.
        new := atomic.AddInt32(&m.state, -mutexLocked)
        if (new+mutexLocked)&mutexLocked == 0 {
-               panic("sync: unlock of unlocked mutex")
+               throw("sync: unlock of unlocked mutex")
        }
 
        old := new

diff --git a/src/sync/mutex_test.go b/src/sync/mutex_test.go
index 91a4855cb1f5862702e815679812433ed3b9a8d8..fbfe4b77fe5f3e09effeb4151579d74695e80499 100644 (file)
--- a/src/sync/mutex_test.go
+++ b/src/sync/mutex_test.go
@@ -7,7 +7,12 @@
 package sync_test
 
 import (
+       "fmt"
+       "internal/testenv"
+       "os"
+       "os/exec"
        "runtime"
+       "strings"
        . "sync"
        "testing"
 )
@@ -71,17 +76,98 @@ func TestMutex(t *testing.T) {
        }
 }
 
-func TestMutexPanic(t *testing.T) {
-       defer func() {
-               if recover() == nil {
-                       t.Fatalf("unlock of unlocked mutex did not panic")
+var misuseTests = []struct {
+       name string
+       f    func()
+}{
+       {
+               "Mutex.Unlock",
+               func() {
+                       var mu Mutex
+                       mu.Unlock()
+               },
+       },
+       {
+               "Mutex.Unlock2",
+               func() {
+                       var mu Mutex
+                       mu.Lock()
+                       mu.Unlock()
+                       mu.Unlock()
+               },
+       },
+       {
+               "RWMutex.Unlock",
+               func() {
+                       var mu RWMutex
+                       mu.Unlock()
+               },
+       },
+       {
+               "RWMutex.Unlock2",
+               func() {
+                       var mu RWMutex
+                       mu.RLock()
+                       mu.Unlock()
+               },
+       },
+       {
+               "RWMutex.Unlock3",
+               func() {
+                       var mu RWMutex
+                       mu.Lock()
+                       mu.Unlock()
+                       mu.Unlock()
+               },
+       },
+       {
+               "RWMutex.RUnlock",
+               func() {
+                       var mu RWMutex
+                       mu.RUnlock()
+               },
+       },
+       {
+               "RWMutex.RUnlock2",
+               func() {
+                       var mu RWMutex
+                       mu.Lock()
+                       mu.RUnlock()
+               },
+       },
+       {
+               "RWMutex.RUnlock3",
+               func() {
+                       var mu RWMutex
+                       mu.RLock()
+                       mu.RUnlock()
+                       mu.RUnlock()
+               },
+       },
+}
+
+func init() {
+       if len(os.Args) == 3 && os.Args[1] == "TESTMISUSE" {
+               for _, test := range misuseTests {
+                       if test.name == os.Args[2] {
+                               test.f()
+                               fmt.Printf("test completed\n")
+                               os.Exit(0)
+                       }
                }
-       }()
+               fmt.Printf("unknown test\n")
+               os.Exit(0)
+       }
+}
 
-       var mu Mutex
-       mu.Lock()
-       mu.Unlock()
-       mu.Unlock()
+func TestMutexMisuse(t *testing.T) {
+       testenv.MustHaveExec(t)
+       for _, test := range misuseTests {
+               out, err := exec.Command(os.Args[0], "TESTMISUSE", test.name).CombinedOutput()
+               if err == nil || !strings.Contains(string(out), "unlocked") {
+                       t.Errorf("%s: did not find failure with message about unlocked lock: %s\n%s\n", test.name, err, out)
+               }
+       }
 }
 
 func BenchmarkMutexUncontended(b *testing.B) {

diff --git a/src/sync/rwmutex.go b/src/sync/rwmutex.go
index 6734360e37a5e50e42c73c8760ca392228ee7bab..71064eeeba3c2564699806187de7faf3b07f97e3 100644 (file)
--- a/src/sync/rwmutex.go
+++ b/src/sync/rwmutex.go
@@ -61,7 +61,7 @@ func (rw *RWMutex) RUnlock() {
        if r := atomic.AddInt32(&rw.readerCount, -1); r < 0 {
                if r+1 == 0 || r+1 == -rwmutexMaxReaders {
                        race.Enable()
-                       panic("sync: RUnlock of unlocked RWMutex")
+                       throw("sync: RUnlock of unlocked RWMutex")
                }
                // A writer is pending.
                if atomic.AddInt32(&rw.readerWait, -1) == 0 {
@@ -115,7 +115,7 @@ func (rw *RWMutex) Unlock() {
        r := atomic.AddInt32(&rw.readerCount, rwmutexMaxReaders)
        if r >= rwmutexMaxReaders {
                race.Enable()
-               panic("sync: Unlock of unlocked RWMutex")
+               throw("sync: Unlock of unlocked RWMutex")
        }
        // Unblock blocked readers, if any.
        for i := 0; i < int(r); i++ {

diff --git a/src/sync/rwmutex_test.go b/src/sync/rwmutex_test.go
index f625bc3a5854de10e6f06b77596db8724a4bdb60..0436f97239c7a16b823e6b05b4772bc134d064f0 100644 (file)
--- a/src/sync/rwmutex_test.go
+++ b/src/sync/rwmutex_test.go
@@ -155,48 +155,6 @@ func TestRLocker(t *testing.T) {
        }
 }
 
-func TestUnlockPanic(t *testing.T) {
-       defer func() {
-               if recover() == nil {
-                       t.Fatalf("unlock of unlocked RWMutex did not panic")
-               }
-       }()
-       var mu RWMutex
-       mu.Unlock()
-}
-
-func TestUnlockPanic2(t *testing.T) {
-       defer func() {
-               if recover() == nil {
-                       t.Fatalf("unlock of unlocked RWMutex did not panic")
-               }
-       }()
-       var mu RWMutex
-       mu.RLock()
-       mu.Unlock()
-}
-
-func TestRUnlockPanic(t *testing.T) {
-       defer func() {
-               if recover() == nil {
-                       t.Fatalf("read unlock of unlocked RWMutex did not panic")
-               }
-       }()
-       var mu RWMutex
-       mu.RUnlock()
-}
-
-func TestRUnlockPanic2(t *testing.T) {
-       defer func() {
-               if recover() == nil {
-                       t.Fatalf("read unlock of unlocked RWMutex did not panic")
-               }
-       }()
-       var mu RWMutex
-       mu.Lock()
-       mu.RUnlock()
-}
-
 func BenchmarkRWMutexUncontended(b *testing.B) {
        type PaddedRWMutex struct {
                RWMutex