Skip to content

Commit 026896a

Browse files
cpuguy83cpuguy83
committed
Make Wait() async
In all of the examples, its recommended to call `Wait()` before starting a process/task. Since `Wait()` is a blocking call, this means it must be called from a goroutine like so: ```go statusC := make(chan uint32) go func() { status, err := task.Wait(ctx) if err != nil { // handle async err } statusC <- status }() task.Start(ctx) <-statusC ``` This means there is a race here where there is no guarentee when the goroutine is going to be scheduled, and even a bit more since this requires an RPC call to be made. In addition, this code is very messy and a common pattern for any caller using Wait+Start. Instead, this changes `Wait()` to use an async model having `Wait()` return a channel instead of the code itself. This ensures that when `Wait()` returns that the client has a handle on the event stream (already made the RPC request) before returning and reduces any sort of race to how the stream is handled by grpc since we can't guarentee that we have a goroutine running and blocked on `Recv()`. Making `Wait()` async also cleans up the code in the caller drastically: ```go statusC, err := task.Wait(ctx) if err != nil { return err } task.Start(ctx) status := <-statusC if status.Err != nil { return err } ``` No more spinning up goroutines and more natural error handling for the caller. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
1 parent 89da512 commit 026896a

File tree

12 files changed

+353
-356
lines changed

12 files changed

+353
-356
lines changed

‎cmd/containerd-stress/main.go‎

Lines changed: 13 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -208,30 +208,22 @@ func (w *worker) runContainer(ctx context.Context, id string) error {
208208
return err
209209
}
210210
defer task.Delete(ctx, containerd.WithProcessKill)
211-
var (
212-
start sync.WaitGroup
213-
status = make(chan uint32, 1)
214-
)
215-
start.Add(1)
216-
go func() {
217-
start.Done()
218-
s, err := task.Wait(w.waitContext)
219-
if err != nil {
220-
if err == context.DeadlineExceeded ||
221-
err == context.Canceled {
222-
close(status)
223-
return
224-
}
225-
w.failures++
226-
logrus.WithError(err).Errorf("wait task %s", id)
227-
}
228-
status <- s
229-
}()
230-
start.Wait()
211+
212+
statusC, err := task.Wait(ctx)
213+
if err != nil {
214+
return err
215+
}
216+
231217
if err := task.Start(ctx); err != nil {
232218
return err
233219
}
234-
<-status
220+
status := <-statusC
221+
if status.Err != nil {
222+
if status.Err == context.DeadlineExceeded || status.Err == context.Canceled {
223+
return nil
224+
}
225+
w.failures++
226+
}
235227
return nil
236228
}
237229

