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>

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()
 

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() {

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 (

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
 	}

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)
+	}
+}