cli: set a timeout for the initial connection ping

Note that this does not fully fix the referenced issue, but
at least makes sure that API clients don't hang forever on
the initialization step.

See: #3652
Signed-off-by: Nick Santos <nick@tilt.dev>

Author: nicks

cli/command/cli.go

@@ -35,6 +35,8 @@ import (
 	notaryclient "github.com/theupdateframework/notary/client"
 )
 
+const defaultInitTimeout = 2 * time.Second
+
 // Streams is an interface which exposes the standard input and output streams
 type Streams interface {
 	In() *streams.In
@@ -77,6 +79,7 @@ type DockerCli struct {
 	currentContext     string
 	dockerEndpoint     docker.Endpoint
 	contextStoreConfig store.Config
+	initTimeout        time.Duration
 }
 
 // DefaultVersion returns api.defaultVersion.
@@ -259,6 +262,9 @@ func NewAPIClientFromFlags(opts *cliflags.CommonOptions, configFile *configfile.
 }
 
 func newAPIClientFromEndpoint(ep docker.Endpoint, configFile *configfile.ConfigFile) (client.APIClient, error) {
+	// NOTE(nick): Currently, this doesn't set any reasonable default timeouts, which
+	// means that the CLI can hang forever on any operation. Repro steps:
+	// https://github.com/docker/cli/issues/3652
 	clientOpts, err := ep.ClientOpts()
 	if err != nil {
 		return nil, err
@@ -313,13 +319,21 @@ func resolveDefaultDockerEndpoint(opts *cliflags.CommonOptions) (docker.Endpoint
 	}, nil
 }
 
+func (cli *DockerCli) getInitTimeout() time.Duration {
+	if cli.initTimeout != 0 {
+		return cli.initTimeout
+	}
+	return defaultInitTimeout
+}
+
 func (cli *DockerCli) initializeFromClient() {
 	ctx := context.Background()
-	if strings.HasPrefix(cli.DockerEndpoint().Host, "tcp://") {
+	host := cli.DockerEndpoint().Host
+	if strings.HasPrefix(host, "tcp://") || strings.HasPrefix(host, "unix://") {
 		// @FIXME context.WithTimeout doesn't work with connhelper / ssh connections
 		// time="2020-04-10T10:16:26Z" level=warning msg="commandConn.CloseWrite: commandconn: failed to wait: signal: killed"
 		var cancel func()
-		ctx, cancel = context.WithTimeout(ctx, 2*time.Second)
+		ctx, cancel = context.WithTimeout(ctx, cli.getInitTimeout())
 		defer cancel()
 	}
 

cli/command/cli_test.go

@@ -5,12 +5,14 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"os"
 	"runtime"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/docker/cli/cli/config"
 	"github.com/docker/cli/cli/config/configfile"
@@ -165,6 +167,41 @@ func TestInitializeFromClient(t *testing.T) {
 	}
 }
 
+// Makes sure we don't hang forever on the initial connection.
+// https://github.com/docker/cli/issues/3652
+func TestInitializeFromClientHangs(t *testing.T) {
+	dir := fs.NewDir(t, t.Name())
+	defer dir.Remove()
+
+	socket := dir.Join("my.sock")
+	l, err := net.Listen("unix", socket)
+	assert.NilError(t, err)
+
+	received := false
+	closeConn := make(chan struct{})
+
+	// Simulate a server that hangs on connections.
+	ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		received = true
+		<-closeConn
+		_, _ = w.Write([]byte("OK"))
+	}))
+	ts.Listener = l
+	ts.Start()
+	defer ts.Close()
+
+	opts := &flags.CommonOptions{Hosts: []string{fmt.Sprintf("unix://%s", socket)}}
+	configFile := &configfile.ConfigFile{}
+	apiClient, err := NewAPIClientFromFlags(opts, configFile)
+	assert.NilError(t, err)
+
+	cli := &DockerCli{client: apiClient}
+	cli.initTimeout = time.Millisecond
+	cli.Initialize(flags.NewClientOptions())
+	assert.Assert(t, received)
+	close(closeConn)
+}
+
 // The CLI no longer disables/hides experimental CLI features, however, we need
 // to verify that existing configuration files do not break
 func TestExperimentalCLI(t *testing.T) {