veyron/lib/signals: fixing logic to remove STOP from signals passed to
ShutdownOnSignals. Add tests to ensure correctness.
Change-Id: Ia8cfbff9a39d2274d26f39519e76fdc9fa84f878
diff --git a/lib/signals/signals.go b/lib/signals/signals.go
index e9b88fe..75ed785 100644
--- a/lib/signals/signals.go
+++ b/lib/signals/signals.go
@@ -39,28 +39,28 @@
// signals we get on account of the channel being full.
ch := make(chan os.Signal, 2)
sawStop := false
- for i := 0; i < len(signals); {
- if s := signals[i]; s == STOP {
- signals = append(signals[:i], signals[i+1:]...)
- if sawStop {
- continue
+ var signalsNoStop []os.Signal
+ for _, s := range signals {
+ switch s {
+ case STOP:
+ if !sawStop {
+ sawStop = true
+ if r := rt.R(); r != nil {
+ stopWaiter := make(chan string, 1)
+ r.WaitForStop(stopWaiter)
+ go func() {
+ for {
+ ch <- stopSignal(<-stopWaiter)
+ }
+ }()
+ }
}
- sawStop = true
- if r := rt.R(); r != nil {
- stopWaiter := make(chan string, 1)
- r.WaitForStop(stopWaiter)
- go func() {
- for {
- ch <- stopSignal(<-stopWaiter)
- }
- }()
- }
- } else {
- i++
+ default:
+ signalsNoStop = append(signalsNoStop, s)
}
}
- if len(signals) > 0 {
- signal.Notify(ch, signals...)
+ if len(signalsNoStop) > 0 {
+ signal.Notify(ch, signalsNoStop...)
}
// At least a buffer of length one so that we don't block on ret <- sig.
ret := make(chan os.Signal, 1)
diff --git a/lib/signals/signals_test.go b/lib/signals/signals_test.go
index d0a6b85..9303a79 100644
--- a/lib/signals/signals_test.go
+++ b/lib/signals/signals_test.go
@@ -21,6 +21,7 @@
func init() {
blackbox.CommandTable["handleDefaults"] = handleDefaults
blackbox.CommandTable["handleCustom"] = handleCustom
+ blackbox.CommandTable["handleCustomWithStop"] = handleCustomWithStop
blackbox.CommandTable["handleDefaultsIgnoreChan"] = handleDefaultsIgnoreChan
}
@@ -54,6 +55,10 @@
program(syscall.SIGABRT)
}
+func handleCustomWithStop([]string) {
+ program(STOP, syscall.SIGABRT, syscall.SIGHUP)
+}
+
func handleDefaultsIgnoreChan([]string) {
defer rt.Init().Shutdown()
closeStopLoop := make(chan struct{})
@@ -111,6 +116,20 @@
c.ExpectEOFAndWait()
}
+// TestCleanShutdownStopCustom verifies that sending a stop comamnd to a child
+// that handles stop command as part of a custom set of signals handled, causes
+// the child to shut down cleanly.
+func TestCleanShutdownStopCustom(t *testing.T) {
+ c := blackbox.HelperCommand(t, "handleCustomWithStop")
+ defer c.Cleanup()
+ c.Cmd.Start()
+ c.Expect("ready")
+ c.WriteLine("stop")
+ c.Expect(fmt.Sprintf("received signal %s", veyron2.LocalStop))
+ c.WriteLine("close")
+ c.ExpectEOFAndWait()
+}
+
// TestStopNoHandler verifies that sending a stop command to a child that does
// not handle stop commands causes the child to exit immediately.
func TestStopNoHandler(t *testing.T) {
@@ -210,3 +229,33 @@
c.WriteLine("close")
c.ExpectEOFAndWait()
}
+
+// TestHandlerCustomSignalWithStop verifies that sending a custom stop signal
+// to a server that listens for that signal causes the server to shut down
+// cleanly, even when a STOP signal is also among the handled signals.
+func TestHandlerCustomSignalWithStop(t *testing.T) {
+ for _, signal := range([]syscall.Signal{syscall.SIGABRT, syscall.SIGHUP}) {
+ c := blackbox.HelperCommand(t, "handleCustomWithStop")
+ c.Cmd.Start()
+ c.Expect("ready")
+ checkSignalIsNotDefault(t, signal)
+ syscall.Kill(c.Cmd.Process.Pid, signal)
+ c.Expect(fmt.Sprintf("received signal %s", signal))
+ c.WriteLine("close")
+ c.ExpectEOFAndWait()
+ c.Cleanup()
+ }
+}
+
+// TestParseSignalsList verifies that ShutdownOnSignals correctly interprets
+// the input list of signals.
+func TestParseSignalsList(t *testing.T) {
+ list := []os.Signal{STOP, syscall.SIGTERM}
+ ShutdownOnSignals(list...)
+ if !isSignalInSet(syscall.SIGTERM, list) {
+ t.Errorf("signal %s not in signal set, as expected: %v", syscall.SIGTERM, list)
+ }
+ if !isSignalInSet(STOP, list) {
+ t.Errorf("signal %s not in signal set, as expected: %v", STOP, list)
+ }
+}
diff --git a/runtimes/google/rt/mgmt_test.go b/runtimes/google/rt/mgmt_test.go
index 91817f5..bcad19d 100644
--- a/runtimes/google/rt/mgmt_test.go
+++ b/runtimes/google/rt/mgmt_test.go
@@ -82,7 +82,7 @@
os.Exit(42) // This should not be reached.
}
-// TestNoWaiters verifies that the child process exits in the absense of any
+// TestNoWaiters verifies that the child process exits in the absence of any
// wait channel being registered with its runtime.
func TestNoWaiters(t *testing.T) {
c := blackbox.HelperCommand(t, "noWaiters")
