Remove launchDir from session and cleanup test

The implementation of the e2e test wasn't properly verifying that run
directories were getting cleaned up. These changes address that.

Also, instead of delegating cleanup of launchDir to the session as a
special case, the session was modified to allow arbitrary callbacks to
be called at exit prior to returning from wait. This is more generic
than wiring through the launch directory and is simple enough.

Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>

core/container/container.go

@@ -155,18 +155,19 @@ func (r *Router) Wait(ccid string) (int, error) {
 }
 
 func (r *Router) Shutdown(timeout time.Duration) {
+	var wg sync.WaitGroup
+	for ccid := range r.containers {
+		wg.Add(1)
+		go func(ccid string) {
+			defer wg.Done()
+			if err := r.Stop(ccid); err != nil {
+				vmLogger.Warnw("failed to stop chaincode", "ccid", ccid, "error", err)
+			}
+		}(ccid)
+	}
+
 	done := make(chan struct{})
 	go func() {
-		var wg sync.WaitGroup
-		wg.Add(len(r.containers))
-		for ccid := range r.containers {
-			go func(ccid string) {
-				if err := r.Stop(ccid); err != nil {
-					vmLogger.Warningf("failed to stop ccid: %s err: %s", ccid, err)
-				}
-				wg.Done()
-			}(ccid)
-		}
 		wg.Wait()
 		close(done)
 	}()

core/container/externalbuilder/externalbuilder.go

@@ -377,7 +377,7 @@ func (b *Builder) Run(ccid, bldDir string, peerConnection *ccintf.PeerConnection
 
 	run := filepath.Join(b.Location, "bin", "run")
 	cmd := b.NewCommand(run, bldDir, launchDir)
-	sess, err := Start(b.Logger, cmd, launchDir)
+	sess, err := Start(b.Logger, cmd, func(error) { os.RemoveAll(launchDir) })
 	if err != nil {
 		os.RemoveAll(launchDir)
 		return nil, errors.Wrapf(err, "builder '%s' run failed to start", b.Name)
@@ -388,7 +388,7 @@ func (b *Builder) Run(ccid, bldDir string, peerConnection *ccintf.PeerConnection
 
 // runCommand runs a command and waits for it to complete.
 func (b *Builder) runCommand(cmd *exec.Cmd) error {
-	sess, err := Start(b.Logger, cmd, "")
+	sess, err := Start(b.Logger, cmd)
 	if err != nil {
 		return err
 	}

core/container/externalbuilder/instance_test.go

@@ -258,7 +258,7 @@ var _ = Describe("Instance", func() {
 	Describe("Stop", func() {
 		It("terminates the process", func() {
 			cmd := exec.Command("sleep", "90")
-			sess, err := externalbuilder.Start(logger, cmd, "")
+			sess, err := externalbuilder.Start(logger, cmd)
 			Expect(err).NotTo(HaveOccurred())
 			instance.Session = sess
 			instance.TermTimeout = time.Minute
@@ -268,14 +268,14 @@ var _ = Describe("Instance", func() {
 			Consistently(errCh).ShouldNot(Receive())
 
 			err = instance.Stop()
-			Expect(err).ToNot(HaveOccurred())
+			Expect(err).NotTo(HaveOccurred())
 			Eventually(errCh).Should(Receive(MatchError("signal: terminated")))
 		})
 
 		Context("when the process doesn't respond to SIGTERM within TermTimeout", func() {
 			It("kills the process with malice", func() {
 				cmd := exec.Command("testdata/ignoreterm.sh")
-				sess, err := externalbuilder.Start(logger, cmd, "")
+				sess, err := externalbuilder.Start(logger, cmd)
 				Expect(err).NotTo(HaveOccurred())
 
 				instance.Session = sess
@@ -286,7 +286,7 @@ var _ = Describe("Instance", func() {
 				Consistently(errCh).ShouldNot(Receive())
 
 				err = instance.Stop()
-				Expect(err).ToNot(HaveOccurred())
+				Expect(err).NotTo(HaveOccurred())
 				Eventually(errCh).Should(Receive(MatchError("signal: killed")))
 			})
 		})

core/container/externalbuilder/session.go

@@ -18,20 +18,22 @@ import (
 	"github.com/hyperledger/fabric/common/flogging"
 )
 
+type ExitFunc func(error)
+
 type Session struct {
 	mutex      sync.Mutex
 	command    *exec.Cmd
 	exited     chan struct{}
 	exitErr    error
 	waitStatus syscall.WaitStatus
-	launchDir  string
+	exitFuncs  []ExitFunc
 }
 
 // Start will start the provided command and return a Session that can be used
 // to await completion or signal the process.
 //
 // The provided logger is used log stderr from the running process.
-func Start(logger *flogging.FabricLogger, cmd *exec.Cmd, launchDir string) (*Session, error) {
+func Start(logger *flogging.FabricLogger, cmd *exec.Cmd, exitFuncs ...ExitFunc) (*Session, error) {
 	logger = logger.With("command", filepath.Base(cmd.Path))
 
 	stderr, err := cmd.StderrPipe()
@@ -46,8 +48,8 @@ func Start(logger *flogging.FabricLogger, cmd *exec.Cmd, launchDir string) (*Ses
 
 	sess := &Session{
 		command:   cmd,
+		exitFuncs: exitFuncs,
 		exited:    make(chan struct{}),
-		launchDir: launchDir,
 	}
 	go sess.waitForExit(logger, stderr)
 
@@ -72,7 +74,9 @@ func (s *Session) waitForExit(logger *flogging.FabricLogger, stderr io.Reader) {
 	defer s.mutex.Unlock()
 	s.exitErr = err
 	s.waitStatus = s.command.ProcessState.Sys().(syscall.WaitStatus)
-	os.RemoveAll(s.launchDir)
+	for _, exit := range s.exitFuncs {
+		exit(s.exitErr)
+	}
 	close(s.exited)
 }
 

core/container/externalbuilder/session_test.go

@@ -36,7 +36,7 @@ var _ = Describe("Session", func() {
 
 	It("starts commands and returns a session handle to wait on", func() {
 		cmd := exec.Command("true")
-		sess, err := externalbuilder.Start(logger, cmd, "")
+		sess, err := externalbuilder.Start(logger, cmd)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(sess).NotTo(BeNil())
 
@@ -46,7 +46,7 @@ var _ = Describe("Session", func() {
 
 	It("captures stderr to the provided logger", func() {
 		cmd := exec.Command("sh", "-c", "echo 'this is a message to stderr' >&2")
-		sess, err := externalbuilder.Start(logger, cmd, "")
+		sess, err := externalbuilder.Start(logger, cmd)
 		Expect(err).NotTo(HaveOccurred())
 		err = sess.Wait()
 		Expect(err).NotTo(HaveOccurred())
@@ -60,7 +60,7 @@ var _ = Describe("Session", func() {
 		Expect(err).NotTo(HaveOccurred())
 		defer stdin.Close()
 
-		sess, err := externalbuilder.Start(logger, cmd, "")
+		sess, err := externalbuilder.Start(logger, cmd)
 		Expect(err).NotTo(HaveOccurred())
 
 		exitCh := make(chan error)
@@ -71,23 +71,60 @@ var _ = Describe("Session", func() {
 		Eventually(exitCh).Should(Receive(MatchError("signal: terminated")))
 	})
 
+	It("calls exit functions", func() {
+		var called1, called2 bool
+		var exitErr1, exitErr2 error
+
+		cmd := exec.Command("true")
+		sess, err := externalbuilder.Start(logger, cmd, func(err error) {
+			called1 = true
+			exitErr1 = err
+		}, func(err error) {
+			called2 = true
+			exitErr2 = err
+		})
+		Expect(err).NotTo(HaveOccurred())
+		Expect(sess).NotTo(BeNil())
+
+		err = sess.Wait()
+		Expect(err).NotTo(HaveOccurred())
+
+		Expect(called1).To(BeTrue())
+		Expect(exitErr1).NotTo(HaveOccurred())
+		Expect(called2).To(BeTrue())
+		Expect(exitErr2).NotTo(HaveOccurred())
+	})
+
 	When("start fails", func() {
 		It("returns an error", func() {
 			cmd := exec.Command("./this-is-not-a-command")
-			_, err := externalbuilder.Start(logger, cmd, "")
+			_, err := externalbuilder.Start(logger, cmd)
 			Expect(err).To(MatchError("fork/exec ./this-is-not-a-command: no such file or directory"))
 		})
 	})
 
 	When("the command fails", func() {
 		It("returns the exit error from the command", func() {
 			cmd := exec.Command("false")
-			sess, err := externalbuilder.Start(logger, cmd, "")
+			sess, err := externalbuilder.Start(logger, cmd)
 			Expect(err).NotTo(HaveOccurred())
 
 			err = sess.Wait()
 			Expect(err).To(MatchError("exit status 1"))
 			Expect(err).To(BeAssignableToTypeOf(&exec.ExitError{}))
 		})
+
+		It("passes the error to the exit function", func() {
+			var exitErr error
+			cmd := exec.Command("false")
+			sess, err := externalbuilder.Start(logger, cmd, func(err error) {
+				exitErr = err
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			err = sess.Wait()
+			Expect(err).To(MatchError("exit status 1"))
+			Expect(exitErr).To(Equal(err))
+		})
 	})
 })

integration/e2e/e2e_test.go

@@ -8,6 +8,7 @@ package e2e
 
 import (
 	"bufio"
+	"bytes"
 	"context"
 	"crypto/sha256"
 	"crypto/tls"
@@ -130,22 +131,23 @@ var _ = Describe("EndToEnd", func() {
 			if datagramReader != nil {
 				datagramReader.Close()
 			}
+
+			// Terminate the processes but defer the network cleanup to the outer
+			// AfterEach.
 			if process != nil {
 				process.Signal(syscall.SIGTERM)
 				Eventually(process.Wait(), network.EventuallyTimeout).Should(Receive())
+				process = nil
 			}
-			if runArtifactsFilePath != "" {
-				// ensure that the temporary directories generated by launched
-				// external chaincodes have been cleaned up
-				runArtifactsFile, err := os.Open(runArtifactsFilePath)
-				Expect(err).NotTo(HaveOccurred())
-				scanner := bufio.NewScanner(runArtifactsFile)
-				for scanner.Scan() {
-					launchFiles, err := ioutil.ReadDir(scanner.Text())
-					Expect(err).To(MatchError(fmt.Sprintf("open %s: no such file or directory", scanner.Text())))
-					Expect(launchFiles).To(BeEmpty())
-				}
-				runArtifactsFile.Close()
+
+			// Ensure that the temporary directories generated by launched external
+			// chaincodes have been cleaned up. This must be done after the peers
+			// have been terminated.
+			contents, err := ioutil.ReadFile(runArtifactsFilePath)
+			Expect(err).NotTo(HaveOccurred())
+			scanner := bufio.NewScanner(bytes.NewBuffer(contents))
+			for scanner.Scan() {
+				Expect(scanner.Text()).NotTo(BeAnExistingFile())
 			}
 		})
 
@@ -179,15 +181,12 @@ var _ = Describe("EndToEnd", func() {
 			nwo.DeployChaincode(network, "testchannel", orderer, chaincode)
 
 			By("ensuring external cc run artifacts exist after deploying")
-			runArtifactsFile, err := os.Open(runArtifactsFilePath)
+			contents, err := ioutil.ReadFile(runArtifactsFilePath)
 			Expect(err).NotTo(HaveOccurred())
-			scanner := bufio.NewScanner(runArtifactsFile)
+			scanner := bufio.NewScanner(bytes.NewBuffer(contents))
 			for scanner.Scan() {
-				runArtifacts, err := ioutil.ReadDir(scanner.Text())
-				Expect(err).NotTo(HaveOccurred())
-				Expect(runArtifacts).NotTo(BeEmpty())
+				Expect(scanner.Text()).To(BeADirectory())
 			}
-			runArtifactsFile.Close()
 
 			By("getting the client peer by name")
 			peer := network.Peer("Org1", "peer0")