diff options
author | Fabricio Voznika <fvoznika@google.com> | 2020-11-18 16:04:26 -0800 |
---|---|---|
committer | gVisor bot <gvisor-bot@google.com> | 2020-11-18 16:06:31 -0800 |
commit | 7158095d687dd9ede3a3fc8da1f4dfbb2ebc176e (patch) | |
tree | f424dfb30dd5950b8b24d61773534583a43ed6d6 /runsc | |
parent | df37babd576ba4607e2fe69eb2c669aa2954b9cb (diff) |
Fix race condition in multi-container wait test
Container is not thread-safe, locking must be done in the caller.
The test was calling Container.Wait() from multiple threads with
no synchronization.
Also removed Container.WaitPID from test because the process might
have already existed when wait is called.
PiperOrigin-RevId: 343176280
Diffstat (limited to 'runsc')
-rw-r--r-- | runsc/container/container.go | 1 | ||||
-rw-r--r-- | runsc/container/multi_container_test.go | 53 |
2 files changed, 11 insertions, 43 deletions
diff --git a/runsc/container/container.go b/runsc/container/container.go index f3d990cfc..418a27beb 100644 --- a/runsc/container/container.go +++ b/runsc/container/container.go @@ -80,6 +80,7 @@ func validateID(id string) error { // - It calls 'runsc delete'. runc implementation kills --all SIGKILL once // again just to be sure, waits, and then proceeds with remaining teardown. // +// Container is thread-unsafe. type Container struct { // ID is the container ID. ID string `json:"id"` diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go index cadc63bf3..45d4e6e6e 100644 --- a/runsc/container/multi_container_test.go +++ b/runsc/container/multi_container_test.go @@ -301,54 +301,21 @@ func TestMultiContainerWait(t *testing.T) { } defer cleanup() - // Check via ps that multiple processes are running. - expectedPL := []*control.Process{ - newProcessBuilder().PID(2).PPID(0).Cmd("sleep").Process(), - } - if err := waitForProcessList(containers[1], expectedPL); err != nil { - t.Errorf("failed to wait for sleep to start: %v", err) - } - - // Wait on the short lived container from multiple goroutines. - wg := sync.WaitGroup{} - for i := 0; i < 3; i++ { - wg.Add(1) - go func(c *Container) { - defer wg.Done() - if ws, err := c.Wait(); err != nil { - t.Errorf("failed to wait for process %s: %v", c.Spec.Process.Args, err) - } else if es := ws.ExitStatus(); es != 0 { - t.Errorf("process %s exited with non-zero status %d", c.Spec.Process.Args, es) - } - if _, err := c.Wait(); err != nil { - t.Errorf("wait for stopped container %s shouldn't fail: %v", c.Spec.Process.Args, err) - } - }(containers[1]) + // Check that we can wait for the sub-container. + c := containers[1] + if ws, err := c.Wait(); err != nil { + t.Errorf("failed to wait for process %s: %v", c.Spec.Process.Args, err) + } else if es := ws.ExitStatus(); es != 0 { + t.Errorf("process %s exited with non-zero status %d", c.Spec.Process.Args, es) } - - // Also wait via PID. - for i := 0; i < 3; i++ { - wg.Add(1) - go func(c *Container) { - defer wg.Done() - const pid = 2 - if ws, err := c.WaitPID(pid); err != nil { - t.Errorf("failed to wait for PID %d: %v", pid, err) - } else if es := ws.ExitStatus(); es != 0 { - t.Errorf("PID %d exited with non-zero status %d", pid, es) - } - if _, err := c.WaitPID(pid); err == nil { - t.Errorf("wait for stopped PID %d should fail", pid) - } - }(containers[1]) + if _, err := c.Wait(); err != nil { + t.Errorf("wait for stopped container %s shouldn't fail: %v", c.Spec.Process.Args, err) } - wg.Wait() - // After Wait returns, ensure that the root container is running and // the child has finished. - expectedPL = []*control.Process{ - newProcessBuilder().Cmd("sleep").Process(), + expectedPL := []*control.Process{ + newProcessBuilder().Cmd("sleep").PID(1).Process(), } if err := waitForProcessList(containers[0], expectedPL); err != nil { t.Errorf("failed to wait for %q to start: %v", strings.Join(containers[0].Spec.Process.Args, " "), err) |