Add StartAndAttach() API endpoint for containers

This solves our prior problems with attach races by ensuring the
order is correct.

Also contains substantial cleanups to the attach code.

Signed-off-by: Matthew Heon <matthew.heon@gmail.com>

Closes: #482
Approved by: baude

3 files changed, 46 insertions, 92 deletions

diff --git a/cmd/podman/attach.go b/cmd/podman/attach.go
index e468964f6..7f7e4d192 100644
--- a/cmd/podman/attach.go
+++ b/cmd/podman/attach.go
@@ -1,11 +1,8 @@
 package main
 
 import (
-	"sync"
-
 	"github.com/pkg/errors"
 	"github.com/projectatomic/libpod/libpod"
-	"github.com/sirupsen/logrus"
 	"github.com/urfave/cli"
 )
 
@@ -65,27 +62,10 @@ func attachCmd(c *cli.Context) error {
 	if conState != libpod.ContainerStateRunning {
 		return errors.Errorf("you can only attach to running containers")
 	}
-	// Create a bool channel to track that the console socket attach
-	// is successful.
-	attached := make(chan bool)
-	// Create a waitgroup so we can sync and wait for all goroutines
-	// to finish before exiting main
-	var wg sync.WaitGroup
 
-	// We increment the wg counter because we need to do the attach
-	wg.Add(1)
-	// Attach to the running container
-	go func() {
-		logrus.Debugf("trying to attach to the container %s", ctr.ID())
-		defer wg.Done()
-		if err := ctr.Attach(c.Bool("no-stdin"), c.String("detach-keys"), attached); err != nil {
-			logrus.Errorf("unable to attach to container %s: %q", ctr.ID(), err)
-		}
-	}()
-	if !<-attached {
-		return errors.Errorf("unable to attach to container %s", ctr.ID())
+	if err := ctr.Attach(c.Bool("no-stdin"), c.String("detach-keys")); err != nil {
+		return errors.Wrapf(err, "error attaching to container %s", ctr.ID())
 	}
-	wg.Wait()
 
 	return nil
 }
diff --git a/cmd/podman/run.go b/cmd/podman/run.go
index 3d6175cef..f68db9036 100644
--- a/cmd/podman/run.go
+++ b/cmd/podman/run.go
@@ -4,7 +4,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"strings"
-	"sync"
 
 	"github.com/pkg/errors"
 	"github.com/projectatomic/libpod/libpod"
@@ -116,38 +115,30 @@ func runCmd(c *cli.Context) error {
 		}
 	}
 
-	// Create a bool channel to track that the console socket attach
-	// is successful.
-	attached := make(chan bool)
-	// Create a waitgroup so we can sync and wait for all goroutines
-	// to finish before exiting main
-	var wg sync.WaitGroup
-
-	if !createConfig.Detach {
-		// We increment the wg counter because we need to do the attach
-		wg.Add(1)
-		// Attach to the running container
-		go func() {
-			logrus.Debugf("trying to attach to the container %s", ctr.ID())
-			defer wg.Done()
-			if err := ctr.Attach(false, c.String("detach-keys"), attached); err != nil {
-				logrus.Errorf("unable to attach to container %s: %q", ctr.ID(), err)
-			}
-		}()
-		if !<-attached {
-			return errors.Errorf("unable to attach to container %s", ctr.ID())
-		}
-	}
-	// Start the container
-	if err := ctr.Start(); err != nil {
-		return errors.Wrapf(err, "unable to start container %q", ctr.ID())
-	}
+	// Handle detached start
 	if createConfig.Detach {
+		if err := ctr.Start(); err != nil {
+			return errors.Wrapf(err, "unable to start container %q", ctr.ID())
+		}
+
 		fmt.Printf("%s\n", ctr.ID())
 		exitCode = 0
 		return nil
 	}
