Merge pull request #5528 from davecheney/fixedbugs/1588135

worker/terminationworker: fix test hang

Fixes LP 1588135

Fix a race on the ownership of the abort signal.

The `TestSignal` test attempts to see if sending `SIGABRT` to the test
binary itself will cause the worker to unblock itself and exit as
exited. However, as the original author discovered there is a race
between sending the signal and the worker spawned from `NewWorker`
registering a handler to catch `SIGABRT`. If the handler was not registered
in time the `SIGABRT` would cause the test process to exit.

To fix this problem they set up a `SIGABRT` handler in `SetUpTest` to ensure
that `SIGABRT` was always handled. However this meant that if `NewWorker`'s
handler was not registered in time to catch the signal, it would be
delivered to the channel registered by `SetUpTest`, causing the test binary
to hang.

The fix is to remove the `SetUpTest` workaround and ensure the handler is
always registered before spawning the loop goroutine so the caller is
assured that once `NewWorker` has returned, the handler is in place and it
is safe to send the signal.

This PR also removes unnecessary use of `testing.BaseSuite`.

(Review request: http://reviews.vapour.ws/r/4975/)

worker/terminationworker/worker.go

@@ -30,30 +30,30 @@ type terminationWorker struct {
 // TerminationSignal signal, and then exits
 // with worker.ErrTerminateAgent.
 func NewWorker() worker.Worker {
-	u := &terminationWorker{}
+	var w terminationWorker
+	c := make(chan os.Signal, 1)
+	signal.Notify(c, TerminationSignal)
 	go func() {
-		defer u.tomb.Done()
-		u.tomb.Kill(u.loop())
+		defer w.tomb.Done()
+		defer signal.Stop(c)
+		w.tomb.Kill(w.loop(c))
 	}()
-	return u
+	return &w
 }
 
-func (u *terminationWorker) Kill() {
-	u.tomb.Kill(nil)
+func (w *terminationWorker) Kill() {
+	w.tomb.Kill(nil)
 }
 
-func (u *terminationWorker) Wait() error {
-	return u.tomb.Wait()
+func (w *terminationWorker) Wait() error {
+	return w.tomb.Wait()
 }
 
-func (u *terminationWorker) loop() (err error) {
-	c := make(chan os.Signal, 1)
-	signal.Notify(c, TerminationSignal)
-	defer signal.Stop(c)
+func (w *terminationWorker) loop(c <-chan os.Signal) (err error) {
 	select {
 	case <-c:
 		return worker.ErrTerminateAgent
-	case <-u.tomb.Dying():
+	case <-w.tomb.Dying():
 		return tomb.ErrDying
 	}
 }

worker/terminationworker/worker_test.go

@@ -5,43 +5,28 @@ package terminationworker_test
 
 import (
 	"os"
-	"os/signal"
 	"runtime"
-	stdtesting "testing"
+	"testing"
 
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
-	"github.com/juju/juju/testing"
 	"github.com/juju/juju/worker"
 	"github.com/juju/juju/worker/terminationworker"
 )
 
-func TestPackage(t *stdtesting.T) {
+func TestPackage(t *testing.T) {
 	gc.TestingT(t)
 }
 
 var _ = gc.Suite(&TerminationWorkerSuite{})
 
 type TerminationWorkerSuite struct {
-	testing.BaseSuite
 	// c is a channel that will wait for the termination
 	// signal, to prevent signals terminating the process.
 	c chan os.Signal
 }
 
-func (s *TerminationWorkerSuite) SetUpTest(c *gc.C) {
-	s.BaseSuite.SetUpTest(c)
-	s.c = make(chan os.Signal, 1)
-	signal.Notify(s.c, terminationworker.TerminationSignal)
-}
-
-func (s *TerminationWorkerSuite) TearDownTest(c *gc.C) {
-	signal.Stop(s.c)
-	close(s.c)
-	s.BaseSuite.TearDownTest(c)
-}
-
 func (s *TerminationWorkerSuite) TestStartStop(c *gc.C) {
 	w := terminationworker.NewWorker()
 	w.Kill()