Merge lp:~rogpeppe/tomb/update-api into lp:tomb

[Roger Peppe]

tomb: update API

- Rename Fatal and Fatalf to Kill and Killf.

- The Stop error value is gone. Use nil now.

- Err will now return ErrStillRunning if the goroutine isn't dead
  yet, which means a nil return reflects an actual nil Kill call.

- Make zero-value of Tomb good to use, and as a consequence, make
  Dead and Dying into methods rather than fields.

https://codereview.appspot.com/5755055/

[Roger Peppe (rogpeppe)]

Reviewers: mp+96158_code.launchpad.net,

Message:
Please take a look.

Description:
As discussed on IRC:

Rename Fatal and Fatalf to Kill and Killf.

Make zero-value of Tomb good to use, and as a consequence, make Dead
and Dying into methods rather than fields.

https://code.launchpad.net/~rogpeppe/tomb/update-api/+merge/96158

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/5755055/

Affected files:
   M tomb.go
   M tomb_test.go

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/5755055/

[lp:~rogpeppe/tomb/update-api]

15. By Roger Peppe on 2012-03-06

remove references to stop

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/5755055/diff/3001/tomb.go
File tomb.go (right):

https://codereview.appspot.com/5755055/diff/3001/tomb.go#newcode44
tomb.go:44: // created or already alive. Once Kill is called with an
s/Kill/Kill or Killf/, as before please.

