Merge pull request #282 from mlaventure/fix-race-in-process-start

Handle case where shim is reaped before the call to the runtime start

integration-test/check_test.go

@@ -258,13 +258,13 @@ func (cs *ContainerdSuite) TearDownTest(c *check.C) {
 		})
 
 		if err := cs.KillContainer(ctr.Id); err != nil {
-			fmt.Fprintf(os.Stderr, "Failed to cleanup leftover test containers: %v", err)
+			fmt.Fprintf(os.Stderr, "Failed to cleanup leftover test containers: %v\n", err)
 		}
 
 		select {
 		case <-ch:
 		case <-time.After(3 * time.Second):
-			fmt.Fprintf(os.Stderr, "TearDownTest: Containerd %v didn't die after 3 seconds", ctr.Id)
+			fmt.Fprintf(os.Stderr, "TearDownTest: Containerd %v didn't die after 3 seconds\n", ctr.Id)
 		}
 	}
 }

runtime/container.go

@@ -465,6 +465,7 @@ func (c *container) Exec(pid string, pspec specs.ProcessSpec, s Stdio) (pp Proce
 func (c *container) createCmd(pid string, cmd *exec.Cmd, p *process) error {
 	p.cmd = cmd
 	if err := cmd.Start(); err != nil {
+		close(p.cmdDoneCh)
 		if exErr, ok := err.(*exec.Error); ok {
 			if exErr.Err == exec.ErrNotFound || exErr.Err == os.ErrNotExist {
 				return fmt.Errorf("%s not installed on system", c.shim)
@@ -472,6 +473,13 @@ func (c *container) createCmd(pid string, cmd *exec.Cmd, p *process) error {
 		}
 		return err
 	}
+	go func() {
+		err := p.cmd.Wait()
+		if err == nil {
+			p.cmdSuccess = true
+		}
+		close(p.cmdDoneCh)
+	}()
 	if err := c.waitForCreate(p, cmd); err != nil {
 		return err
 	}

runtime/process.go

@@ -67,6 +67,7 @@ func newProcess(config *processConfig) (*process, error) {
 		container: config.c,
 		spec:      config.processSpec,
 		stdio:     config.stdio,
+		cmdDoneCh: make(chan struct{}),
 	}
 	uid, gid, err := getRootIDs(config.spec)
 	if err != nil {
@@ -148,6 +149,8 @@ type process struct {
 	spec        specs.ProcessSpec
 	stdio       Stdio
 	cmd         *exec.Cmd
+	cmdSuccess  bool
+	cmdDoneCh   chan struct{}
 }
 
 func (p *process) ID() string {
@@ -230,8 +233,8 @@ func (p *process) getPidFromFile() (int, error) {
 
 // Wait will reap the shim process
 func (p *process) Wait() {
-	if p.cmd != nil {
-		p.cmd.Wait()
+	if p.cmdDoneCh != nil {
+		<-p.cmdDoneCh
 	}
 }
 
@@ -261,10 +264,9 @@ func (p *process) Signal(s os.Signal) error {
 func (p *process) Start() error {
 	if p.ID() == InitProcessID {
 		var (
-			errC     = make(chan error, 1)
-			shimExit = make(chan struct{}, 1)
-			args     = append(p.container.runtimeArgs, "start", p.container.id)
-			cmd      = exec.Command(p.container.runtime, args...)
+			errC = make(chan error, 1)
+			args = append(p.container.runtimeArgs, "start", p.container.id)
+			cmd  = exec.Command(p.container.runtime, args...)
 		)
 		go func() {
 			out, err := cmd.CombinedOutput()
@@ -273,19 +275,21 @@ func (p *process) Start() error {
 			}
 			errC <- nil
 		}()
-		go func() {
-			p.Wait()
-			close(shimExit)
-		}()
 		select {
 		case err := <-errC:
 			if err != nil {
 				return err
 			}
-		case <-shimExit:
-			cmd.Process.Kill()
-			cmd.Wait()
-			return ErrShimExited
+		case <-p.cmdDoneCh:
+			if !p.cmdSuccess {
+				cmd.Process.Kill()
+				cmd.Wait()
+				return ErrShimExited
+			}
+			err := <-errC
+			if err != nil {
+				return err
+			}
 		}
 	}
 	return nil

supervisor/sort_test.go

@@ -1,10 +1,10 @@
 package supervisor
 
 import (
+	"flag"
 	"os"
 	"sort"
 	"testing"
-	"flag"
 
 	"github.com/docker/containerd/runtime"
 	"github.com/docker/containerd/specs"
@@ -71,7 +71,6 @@ func (p *testProcess) State() runtime.State {
 }
 
 func (p *testProcess) Wait() {
-
 }
 
 func TestSortProcesses(t *testing.T) {