‎cmd/ctr/attach.go‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,12 @@ var taskAttachCommand = cli.Command{
4444
return err
4545
}
4646
defer task.Delete(ctx)
47+
48+
statusC, err := task.Wait(ctx)
49+
if err != nil {
50+
return err
51+
}
52+
4753
if tty {
4854
if err := handleConsoleResize(ctx, task, con); err != nil {
4955
logrus.WithError(err).Error("console resize")
@@ -52,12 +58,13 @@ var taskAttachCommand = cli.Command{
5258
sigc := forwardAllSignals(ctx, task)
5359
defer stopCatch(sigc)
5460
}
55-
status, err := task.Wait(ctx)
56-
if err != nil {
61+
62+
ec := <-statusC
63+
if ec.Err != nil {
5764
return err
5865
}
59-
if status != 0 {
60-
return cli.NewExitError("", int(status))
66+
if ec.Code != 0 {
67+
return cli.NewExitError("", int(ec.Code))
6168
}
6269
return nil
6370
},

‎cmd/ctr/exec.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -70,14 +70,11 @@ var taskExecCommand = cli.Command{
7070
}
7171
defer process.Delete(ctx)
7272

73-
statusC := make(chan uint32, 1)
74-
go func() {
75-
status, err := process.Wait(ctx)
76-
if err != nil {
77-
logrus.WithError(err).Error("wait process")
78-
}
79-
statusC <- status
80-
}()
73+
statusC, err := task.Wait(ctx)
74+
if err != nil {
75+
return err
76+
}
77+
8178
var con console.Console
8279
if tty {
8380
con = console.Current()
@@ -98,8 +95,11 @@ var taskExecCommand = cli.Command{
9895
defer stopCatch(sigc)
9996
}
10097
status := <-statusC
101-
if status != 0 {
102-
return cli.NewExitError("", int(status))
98+
if status.Err != nil {
99+
return status.Err
100+
}
101+
if status.Code != 0 {
102+
return cli.NewExitError("", int(status.Code))
103103
}
104104
return nil
105105
},

‎cmd/ctr/run.go‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -129,14 +129,11 @@ var runCommand = cli.Command{
129129
}
130130
defer task.Delete(ctx)
131131

132-
statusC := make(chan uint32, 1)
133-
go func() {
134-
status, err := task.Wait(ctx)
135-
if err != nil {
136-
logrus.WithError(err).Error("wait process")
137-
}
138-
statusC <- status
139-
}()
132+
statusC, err := task.Wait(ctx)
133+
if err != nil {
134+
return err
135+
}
136+
140137
var con console.Console
141138
if tty {
142139
con = console.Current()
@@ -158,11 +155,15 @@ var runCommand = cli.Command{
158155
}
159156

160157
status := <-statusC
158+
if status.Err != nil {
159+
return status.Err
160+
}
161+
161162
if _, err := task.Delete(ctx); err != nil {
162163
return err
163164
}
164-
if status != 0 {
165-
return cli.NewExitError("", int(status))
165+
if status.Code != 0 {
166+
return cli.NewExitError("", int(status.Code))
166167
}
167168
return nil
168169
},

‎cmd/ctr/start.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,11 @@ var taskStartCommand = cli.Command{
4747
}
4848
defer task.Delete(ctx)
4949

50-
statusC := make(chan uint32, 1)
51-
go func() {
52-
status, err := task.Wait(ctx)
53-
if err != nil {
54-
logrus.WithError(err).Error("wait process")
55-
}
56-
statusC <- status
57-
}()
50+
statusC, err := task.Wait(ctx)
51+
if err != nil {
52+
return err
53+
}
54+
5855
var con console.Console
5956
if tty {
6057
con = console.Current()
@@ -76,11 +73,14 @@ var taskStartCommand = cli.Command{
7673
}
7774

7875
status := <-statusC
76+
if status.Err != nil {
77+
return err
78+
}
7979
if _, err := task.Delete(ctx); err != nil {
8080
return err
8181
}
82-
if status != 0 {
83-
return cli.NewExitError("", int(status))
82+
if status.Code != 0 {
83+
return cli.NewExitError("", int(status.Code))
8484
}
8585
return nil
8686
},

‎container_checkpoint_test.go‎

Lines changed: 25 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,11 @@ func TestCheckpointRestore(t *testing.T) {
4747
}
4848
defer task.Delete(ctx)
4949

50-
statusC := make(chan uint32, 1)
51-
go func() {
52-
status, err := task.Wait(ctx)
53-
if err != nil {
54-
t.Error(err)
55-
}
56-
statusC <- status
57-
}()
50+
statusC, err := task.Wait(ctx)
51+
if err != nil {
52+
t.Error(err)
53+
return
54+
}
5855

5956
if err := task.Start(ctx); err != nil {
6057
t.Error(err)
@@ -79,13 +76,11 @@ func TestCheckpointRestore(t *testing.T) {
7976
}
8077
defer task.Delete(ctx)
8178

82-
go func() {
83-
status, err := task.Wait(ctx)
84-
if err != nil {
85-
t.Error(err)
86-
}
87-
statusC <- status
88-
}()
79+
statusC, err = task.Wait(ctx)
80+
if err != nil {
81+
t.Error(err)
82+
return
83+
}
8984

9085
if err := task.Start(ctx); err != nil {
9186
t.Error(err)
@@ -137,14 +132,11 @@ func TestCheckpointRestoreNewContainer(t *testing.T) {
137132
}
138133
defer task.Delete(ctx)
139134

140-
statusC := make(chan uint32, 1)
141-
go func() {
142-
status, err := task.Wait(ctx)
143-
if err != nil {
144-
t.Error(err)
145-
}
146-
statusC <- status
147-
}()
135+
statusC, err := task.Wait(ctx)
136+
if err != nil {
137+
t.Error(err)
138+
return
139+
}
148140

149141
if err := task.Start(ctx); err != nil {
150142
t.Error(err)
@@ -177,13 +169,11 @@ func TestCheckpointRestoreNewContainer(t *testing.T) {
177169
}
178170
defer task.Delete(ctx)
179171

180-
go func() {
181-
status, err := task.Wait(ctx)
182-
if err != nil {
183-
t.Error(err)
184-
}
185-
statusC <- status
186-
}()
172+
statusC, err = task.Wait(ctx)
173+
if err != nil {
174+
t.Error(err)
175+
return
176+
}
187177

188178
if err := task.Start(ctx); err != nil {
189179
t.Error(err)
@@ -240,14 +230,11 @@ func TestCheckpointLeaveRunning(t *testing.T) {
240230
}
241231
defer task.Delete(ctx)
242232

243-
statusC := make(chan uint32, 1)
244-
go func() {
245-
status, err := task.Wait(ctx)
246-
if err != nil {
247-
t.Error(err)
248-
}
249-
statusC <- status
250-
}()
233+
statusC, err := task.Wait(ctx)
234+
if err != nil {
235+
t.Error(err)
236+
return
237+
}
251238

252239
if err := task.Start(ctx); err != nil {
253240
t.Error(err)

‎container_linux_test.go‎

Lines changed: 25 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,11 @@ func TestContainerUpdate(t *testing.T) {
5757
}
5858
defer task.Delete(ctx)
5959

60-
statusC := make(chan uint32, 1)
61-
go func() {
62-
status, err := task.Wait(ctx)
63-
if err != nil {
64-
t.Error(err)
65-
}
66-
statusC <- status
67-
}()
60+
statusC, err := task.Wait(ctx)
61+
if err != nil {
62+
t.Error(err)
63+
return
64+
}
6865

6966
// check that the task has a limit of 32mb
7067
cgroup, err := cgroups.Load(cgroups.V1, cgroups.PidPath(int(task.Pid())))
@@ -157,14 +154,12 @@ func TestShimInCgroup(t *testing.T) {
157154
}
158155
defer task.Delete(ctx)
159156

160-
statusC := make(chan uint32, 1)
161-
go func() {
162-
status, err := task.Wait(ctx)
163-
if err != nil {
164-
t.Error(err)
165-
}
166-
statusC <- status
167-
}()
157+
statusC, err := task.Wait(ctx)
158+
if err != nil {
159+
t.Error(err)
160+
return
161+
}
162+
168163
// check to see if the shim is inside the cgroup
169164
processes, err := cg.Processes(cgroups.Devices, false)
170165
if err != nil {
@@ -221,17 +216,11 @@ func TestDaemonRestart(t *testing.T) {
221216
}
222217
defer task.Delete(ctx)
223218

224-
synC := make(chan struct{})
225-
statusC := make(chan uint32, 1)
226-
go func() {
227-
synC <- struct{}{}
228-
status, err := task.Wait(ctx)
229-
if err == nil {
230-
t.Errorf(`first task.Wait() should have failed with "transport is closing"`)
231-
}
232-
statusC <- status
233-
}()
234-
<-synC
219+
statusC, err := task.Wait(ctx)
220+
if err != nil {
221+
t.Error(err)
222+
return
223+
}
235224

236225
if err := task.Start(ctx); err != nil {
237226
t.Error(err)
@@ -242,7 +231,10 @@ func TestDaemonRestart(t *testing.T) {
242231
t.Fatal(err)
243232
}
244233

245-
<-statusC
234+
status := <-statusC
235+
if status.Err == nil {
236+
t.Errorf(`first task.Wait() should have failed with "transport is closing"`)
237+
}
246238

247239
waitCtx, waitCancel := context.WithTimeout(ctx, 2*time.Second)
248240
serving, err := client.IsServing(waitCtx)
@@ -251,15 +243,11 @@ func TestDaemonRestart(t *testing.T) {
251243
t.Fatalf("containerd did not start within 2s: %v", err)
252244
}
253245

254-
go func() {
255-
synC <- struct{}{}
256-
status, err := task.Wait(ctx)
257-
if err != nil {
258-
t.Error(err)
259-
}
260-
statusC <- status
261-
}()
262-
<-synC
246+
statusC, err = task.Wait(ctx)
247+
if err != nil {
248+
t.Error(err)
249+
return
250+
}
263251

264252
if err := task.Kill(ctx, syscall.SIGKILL); err != nil {
265253
t.Fatal(err)

0 commit comments

Comments
 (0)