os/signal: lazily start signal watch loop only on Notify

By lazily starting the signal watch loop only on Notify,
we are able to have deadlock detection even when
"os/signal" is imported.

Thanks to Ian Lance Taylor for the solution and discussion.

With this change in, fix a runtime gorountine count test that
assumed that os/signal.init would unconditionally start the
signal watching goroutine, but alas no more.

Fixes #21576.

Change-Id: I6eecf82a887f59f2ec8897f1bcd67ca311ca42ff
Reviewed-on: https://go-review.googlesource.com/c/go/+/101036
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/os/signal/signal.go b/src/os/signal/signal.go
index a0eba0d50fb45785d6d3e8635def7e28de6b41e9..136dd9cc97f504b19ebd24ed2e2dc88e0e93cde5 100644 (file)
--- a/src/os/signal/signal.go
+++ b/src/os/signal/signal.go
@@ -92,6 +92,15 @@ func Ignored(sig os.Signal) bool {
        return sn >= 0 && signalIgnored(sn)
 }
 
+var (
+       // watchSignalLoopOnce guards calling the conditionally
+       // initialized watchSignalLoop. If watchSignalLoop is non-nil,
+       // it will be run in a goroutine lazily once Notify is invoked.
+       // See Issue 21576.
+       watchSignalLoopOnce sync.Once
+       watchSignalLoop     func()
+)
+
 // Notify causes package signal to relay incoming signals to c.
 // If no signals are provided, all incoming signals will be relayed to c.
 // Otherwise, just the provided signals will.
@@ -113,6 +122,12 @@ func Notify(c chan<- os.Signal, sig ...os.Signal) {
                panic("os/signal: Notify using nil channel")
        }
 
+       watchSignalLoopOnce.Do(func() {
+               if watchSignalLoop != nil {
+                       go watchSignalLoop()
+               }
+       })
+
        handlers.Lock()
        defer handlers.Unlock()
 

diff --git a/src/os/signal/signal_plan9.go b/src/os/signal/signal_plan9.go
index a1eb68855ed277325caa69021cb12f3ea73846d8..8408607c7fa37879dc65de03e3ce496b8781b699 100644 (file)
--- a/src/os/signal/signal_plan9.go
+++ b/src/os/signal/signal_plan9.go
@@ -20,7 +20,8 @@ func signal_recv() string
 
 func init() {
        signal_enable(0) // first call - initialize
-       go loop()
+
+       watchSignalLoop = loop
 }
 
 func loop() {

diff --git a/src/os/signal/signal_unix.go b/src/os/signal/signal_unix.go
index 7fa634f15ad83a006167a5d449ecaf3d113662cc..0bbf41bfde5f579c2de88ce53d09e871dea5b3cc 100644 (file)
--- a/src/os/signal/signal_unix.go
+++ b/src/os/signal/signal_unix.go
@@ -26,7 +26,8 @@ func loop() {
 
 func init() {
        signal_enable(0) // first call - initialize
-       go loop()
+
+       watchSignalLoop = loop
 }
 
 const (

diff --git a/src/runtime/testdata/testprogcgo/numgoroutine.go b/src/runtime/testdata/testprogcgo/numgoroutine.go
index 12fda49a1316c4fae98fcdb9bc64f557e3903233..5bdfe52ed41b4d64a99f0ad7b3305f23c3afc43f 100644 (file)
--- a/src/runtime/testdata/testprogcgo/numgoroutine.go
+++ b/src/runtime/testdata/testprogcgo/numgoroutine.go
@@ -41,13 +41,6 @@ func NumGoroutine() {
        // Test that there are just the expected number of goroutines
        // running. Specifically, test that the spare M's goroutine
        // doesn't show up.
-       //
-       // On non-Windows platforms there's a signal handling thread
-       // started by os/signal.init in addition to the main
-       // goroutine.
-       if runtime.GOOS != "windows" {
-               baseGoroutines = 1
-       }
        if _, ok := checkNumGoroutine("first", 1+baseGoroutines); !ok {
                return
        }

diff --git a/test/fixedbugs/issue21576.go b/test/fixedbugs/issue21576.go
new file mode 100644 (file)
index 0000000..79baec9
--- /dev/null
+++ b/test/fixedbugs/issue21576.go
@@ -0,0 +1,60 @@
+// +build !nacl,!js
+// run
+
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+//
+// Ensure that deadlock detection can still
+// run even with an import of "_ os/signal".
+
+package main
+
+import (
+       "bytes"
+       "context"
+       "io/ioutil"
+       "log"
+       "os"
+       "os/exec"
+       "path/filepath"
+       "time"
+)
+
+const prog = `
+package main
+
+import _ "os/signal"
+
+func main() {
+  c := make(chan int)
+  c <- 1
+}
+`
+
+func main() {
+       dir, err := ioutil.TempDir("", "21576")
+       if err != nil {
+               log.Fatal(err)
+       }
+       defer os.RemoveAll(dir)
+
+       file := filepath.Join(dir, "main.go")
+       if err := ioutil.WriteFile(file, []byte(prog), 0655); err != nil {
+               log.Fatalf("Write error %v", err)
+       }
+
+       ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+       defer cancel()
+
+       cmd := exec.CommandContext(ctx, "go", "run", file)
+       output, err := cmd.CombinedOutput()
+       if err == nil {
+               log.Fatalf("Passed, expected an error")
+       }
+
+       want := []byte("fatal error: all goroutines are asleep - deadlock!")
+       if !bytes.Contains(output, want) {
+               log.Fatalf("Unmatched error message %q:\nin\n%s", want, output)
+       }
+}