Merge lp:~niemeyer/tomb/err-dying into lp:tomb

Proposed by Gustavo Niemeyer
Status: Merged
Merged at revision: 14
Proposed branch: lp:~niemeyer/tomb/err-dying
Merge into: lp:tomb
Diff against target: 176 lines (+63/-16)
3 files modified
.lbox (+1/-0)
tomb.go (+26/-7)
tomb_test.go (+36/-9)
To merge this branch: bzr merge lp:~niemeyer/tomb/err-dying
Reviewer Review Type Date Requested Status
Gustavo Niemeyer Pending
Review via email: mp+100662@code.launchpad.net

Description of the change

Added ErrDying. Renamed ErrStillRunning to ErrStillAlive.

https://codereview.appspot.com/5981052/

To post a comment you must log in.
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :
Download full text (5.6 KiB)

Reviewers: mp+100662_code.launchpad.net,

Message:
Please take a look.

Description:
Added ErrDying. Renamed ErrStillRunning to ErrStillAlive.

https://code.launchpad.net/~niemeyer/tomb/err-dying/+merge/100662

(do not edit description out of merge proposal)

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

Affected files:
   A .lbox
   A [revision details]
   M tomb.go
   M tomb_test.go

Index: .lbox
=== added file '.lbox'
--- .lbox 1970-01-01 00:00:00 +0000
+++ .lbox 2012-04-03 17:55:15 +0000
@@ -0,0 +1,1 @@
+propose -cr -for=lp:tomb

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: tomb.go
=== modified file 'tomb.go'
--- tomb.go 2012-03-06 17:16:14 +0000
+++ tomb.go 2012-04-03 17:53:18 +0000
@@ -52,6 +52,12 @@
  // explicit blocking until the state changes, and also to selectively
  // unblock select statements accordingly.
  //
+// When the tomb state changes to dying and there's still logic going
+// on within the goroutine, nested functions and methos may choose to
+// return ErrDying as their error value, as this error won't alter the
+// tomb state if provied to the Kill method. This is a convenient way to
+// follow standard Go practices in the context of a dying tomb.
+//
  // For background and a detailed example, see the following blog post:
  //
  // http://blog.labix.org/2011/10/09/death-of-goroutines-under-control
@@ -63,14 +69,17 @@
   reason error
  }

-var ErrStillRunning = errors.New("tomb: goroutine is still running")
+var (
+ ErrStillAlive = errors.New("tomb: still alive")
+ ErrDying = errors.New("tomb: dying")
+)

  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.reason = ErrStillAlive
   }
   t.m.Unlock()
  }
@@ -108,12 +117,24 @@
  }

  // 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.