-	wg.Wait()
+
+	// TODO: that "false" should probably be linked to -i
+	// Handle this when we split streams to allow attaching just stdin/out/err
+	attachChan, err := ctr.StartAndAttach(false, c.String("detach-keys"))
+	if err != nil {
+		return errors.Wrapf(err, "unable to start container %q", ctr.ID())
+	}
+
+	// Wait for attach to complete
+	err = <-attachChan
+	if err != nil {
+		return errors.Wrapf(err, "error attaching to container %s", ctr.ID())
+	}
+
 	if ecode, err := ctr.ExitCode(); err != nil {
 		logrus.Errorf("unable to get exit code of container %s: %q", ctr.ID(), err)
 	} else {
diff --git a/cmd/podman/start.go b/cmd/podman/start.go
index 5053abc07..c90fab645 100644
--- a/cmd/podman/start.go
+++ b/cmd/podman/start.go
@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"os"
 	"strconv"
-	"sync"
 
 	"github.com/pkg/errors"
 	"github.com/projectatomic/libpod/libpod"
@@ -75,13 +74,6 @@ func startCmd(c *cli.Context) error {
 	}
 	var lastError error
 	for _, container := range args {
-		// Create a bool channel to track that the console socket attach
-		// is successful.
-		attached := make(chan bool)
-		// Create a waitgroup so we can sync and wait for all goroutines
-		// to finish before exiting main
-		var wg sync.WaitGroup
-
 		ctr, err := runtime.LookupContainer(container)
 		if err != nil {
 			if lastError != nil {
@@ -104,50 +96,41 @@ func startCmd(c *cli.Context) error {
 		if err != nil {
 			return errors.Wrapf(err, "unable to parse annotations in %s", ctr.ID())
 		}
+
+		// Handle start --attach
 		// We only get a terminal session if both a tty was specified in the spec and
 		// -a on the command-line was given.
 		if attach && tty {
-			// We increment the wg counter because we need to do the attach
-			wg.Add(1)
-			// Attach to the running container
-			go func() {
-				logrus.Debugf("trying to attach to the container %s", ctr.ID())
-				defer wg.Done()
-				if err := ctr.Attach(noStdIn, c.String("detach-keys"), attached); err != nil {
-					logrus.Errorf("unable to attach to container %s: %q", ctr.ID(), err)
-				}
-			}()
-			if !<-attached {
-				return errors.Errorf("unable to attach to container %s", ctr.ID())
+			attachChan, err := ctr.StartAndAttach(noStdIn, c.String("detach-keys"))
+			if err != nil {
+				return errors.Wrapf(err, "unable to start container %s", ctr.ID())
+			}
+
+			// Wait for attach to complete
+			err = <-attachChan
+			if err != nil {
+				return errors.Wrapf(err, "error attaching to container %s", ctr.ID())
 			}
+
+			if ecode, err := ctr.ExitCode(); err != nil {
+				logrus.Errorf("unable to get exit code of container %s: %q", ctr.ID(), err)
+			} else {
+				exitCode = int(ecode)
+			}
+
+			return ctr.Cleanup()
 		}
-		err = ctr.Start()
-		if err != nil {
+
+		// Handle non-attach start
+		if err := ctr.Start(); err != nil {
 			if lastError != nil {
 				fmt.Fprintln(os.Stderr, lastError)
 			}
-			lastError = errors.Wrapf(err, "unable to start %s", container)
+			lastError = errors.Wrapf(err, "unable to start container %q", container)
 			continue
 		}
-		if !attach {
-			fmt.Println(ctr.ID())
-		}
-		wg.Wait()
-		if ecode, err := ctr.ExitCode(); err != nil {
-			logrus.Errorf("unable to get exit code of container %s: %q", ctr.ID(), err)
-		} else {
-			exitCode = int(ecode)
-		}
-		if lastError != nil {
-			fmt.Fprintln(os.Stderr, lastError)
-		}
-		// We can only do this if we attached
-		// Otherwise the container is probably still running
-		if attach && tty {
-			lastError = ctr.Cleanup()
-			// No need for LastError as we can only have one ctr
-			// with attach
-		}
+		fmt.Println(ctr.ID())
 	}
+
 	return lastError
 }