Merge pull request #4960 from neersighted/plugin_comments

plugin: update/improve process lifecycle documentation

cli-plugins/socket/socket.go

@@ -11,12 +11,13 @@ import (
 	"sync"
 )
 
-// EnvKey represents the well-known environment variable used to pass the plugin being
+// EnvKey represents the well-known environment variable used to pass the
-// executed the socket name it should listen on to coordinate with the host CLI.
+// plugin being executed the socket name it should listen on to coordinate with
+// the host CLI.
 const EnvKey = "DOCKER_CLI_PLUGIN_SOCKET"
 
-// NewPluginServer creates a plugin server that listens on a new Unix domain socket.
+// NewPluginServer creates a plugin server that listens on a new Unix domain
-// `h` is called for each new connection to the socket in a goroutine.
+// socket. h is called for each new connection to the socket in a goroutine.
 func NewPluginServer(h func(net.Conn)) (*PluginServer, error) {
 	l, err := listen("docker_cli_" + randomID())
 	if err != nil {
@@ -63,7 +64,7 @@ func (pl *PluginServer) accept() error {
 	defer pl.mu.Unlock()
 
 	if pl.closed {
-		// handle potential race condition between Close and Accept
+		// Handle potential race between Close and accept.
 		conn.Close()
 		return errors.New("plugin server is closed")
 	}
@@ -74,20 +75,25 @@ func (pl *PluginServer) accept() error {
 	return nil
 }
 
+// Addr returns the [net.Addr] of the underlying [net.Listener].
 func (pl *PluginServer) Addr() net.Addr {
 	return pl.l.Addr()
 }
 
-// Close ensures that the server is no longer accepting new connections and closes all existing connections.
+// Close ensures that the server is no longer accepting new connections and
-// Existing connections will receive [io.EOF].
+// closes all existing connections. Existing connections will receive [io.EOF].
+//
+// The error value is that of the underlying [net.Listner.Close] call.
 func (pl *PluginServer) Close() error {
 	// Remove the listener socket, if it exists on the filesystem.
 	unlink(pl.l)
 
-	// Close connections first to ensure the connections get io.EOF instead of a connection reset.
+	// Close connections first to ensure the connections get io.EOF instead
+	// of a connection reset.
 	pl.closeAllConns()
 
-	// Try to ensure that any active connections have a chance to receive io.EOF
+	// Try to ensure that any active connections have a chance to receive
+	// io.EOF.
 	runtime.Gosched()
 
 	return pl.l.Close()
@@ -97,7 +103,7 @@ func (pl *PluginServer) closeAllConns() {
 	pl.mu.Lock()
 	defer pl.mu.Unlock()
 
-	// Prevent new connections from being accepted
+	// Prevent new connections from being accepted.
 	pl.closed = true
 
 	for _, conn := range pl.conns {

cmd/docker/docker.go

@@ -220,33 +220,46 @@ func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string,
 		return err
 	}
 
-	// Establish the plugin socket, adding it to the environment under a well-known key if successful.
+	// Establish the plugin socket, adding it to the environment under a
+	// well-known key if successful.
 	srv, err := socket.NewPluginServer(nil)
 	if err == nil {
-		envs = append(envs, socket.EnvKey+"="+srv.Addr().String())
+		plugincmd.Env = append(plugincmd.Env, socket.EnvKey+"="+srv.Addr().String())
 	}
 
-	plugincmd.Env = append(envs, plugincmd.Env...)
+	// Set additional environment variables specified by the caller.
+	plugincmd.Env = append(plugincmd.Env, envs...)
 
+	// Background signal handling logic: block on the signals channel, and
+	// notify the plugin via the PluginServer (or signal) as appropriate.
 	const exitLimit = 3
-
 	signals := make(chan os.Signal, exitLimit)
 	signal.Notify(signals, platformsignals.TerminationSignals...)
-	// signal handling goroutine: listen on signals channel, and if conn is
-	// non-nil, attempt to close it to let the plugin know to exit. Regardless
-	// of whether we successfully signal the plugin or not, after 3 SIGINTs,
-	// we send a SIGKILL to the plugin process and exit
 	go func() {
 		retries := 0
 		for range signals {
+			// If stdin is a TTY, the kernel will forward
+			// signals to the subprocess because the shared
+			// pgid makes the TTY a controlling terminal.
+			//
+			// The plugin should have it's own copy of this
+			// termination logic, and exit after 3 retries
+			// on it's own.
 			if dockerCli.Out().IsTerminal() {
-				// running attached to a terminal, so the plugin will already
-				// receive signals due to sharing a pgid with the parent CLI
 				continue
 			}
 
-			srv.Close()
+			// Terminate the plugin server, which will
+			// close all connections with plugin
+			// subprocesses, and signal them to exit.
+			//
+			// Repeated invocations will result in EINVAL,
+			// or EBADF; but that is fine for our purposes.
+			_ = srv.Close()
 
+			// If we're still running after 3 interruptions
+			// (SIGINT/SIGTERM), send a SIGKILL to the plugin as a
+			// final attempt to terminate, and exit.
 			retries++
 			if retries >= exitLimit {
 				_, _ = fmt.Fprintf(dockerCli.Err(), "got %d SIGTERM/SIGINTs, forcefully exiting\n", retries)