+// Kill may be called multiple times, but only the first
+// non-nil error is recorded as the reason for termination.
+//
+// If reason is ErrDying, the previous reason isn't replaced
+// even if it is nil. It's a runtime error to call Kill with
+// ErrDying if t is not in a dying state.
  func (t *Tomb) Kill(reason error) {
   t.init()
   t.m.Lock()
- if t.reason == nil || t.reason == ErrStillRunning {
+ if reason == ErrDying {
+ alive := t.reason == ErrStillAlive
+ t.m.Unlock()
+ if alive {
+ panic("tomb: Kill with ErrDying while still alive")
+ }
+ return
+ }
+ if t.reason == nil || t.reason == ErrStillAlive {
    t.reason = reason
   }
   // If the receive on t.dying succeeds, then
@@ -136,7 +157,7 @@
  }

  // Err returns the reason for the goroutine death provided via Kill
-// or Killf, or ErrStillRunning when the goroutine is still alive.
+// or Killf, or ErrStillAlive when the goroutine is still al...

Read more...

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

https://codereview.appspot.com/5981052/diff/1/tomb.go
File tomb.go (right):

https://codereview.appspot.com/5981052/diff/1/tomb.go#newcode84
tomb.go:84: t.m.Unlock()
On 2012/04/03 18:25:21, TheMue wrote:
> Why don't you defer t.m.Unlock() immediately after t.m.Lock()?

Because these are assignments. defer is unnecessary overhead here.

https://codereview.appspot.com/5981052/diff/1/tomb.go#newcode148
tomb.go:148: t.m.Unlock()
On 2012/04/03 18:25:21, TheMue wrote:
> Againg a defer would put Lock() and Unlock() together, line 131 could
be removed
> and the statement t.reason == ErrStillAlive could move into the if
statement.

Sounds good.

https://codereview.appspot.com/5981052/diff/1/tomb.go#newcode165
tomb.go:165: t.m.Unlock()
On 2012/04/03 18:25:21, TheMue wrote:
> Like above:

> t.init()
> t.m.Lock()
> defer t.m.Unlock()
> return t.reason

That's an assignment. Defer is unnecessary overhead here.

https://codereview.appspot.com/5981052/

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :
lp:~niemeyer/tomb/err-dying updated
16. By Gustavo Niemeyer

Use a defer in Kill, as suggested by Frank.

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

*** Submitted:

Added ErrDying. Renamed ErrStillRunning to ErrStillAlive.

R=TheMue
CC=
https://codereview.appspot.com/5981052

https://codereview.appspot.com/5981052/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file '.lbox'
2--- .lbox 1970-01-01 00:00:00 +0000
3+++ .lbox 2012-04-03 19:00:26 +0000
4@@ -0,0 +1,1 @@
5+propose -cr -for=lp:tomb
6
7=== modified file 'tomb.go'
8--- tomb.go 2012-03-06 17:16:14 +0000
9+++ tomb.go 2012-04-03 19:00:26 +0000
10@@ -52,6 +52,12 @@
11 // explicit blocking until the state changes, and also to selectively
12 // unblock select statements accordingly.
13 //
14+// When the tomb state changes to dying and there's still logic going
15+// on within the goroutine, nested functions and methos may choose to
16+// return ErrDying as their error value, as this error won't alter the
17+// tomb state if provied to the Kill method. This is a convenient way to
18+// follow standard Go practices in the context of a dying tomb.
19+//
20 // For background and a detailed example, see the following blog post:
21 //
22 // http://blog.labix.org/2011/10/09/death-of-goroutines-under-control
23@@ -63,14 +69,17 @@
24 reason error
25 }
26
27-var ErrStillRunning = errors.New("tomb: goroutine is still running")
28+var (
29+ ErrStillAlive = errors.New("tomb: still alive")
30+ ErrDying = errors.New("tomb: dying")
31+)
32
33 func (t *Tomb) init() {
34 t.m.Lock()
35 if t.dead == nil {
36 t.dead = make(chan struct{})
37 t.dying = make(chan struct{})
38- t.reason = ErrStillRunning
39+ t.reason = ErrStillAlive
40 }
41 t.m.Unlock()
42 }
43@@ -108,12 +117,23 @@
44 }
45
46 // Kill flags the goroutine as dying for the given reason.
47-// Kill may be called multiple times, but only the first non-nil error is
48-// recorded as the reason for termination.
49+// Kill may be called multiple times, but only the first
50+// non-nil error is recorded as the reason for termination.
51+//
52+// If reason is ErrDying, the previous reason isn't replaced
53+// even if it is nil. It's a runtime error to call Kill with
54+// ErrDying if t is not in a dying state.
55 func (t *Tomb) Kill(reason error) {
56 t.init()
57 t.m.Lock()
58- if t.reason == nil || t.reason == ErrStillRunning {
59+ defer t.m.Unlock()
60+ if reason == ErrDying {
61+ if t.reason == ErrStillAlive {
62+ panic("tomb: Kill with ErrDying while still alive")
63+ }
64+ return
65+ }
66+ if t.reason == nil || t.reason == ErrStillAlive {
67 t.reason = reason
68 }
69 // If the receive on t.dying succeeds, then
70@@ -124,7 +144,6 @@
71 default:
72 close(t.dying)
73 }
74- t.m.Unlock()
75 }
76
77 // Killf works like Kill, but builds the reason providing the received
78@@ -136,7 +155,7 @@
79 }
80
81 // Err returns the reason for the goroutine death provided via Kill
82-// or Killf, or ErrStillRunning when the goroutine is still alive.
83+// or Killf, or ErrStillAlive when the goroutine is still alive.
84 func (t *Tomb) Err() (reason error) {
85 t.init()
86 t.m.Lock()
87
88=== modified file 'tomb_test.go'
89--- tomb_test.go 2012-03-06 17:16:14 +0000
90+++ tomb_test.go 2012-04-03 19:00:26 +0000
91@@ -8,8 +8,8 @@
92 )
93
94 func TestNewTomb(t *testing.T) {
95- tb := new(tomb.Tomb)
96- testState(t, tb, false, false, tomb.ErrStillRunning)
97+ tb := &tomb.Tomb{}
98+ testState(t, tb, false, false, tomb.ErrStillAlive)
99
100 tb.Done()
101 testState(t, tb, true, true, nil)
102@@ -17,7 +17,7 @@
103
104 func TestKill(t *testing.T) {
105 // a nil reason flags the goroutine as dying
106- tb := new(tomb.Tomb)
107+ tb := &tomb.Tomb{}
108 tb.Kill(nil)
109 testState(t, tb, true, false, nil)
110
111@@ -35,10 +35,12 @@
112 }
113
114 func TestKillf(t *testing.T) {
115- tb := new(tomb.Tomb)
116+ tb := &tomb.Tomb{}
117
118- err := errors.New("BOOM")
119- tb.Killf("BO%s", "OM")
120+ err := tb.Killf("BO%s", "OM")
121+ if s := err.Error(); s != "BOOM" {
122+ t.Fatalf(`Killf("BO%s", "OM"): want "BOOM", got %q`, s)
123+ }
124 testState(t, tb, true, false, err)
125
126 // another non-nil reason won't replace the first one
127@@ -49,6 +51,31 @@
128 testState(t, tb, true, true, err)
129 }
130
131+func TestErrDying(t *testing.T) {
132+ // ErrDying being used properly, after a clean death.
133+ tb := &tomb.Tomb{}
134+ tb.Kill(nil)
135+ tb.Kill(tomb.ErrDying)
136+ testState(t, tb, true, false, nil)
137+
138+ // ErrDying being used properly, after an errorful death.
139+ err := errors.New("some error")
140+ tb.Kill(err)
141+ tb.Kill(tomb.ErrDying)
142+ testState(t, tb, true, false, err)
143+
144+ // ErrDying being use badly, with an alive tomb.
145+ tb = &tomb.Tomb{}
146+ defer func() {
147+ err := recover()
148+ if err != "tomb: Kill with ErrDying while still alive" {
149+ t.Fatalf("Wrong panic on Kill(ErrDying): %v", err)
150+ }
151+ testState(t, tb, false, false, tomb.ErrStillAlive)
152+ }()
153+ tb.Kill(tomb.ErrDying)
154+}
155+
156 func testState(t *testing.T, tb *tomb.Tomb, wantDying, wantDead bool, wantErr error) {
157 select {
158 case <-tb.Dying():
159@@ -72,14 +99,14 @@
160 t.Error("<-Dead: should not block")
161 }
162 }
163- if err := tb.Err(); !reflect.DeepEqual(err, wantErr) {
164+ if err := tb.Err(); err != wantErr {
165 t.Errorf("Err: want %#v, got %#v", wantErr, err)
166 }
167 if wantDead && seemsDead {
168 waitErr := tb.Wait()
169 switch {
170- case waitErr == tomb.ErrStillRunning:
171- t.Errorf("Wait should not return ErrStillRunning")
172+ case waitErr == tomb.ErrStillAlive:
173+ t.Errorf("Wait should not return ErrStillAlive")
174 case !reflect.DeepEqual(waitErr, wantErr):
175 t.Errorf("Wait: want %#v, got %#v", wantErr, waitErr)
176 }

Subscribers

People subscribed via source and target branches

to all changes: