update go-plugin

All our changes have been merged, so this moved the dependency back to
master

go.mod

@@ -63,7 +63,7 @@ require (
 	github.com/hashicorp/go-immutable-radix v0.0.0-20180129170900-7f3cd4390caa // indirect
 	github.com/hashicorp/go-msgpack v0.0.0-20150518234257-fa3f63826f7c // indirect
 	github.com/hashicorp/go-multierror v1.0.0
-	github.com/hashicorp/go-plugin v0.0.0-20181205205220-20341d70f4ff
+	github.com/hashicorp/go-plugin v0.0.0-20181212150838-f444068e8f5a
 	github.com/hashicorp/go-retryablehttp v0.5.0
 	github.com/hashicorp/go-rootcerts v0.0.0-20160503143440-6bb64b370b90
 	github.com/hashicorp/go-safetemp v0.0.0-20180326211150-b1a1dbde6fdc // indirect

go.sum

@@ -139,6 +139,8 @@ github.com/hashicorp/go-multierror v1.0.0 h1:iVjPR7a6H0tWELX5NxNe7bYopibicUzc7uP
 github.com/hashicorp/go-multierror v1.0.0/go.mod h1:dHtQlpGsu+cZNNAkkCN/P3hoUDHhCYQXV3UM06sGGrk=
 github.com/hashicorp/go-plugin v0.0.0-20181205205220-20341d70f4ff h1:z9Nk32P4kDgdYMZU4OGX1Nfpm2q9E++10TSQ6sltD/k=
 github.com/hashicorp/go-plugin v0.0.0-20181205205220-20341d70f4ff/go.mod h1:Ft7ju2vWzhO0ETMKUVo12XmXmII6eSUS4rsPTkY/siA=
+github.com/hashicorp/go-plugin v0.0.0-20181212150838-f444068e8f5a h1:z9eTtDWoxYrJvtAD+xAepmTEfEmYgouWUytJ84UWAr8=
+github.com/hashicorp/go-plugin v0.0.0-20181212150838-f444068e8f5a/go.mod h1:Ft7ju2vWzhO0ETMKUVo12XmXmII6eSUS4rsPTkY/siA=
 github.com/hashicorp/go-retryablehttp v0.5.0 h1:aVN0FYnPwAgZI/hVzqwfMiM86ttcHTlQKbBVeVmXPIs=
 github.com/hashicorp/go-retryablehttp v0.5.0/go.mod h1:9B5zBasrRhHXnJnui7y6sL7es7NDiJgTc6Er0maI1Xs=
 github.com/hashicorp/go-rootcerts v0.0.0-20160503143440-6bb64b370b90 h1:9HVkPxOpo+yO93Ah4yrO67d/qh0fbLLWbKqhYjyHq9A=

vendor/github.com/hashicorp/go-plugin/client.go

@@ -74,7 +74,6 @@ var (
 type Client struct {
 	config            *ClientConfig
 	exited            bool
-	doneLogging       chan struct{}
 	l                 sync.Mutex
 	address           net.Addr
 	process           *os.Process
@@ -82,7 +81,16 @@ type Client struct {
 	protocol          Protocol
 	logger            hclog.Logger
 	doneCtx           context.Context
+	ctxCancel         context.CancelFunc
 	negotiatedVersion int
+
+	// clientWaitGroup is used to manage the lifecycle of the plugin management
+	// goroutines.
+	clientWaitGroup sync.WaitGroup
+
+	// processKilled is used for testing only, to flag when the process was
+	// forcefully killed.
+	processKilled bool
 }
 
 // NegotiatedVersion returns the protocol version negotiated with the server.
@@ -369,6 +377,14 @@ func (c *Client) Exited() bool {
 	return c.exited
 }
 
+// killed is used in tests to check if a process failed to exit gracefully, and
+// needed to be killed.
+func (c *Client) killed() bool {
+	c.l.Lock()
+	defer c.l.Unlock()
+	return c.processKilled
+}
+
 // End the executing subprocess (if it is running) and perform any cleanup
 // tasks necessary such as capturing any remaining logs and so on.
 //
@@ -380,7 +396,6 @@ func (c *Client) Kill() {
 	c.l.Lock()
 	process := c.process
 	addr := c.address
-	doneCh := c.doneLogging
 	c.l.Unlock()
 
 	// If there is no process, there is nothing to kill.
@@ -389,11 +404,14 @@ func (c *Client) Kill() {
 	}
 
 	defer func() {
+		// Wait for the all client goroutines to finish.
+		c.clientWaitGroup.Wait()
+
 		// Make sure there is no reference to the old process after it has been
 		// killed.
 		c.l.Lock()
-		defer c.l.Unlock()
 		c.process = nil
+		c.l.Unlock()
 	}()
 
 	// We need to check for address here. It is possible that the plugin
@@ -416,6 +434,8 @@ func (c *Client) Kill() {
 				// kill in a moment anyways.
 				c.logger.Warn("error closing client during Kill", "err", err)
 			}
+		} else {
+			c.logger.Error("client", "error", err)
 		}
 	}
 
@@ -424,19 +444,20 @@ func (c *Client) Kill() {
 	// doneCh which would be closed if the process exits.
 	if graceful {
 		select {
-		case <-doneCh:
+		case <-c.doneCtx.Done():
 			c.logger.Debug("plugin exited")
 			return
-		case <-time.After(1500 * time.Millisecond):
+		case <-time.After(2 * time.Second):
-			c.logger.Warn("plugin failed to exit gracefully")
 		}
 	}
 
 	// If graceful exiting failed, just kill it
+	c.logger.Warn("plugin failed to exit gracefully")
 	process.Kill()
 
-	// Wait for the client to finish logging so we have a complete log
+	c.l.Lock()
-	<-doneCh
+	c.processKilled = true
+	c.l.Unlock()
 }
 
 // Starts the underlying subprocess, communicating with it to negotiate
@@ -455,7 +476,7 @@ func (c *Client) Start() (addr net.Addr, err error) {
 
 	// If one of cmd or reattach isn't set, then it is an error. We wrap
 	// this in a {} for scoping reasons, and hopeful that the escape
-	// analysis will pop the stock here.
+	// analysis will pop the stack here.
 	{
 		cmdSet := c.config.Cmd != nil
 		attachSet := c.config.Reattach != nil
@@ -469,59 +490,8 @@ func (c *Client) Start() (addr net.Addr, err error) {
 		}
 	}
 
-	// Create the logging channel for when we kill
-	c.doneLogging = make(chan struct{})
-	// Create a context for when we kill
-	var ctxCancel context.CancelFunc
-	c.doneCtx, ctxCancel = context.WithCancel(context.Background())
-
 	if c.config.Reattach != nil {
-		// Verify the process still exists. If not, then it is an error
+		return c.reattach()
-		p, err := os.FindProcess(c.config.Reattach.Pid)
-		if err != nil {
-			return nil, err
-		}
-
-		// Attempt to connect to the addr since on Unix systems FindProcess
-		// doesn't actually return an error if it can't find the process.
-		conn, err := net.Dial(
-			c.config.Reattach.Addr.Network(),
-			c.config.Reattach.Addr.String())
-		if err != nil {
-			p.Kill()
-			return nil, ErrProcessNotFound
-		}
-		conn.Close()
-
-		// Goroutine to mark exit status
-		go func(pid int) {
-			// ensure the context is cancelled when we're done
-			defer ctxCancel()
-			// Wait for the process to die
-			pidWait(pid)
-
-			// Log so we can see it
-			c.logger.Debug("reattached plugin process exited")
-
-			// Mark it
-			c.l.Lock()
-			defer c.l.Unlock()
-			c.exited = true
-
-			// Close the logging channel since that doesn't work on reattach
-			close(c.doneLogging)
-		}(p.Pid)
-
-		// Set the address and process
-		c.address = c.config.Reattach.Addr
-		c.process = p
-		c.protocol = c.config.Reattach.Protocol
-		if c.protocol == "" {
-			// Default the protocol to net/rpc for backwards compatibility
-			c.protocol = ProtocolNetRPC
-		}
-
-		return c.address, nil
 	}
 
 	if c.config.VersionedPlugins == nil {
@@ -618,11 +588,15 @@ func (c *Client) Start() (addr net.Addr, err error) {
 		}
 	}()
 
-	// Start goroutine to wait for process to exit
+	// Create a context for when we kill
-	exitCh := make(chan struct{})
+	c.doneCtx, c.ctxCancel = context.WithCancel(context.Background())
+
+	c.clientWaitGroup.Add(1)
 	go func() {
 		// ensure the context is cancelled when we're done
-		defer ctxCancel()
+		defer c.ctxCancel()
+
+		defer c.clientWaitGroup.Done()
 
 		// get the cmd info early, since the process information will be removed
 		// in Kill.
@@ -645,9 +619,6 @@ func (c *Client) Start() (addr net.Addr, err error) {
 		c.logger.Debug("plugin process exited", debugMsgArgs...)
 		os.Stderr.Sync()
 
-		// Mark that we exited
-		close(exitCh)
-
 		// Set that we exited, which takes a lock
 		c.l.Lock()
 		defer c.l.Unlock()
@@ -655,12 +626,16 @@ func (c *Client) Start() (addr net.Addr, err error) {
 	}()
 
 	// Start goroutine that logs the stderr
+	c.clientWaitGroup.Add(1)
+	// logStderr calls Done()
 	go c.logStderr(cmdStderr)
 
 	// Start a goroutine that is going to be reading the lines
 	// out of stdout
 	linesCh := make(chan string)
+	c.clientWaitGroup.Add(1)
 	go func() {
+		defer c.clientWaitGroup.Done()
 		defer close(linesCh)
 
 		scanner := bufio.NewScanner(cmdStdout)
@@ -671,8 +646,12 @@ func (c *Client) Start() (addr net.Addr, err error) {
 
 	// Make sure after we exit we read the lines from stdout forever
 	// so they don't block since it is a pipe.
+	// The scanner goroutine above will close this, but track it with a wait
+	// group for completeness.
+	c.clientWaitGroup.Add(1)
 	defer func() {
 		go func() {
+			defer c.clientWaitGroup.Done()
 			for range linesCh {
 			}
 		}()
@@ -686,7 +665,7 @@ func (c *Client) Start() (addr net.Addr, err error) {
 	select {
 	case <-timeout:
 		err = errors.New("timeout while waiting for plugin to start")
-	case <-exitCh:
+	case <-c.doneCtx.Done():
 		err = errors.New("plugin exited before we could connect")
 	case line := <-linesCh:
 		// Trim the line and split by "|" in order to get the parts of
@@ -797,6 +776,59 @@ func (c *Client) loadServerCert(cert string) error {
 	return nil
 }
 
+func (c *Client) reattach() (net.Addr, error) {
+	// Verify the process still exists. If not, then it is an error
+	p, err := os.FindProcess(c.config.Reattach.Pid)
+	if err != nil {
+		return nil, err
+	}
+
+	// Attempt to connect to the addr since on Unix systems FindProcess
+	// doesn't actually return an error if it can't find the process.
+	conn, err := net.Dial(
+		c.config.Reattach.Addr.Network(),
+		c.config.Reattach.Addr.String())
+	if err != nil {
+		p.Kill()
+		return nil, ErrProcessNotFound
+	}
+	conn.Close()
+
+	// Create a context for when we kill
+	c.doneCtx, c.ctxCancel = context.WithCancel(context.Background())
+
+	c.clientWaitGroup.Add(1)
+	// Goroutine to mark exit status
+	go func(pid int) {
+		defer c.clientWaitGroup.Done()
+
+		// ensure the context is cancelled when we're done
+		defer c.ctxCancel()
+
+		// Wait for the process to die
+		pidWait(pid)
+
+		// Log so we can see it
+		c.logger.Debug("reattached plugin process exited")
+
+		// Mark it
+		c.l.Lock()
+		defer c.l.Unlock()
+		c.exited = true
+	}(p.Pid)
+
+	// Set the address and process
+	c.address = c.config.Reattach.Addr
+	c.process = p
+	c.protocol = c.config.Reattach.Protocol
+	if c.protocol == "" {
+		// Default the protocol to net/rpc for backwards compatibility
+		c.protocol = ProtocolNetRPC
+	}
+
+	return c.address, nil
+}
+
 // checkProtoVersion returns the negotiated version and PluginSet.
 // This returns an error if the server returned an incompatible protocol
 // version, or an invalid handshake response.
@@ -902,7 +934,7 @@ func (c *Client) dialer(_ string, timeout time.Duration) (net.Conn, error) {
 }
 
 func (c *Client) logStderr(r io.Reader) {
-	defer close(c.doneLogging)
+	defer c.clientWaitGroup.Done()
 
 	scanner := bufio.NewScanner(r)
 	l := c.logger.Named(filepath.Base(c.config.Cmd.Path))

vendor/modules.txt

@@ -316,7 +316,7 @@ github.com/hashicorp/go-getter/helper/url
 github.com/hashicorp/go-hclog
 # github.com/hashicorp/go-multierror v1.0.0
 github.com/hashicorp/go-multierror
-# github.com/hashicorp/go-plugin v0.0.0-20181205205220-20341d70f4ff
+# github.com/hashicorp/go-plugin v0.0.0-20181212150838-f444068e8f5a
 github.com/hashicorp/go-plugin
 github.com/hashicorp/go-plugin/internal/proto
 # github.com/hashicorp/go-retryablehttp v0.5.0