response to comments

nicks · nicks · commit 26a184192c89 · 2022-07-08T18:50:09.000-04:00
Signed-off-by: Nick Santos &lt;nick.santos@docker.com&gt;
diff --git a/cli/command/cli.go b/cli/command/cli.go
@@ -193,6 +193,15 @@ func WithInitializeClient(makeClient func(dockerCli *DockerCli) (client.APIClien
 	}
 }
 
+// WithInitializeTimeout is passed to DockerCli.Initialize by callers who wish to set a
+// timeout on the initial ping duration. The default is 2s.
+func WithInitializeTimeout(timeout time.Duration) InitializeOpt {
+	return func(dockerCli *DockerCli) error {
+		dockerCli.initTimeout = timeout
+		return nil
+	}
+}
+
 // Initialize the dockerCli runs initialization that must happen after command
 // line flags are parsed.
 func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...InitializeOpt) error {
@@ -262,9 +271,6 @@ 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
diff --git a/cli/command/cli_test.go b/cli/command/cli_test.go
@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"path/filepath"
 	"runtime"
 	"strings"
 	"testing"
@@ -170,20 +171,21 @@ 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")
+	dir := t.TempDir()
+	socket := filepath.Join(dir, "my.sock")
 	l, err := net.Listen("unix", socket)
 	assert.NilError(t, err)
 
-	received := false
-	closeConn := make(chan struct{})
+	receiveReqCh := make(chan bool)
+	timeoutCtx, cancel := context.WithTimeout(context.Background(), time.Second)
+	defer cancel()
 
 	// Simulate a server that hangs on connections.
 	ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		received = true
-		<-closeConn
+		select {
+		case <-timeoutCtx.Done():
+		case receiveReqCh <- true: // Blocks until someone receives on the channel.
+		}
 		_, _ = w.Write([]byte("OK"))
 	}))
 	ts.Listener = l
@@ -195,11 +197,25 @@ func TestInitializeFromClientHangs(t *testing.T) {
 	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)
+	initializedCh := make(chan bool)
+
+	go func() {
+		cli := &DockerCli{client: apiClient}
+		cli.Initialize(flags.NewClientOptions(), WithInitializeTimeout(time.Millisecond))
+		close(initializedCh)
+	}()
+
+	select {
+	case <-timeoutCtx.Done():
+		t.Fatal("timeout waiting for initialization to complete")
+	case <-initializedCh:
+	}
+
+	select {
+	case <-timeoutCtx.Done():
+		t.Fatal("server never received an init request")
+	case <-receiveReqCh:
+	}
 }
 
 // The CLI no longer disables/hides experimental CLI features, however, we need