https://codereview.appspot.com/5755055/diff/3001/tomb.go#newcode80
tomb.go:80: if t.state == uninitialized {
if t.dead == nil {
     t.dead = make(...)
     t.dying = make(...)
     t.reason = ErrStillRunning
}

We don't need "state".

https://codereview.appspot.com/5755055/diff/3001/tomb.go#newcode130
tomb.go:130: reason = errCleanKill
nil itself is errCleanKill. We don't need a second way to flag this.

https://codereview.appspot.com/5755055/diff/3001/tomb.go#newcode136
tomb.go:136: if t.state == running {
if t.reason != ErrStillRunning

https://codereview.appspot.com/5755055/diff/3001/tomb.go#newcode162
tomb.go:162: }
The switch is unnecessary with the suggested change. Just return
t.reason at all times.

https://codereview.appspot.com/5755055/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/5755055/

[lp:~rogpeppe/tomb/update-api]

16. By Roger Peppe on 2012-03-06

simplifications for review

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/5755055/

[lp:~rogpeppe/tomb/update-api]

17. By Roger Peppe on 2012-03-06

modify comment

[Roger Peppe (rogpeppe)]

much better with those changes, thanks.
PTAL.

https://codereview.appspot.com/5755055/diff/3001/tomb.go
File tomb.go (right):

https://codereview.appspot.com/5755055/diff/3001/tomb.go#newcode44
tomb.go:44: // created or already alive. Once Kill is called with an
On 2012/03/06 16:07:35, niemeyer wrote:
> s/Kill/Kill or Killf/, as before please.

Done.

https://codereview.appspot.com/5755055/diff/3001/tomb.go#newcode80
tomb.go:80: if t.state == uninitialized {
On 2012/03/06 16:07:35, niemeyer wrote:
> if t.dead == nil {
> t.dead = make(...)
> t.dying = make(...)
> t.reason = ErrStillRunning
> }

> We don't need "state".

Done.

https://codereview.appspot.com/5755055/diff/3001/tomb.go#newcode130
tomb.go:130: reason = errCleanKill
On 2012/03/06 16:07:35, niemeyer wrote:
> nil itself is errCleanKill. We don't need a second way to flag this.

Done.

https://codereview.appspot.com/5755055/diff/3001/tomb.go#newcode136
tomb.go:136: if t.state == running {
On 2012/03/06 16:07:35, niemeyer wrote:
> if t.reason != ErrStillRunning

Done.

https://codereview.appspot.com/5755055/diff/3001/tomb.go#newcode162
tomb.go:162: }
On 2012/03/06 16:07:35, niemeyer wrote:
> The switch is unnecessary with the suggested change. Just return
t.reason at all
> times.

Done.

https://codereview.appspot.com/5755055/

[Gustavo Niemeyer (niemeyer)]

A couple of trivials, and should be ready to submit.

https://codereview.appspot.com/5755055/diff/6003/tomb.go
File tomb.go (right):

https://codereview.appspot.com/5755055/diff/6003/tomb.go#newcode106
tomb.go:106: t.init()
init isn't needed here.

https://codereview.appspot.com/5755055/diff/6003/tomb_test.go
File tomb_test.go (right):

https://codereview.appspot.com/5755055/diff/6003/tomb_test.go#newcode19
tomb_test.go:19: // the Kill reason flags the goroutine as dying
s/Kill/nil/

https://codereview.appspot.com/5755055/diff/6003/tomb_test.go#newcode24
tomb_test.go:24: // a non-Kill reason now will override Kill
s/Kill/nil/g

https://codereview.appspot.com/5755055/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/5755055/

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/5755055/diff/6003/tomb.go
File tomb.go (right):

https://codereview.appspot.com/5755055/diff/6003/tomb.go#newcode106
tomb.go:106: t.init()
On 2012/03/06 17:06:55, niemeyer wrote:
> init isn't needed here.

Done.

https://codereview.appspot.com/5755055/diff/6003/tomb_test.go
File tomb_test.go (right):

https://codereview.appspot.com/5755055/diff/6003/tomb_test.go#newcode19
tomb_test.go:19: // the Kill reason flags the goroutine as dying
On 2012/03/06 17:06:55, niemeyer wrote:
> s/Kill/nil/

Done.

https://codereview.appspot.com/5755055/diff/6003/tomb_test.go#newcode24
tomb_test.go:24: // a non-Kill reason now will override Kill
On 2012/03/06 17:06:55, niemeyer wrote:
> s/Kill/nil/g

Done.

https://codereview.appspot.com/5755055/

[lp:~rogpeppe/tomb/update-api]

18. By Roger Peppe on 2012-03-06

trivials for review

[Gustavo Niemeyer (niemeyer)]

*** Submitted:

tomb: update API

- Rename Fatal and Fatalf to Kill and Killf.

- The Stop error value is gone. Use nil now.

- Err will now return ErrStillRunning if the goroutine isn't dead
   yet, which means a nil return reflects an actual nil Kill call.

- Make zero-value of Tomb good to use, and as a consequence, make
   Dead and Dying into methods rather than fields.

R=niemeyer
CC=
https://codereview.appspot.com/5755055

https://codereview.appspot.com/5755055/

=== modified file 'tomb.go'
--- tomb.go	2012-01-18 00:28:24 +0000
+++ tomb.go	2012-03-06 17:23:19 +0000
@@ -34,15 +34,14 @@
 import (
 	"errors"
 	"fmt"
-
 	"sync"
 )
 
 // A Tomb tracks the lifecycle of a goroutine as alive, dying or dead,
 // and the reason for its death.
 //
-// The initial state of a Tomb informs that a goroutine is about to be
-// created or already alive. Once Fatal or Fatalf is called with an
+// The zero value of a Tomb assumes that a goroutine is about to be
+// created or already alive. Once Kill or Killf is called with an
 // argument that informs the reason for death, the goroutine is in
 // a dying state and is expected to terminate soon. Right before the
 // goroutine function or method returns, Done must be called to inform
@@ -61,74 +60,85 @@
 	m      sync.Mutex
 	dying  chan struct{}
 	dead   chan struct{}
-	Dying  <-chan struct{}
-	Dead   <-chan struct{}
 	reason error
 }
 
-// The Stop error is used as a reason for a goroutine to stop cleanly.
-var Stop = errors.New("clean stop")
-
-// New creates a new Tomb to track the lifecycle of a goroutine
-// that is already alive or about to be created.
-func New() *Tomb {
-	t := &Tomb{dying: make(chan struct{}), dead: make(chan struct{})}
-	t.Dying = t.dying
-	t.Dead = t.dead
-	return t
+var ErrStillRunning = errors.New("tomb: goroutine is still running")
+
+func (t *Tomb) init() {
+	t.m.Lock()
+	if t.dead == nil {
+		t.dead = make(chan struct{})
+		t.dying = make(chan struct{})
+		t.reason = ErrStillRunning
+	}
+	t.m.Unlock()
+}
+
+// Dead returns the channel that can be used to wait
+// until t.Done has been called.
+func (t *Tomb) Dead() <-chan struct{} {
+	t.init()
+	return t.dead
+}
+
+// Dying returns the channel that can be used to wait
+// until t.Kill or t.Done has been called.
+func (t *Tomb) Dying() <-chan struct{} {
+	t.init()
+	return t.dying
 }
 
 // Wait blocks until the goroutine is in a dead state and returns the
-// reason for its death. If the reason is Stop, nil is returned.
+// reason for its death.
 func (t *Tomb) Wait() error {
-	<-t.Dead
-	if t.reason == Stop {
-		return nil
-	}
+	t.init()
+	<-t.dead
 	return t.reason
 }
 
 // Done flags the goroutine as dead, and should be called a single time
 // right before the goroutine function or method returns.
 // If the goroutine was not already in a dying state before Done is
-// called, it will be flagged as dying and dead at once with Stop as
-// the reason for death.
+// called, it will be flagged as dying and dead at once with no
+// error.
 func (t *Tomb) Done() {
-	t.Fatal(Stop)
+	t.Kill(nil)
 	close(t.dead)
 }
 
-// Fatal flags the goroutine as dying for the given reason.
-// Fatal may be called multiple times, but only the first error is
+// Kill flags the goroutine as dying for the given reason.
+// Kill may be called multiple times, but only the first non-nil error is
 // recorded as the reason for termination.
-// The Stop value may be used to terminate a goroutine cleanly.
-func (t *Tomb) Fatal(reason error) {
-	if reason == nil {
-		panic("Fatal with nil reason")
-	}
+func (t *Tomb) Kill(reason error) {
+	t.init()
 	t.m.Lock()
-	if t.reason == nil || t.reason == Stop {
+	if t.reason == nil || t.reason == ErrStillRunning {
 		t.reason = reason
 	}
+	// If the receive on t.dying succeeds, then
+	// it can only be because we have already closed it.
+	// If it blocks, then we know that it needs to be closed.
 	select {
-	case <-t.Dying:
+	case <-t.dying:
 	default:
 		close(t.dying)
 	}
 	t.m.Unlock()
 }
 
-// Fatalf works like Fatal, but builds the reason providing the received
+// Killf works like Kill, but builds the reason providing the received
 // arguments to fmt.Errorf. The generated error is also returned.
-func (t *Tomb) Fatalf(format string, args ...interface{}) error {
-	err := fmt.Errorf(format, args...)
-	t.Fatal(err)
+func (t *Tomb) Killf(f string, a ...interface{}) error {
+	err := fmt.Errorf(f, a...)
+	t.Kill(err)
 	return err
 }
 
-// Err returns the reason for the goroutine death provided via Fatal
-// or Fatalf, or nil in case the goroutine is still alive.
+// Err returns the reason for the goroutine death provided via Kill
+// or Killf, or ErrStillRunning when the goroutine is still alive.
 func (t *Tomb) Err() (reason error) {
+	t.init()
 	t.m.Lock()
 	reason = t.reason
 	t.m.Unlock()

=== modified file 'tomb_test.go'
--- tomb_test.go	2012-01-18 00:28:24 +0000
+++ tomb_test.go	2012-03-06 17:23:19 +0000
@@ -4,48 +4,45 @@
 	"errors"
 	"launchpad.net/tomb"
 	"reflect"
-
 	"testing"
 )
 
 func TestNewTomb(t *testing.T) {
-	tb := tomb.New()
-	testState(t, tb, false, false, nil)
+	tb := new(tomb.Tomb)
+	testState(t, tb, false, false, tomb.ErrStillRunning)
 
 	tb.Done()
-	testState(t, tb, true, true, tomb.Stop)
+	testState(t, tb, true, true, nil)
 }
 
-func TestFatal(t *testing.T) {
-	tb := tomb.New()
-
-	// the Stop reason flags the goroutine as dying
-	tb = tomb.New()
-	tb.Fatal(tomb.Stop)
-	testState(t, tb, true, false, tomb.Stop)
-
-	// a non-Stop reason now will override Stop
+func TestKill(t *testing.T) {
+	// a nil reason flags the goroutine as dying
+	tb := new(tomb.Tomb)
+	tb.Kill(nil)
+	testState(t, tb, true, false, nil)
+
+	// a non-nil reason now will override Kill
 	err := errors.New("some error")
-	tb.Fatal(err)
+	tb.Kill(err)
 	testState(t, tb, true, false, err)
 
 	// another non-nil reason won't replace the first one
-	tb.Fatal(errors.New("ignore me"))
+	tb.Kill(errors.New("ignore me"))
 	testState(t, tb, true, false, err)
 
 	tb.Done()
 	testState(t, tb, true, true, err)
 }
 
-func TestFatalf(t *testing.T) {
-	tb := tomb.New()
+func TestKillf(t *testing.T) {
+	tb := new(tomb.Tomb)
 
 	err := errors.New("BOOM")
-	tb.Fatalf("BO%s", "OM")
+	tb.Killf("BO%s", "OM")
 	testState(t, tb, true, false, err)
 
-	// another non-Stop reason won't replace the first one
-	tb.Fatalf("ignore me")
+	// another non-nil reason won't replace the first one
+	tb.Killf("ignore me")
 	testState(t, tb, true, false, err)
 
 	tb.Done()
@@ -54,7 +51,7 @@
 
 func testState(t *testing.T, tb *tomb.Tomb, wantDying, wantDead bool, wantErr error) {
 	select {
-	case <-tb.Dying:
+	case <-tb.Dying():
 		if !wantDying {
 			t.Error("<-Dying: should block")
 		}
@@ -65,7 +62,7 @@
 	}
 	seemsDead := false
 	select {
-	case <-tb.Dead:
+	case <-tb.Dead():
 		if !wantDead {
 			t.Error("<-Dead: should block")
 		}
@@ -80,11 +77,10 @@
 	}
 	if wantDead && seemsDead {
 		waitErr := tb.Wait()
-		if wantErr == tomb.Stop {
-			if waitErr != nil {
-				t.Errorf("Wait: want nil, got %#v", waitErr)
-			}
-		} else if !reflect.DeepEqual(waitErr, wantErr) {
+		switch {
+		case waitErr == tomb.ErrStillRunning:
+			t.Errorf("Wait should not return ErrStillRunning")
+		case !reflect.DeepEqual(waitErr, wantErr):
 			t.Errorf("Wait: want %#v, got %#v", wantErr, waitErr)
 		}
 	}