From 9900c7a348ad170548975c957916241e7a79e0c4 Mon Sep 17 00:00:00 2001
From: Nick Santos <nick.santos@docker.com>
Date: Thu, 22 Dec 2022 18:32:19 -0500
Subject: [PATCH] client: improve error messaging on crash

Repro steps:
- Run Docker Desktop
- Run `docker run busybox tail -f /dev/null`
- Run `pkill "Docker Desktop"

Expected:
An error message that indicates that Docker Desktop is shutting down.

Actual:
An error message that looks like this:

```
error waiting for container: invalid character 's' looking for beginning of value
```

here's an example:

https://github.com/docker/for-mac/issues/6575#issuecomment-1324879001

After this change, you get an error message like:

```
error waiting for container: copying response body from Docker: unexpected EOF
```

which is a bit more explicit.

Signed-off-by: Nick Santos <nick.santos@docker.com>
---
 client/container_wait.go      | 23 +++++++++++++--
 client/container_wait_test.go | 55 +++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/client/container_wait.go b/client/container_wait.go
index 9aff7161325ce..2375eb1e80f87 100644
--- a/client/container_wait.go
+++ b/client/container_wait.go
@@ -1,14 +1,19 @@
 package client // import "github.com/docker/docker/client"
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
+	"io"
 	"net/url"
 
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/versions"
 )
 
+const containerWaitErrorMsgLimit = 2 * 1024 /* Max: 2KiB */
+
 // ContainerWait waits until the specified container is in a certain state
 // indicated by the given condition, either "not-running" (default),
 // "next-exit", or "removed".
@@ -46,9 +51,23 @@ func (cli *Client) ContainerWait(ctx context.Context, containerID string, condit
 
 	go func() {
 		defer ensureReaderClosed(resp)
+
+		body := resp.body
+		responseText := bytes.NewBuffer(nil)
+		stream := io.TeeReader(body, responseText)
+
 		var res container.WaitResponse
-		if err := json.NewDecoder(resp.body).Decode(&res); err != nil {
-			errC <- err
+		if err := json.NewDecoder(stream).Decode(&res); err != nil {
+			// NOTE(nicks): The /wait API does not work well with HTTP proxies.
+			// At any time, the proxy could cut off the response stream.
+			//
+			// But because the HTTP status has already been written, the proxy's
+			// only option is to write a plaintext error message.
+			//
+			// If there's a JSON parsing error, read the real error message
+			// off the body and send it to the client.
+			_, _ = io.ReadAll(io.LimitReader(stream, containerWaitErrorMsgLimit))
+			errC <- errors.New(responseText.String())
 			return
 		}
 
diff --git a/client/container_wait_test.go b/client/container_wait_test.go
index 63d08ded0d4d4..0c6e974773fd1 100644
--- a/client/container_wait_test.go
+++ b/client/container_wait_test.go
@@ -62,6 +62,61 @@ func TestContainerWait(t *testing.T) {
 	}
 }
 
+func TestContainerWaitProxyInterrupt(t *testing.T) {
+	expectedURL := "/v1.30/containers/container_id/wait"
+	msg := "copying response body from Docker: unexpected EOF"
+	client := &Client{
+		version: "1.30",
+		client: newMockClient(func(req *http.Request) (*http.Response, error) {
+			if !strings.HasPrefix(req.URL.Path, expectedURL) {
+				return nil, fmt.Errorf("Expected URL '%s', got '%s'", expectedURL, req.URL)
+			}
+			return &http.Response{
+				StatusCode: http.StatusOK,
+				Body:       io.NopCloser(strings.NewReader(msg)),
+			}, nil
+		}),
+	}
+
+	resultC, errC := client.ContainerWait(context.Background(), "container_id", "")
+	select {
+	case err := <-errC:
+		if !strings.Contains(err.Error(), msg) {
+			t.Fatalf("Expected: %s, Actual: %s", msg, err.Error())
+		}
+	case result := <-resultC:
+		t.Fatalf("Unexpected result: %v", result)
+	}
+}
+
+func TestContainerWaitProxyInterruptLong(t *testing.T) {
+	expectedURL := "/v1.30/containers/container_id/wait"
+	msg := strings.Repeat("x", containerWaitErrorMsgLimit*5)
+	client := &Client{
+		version: "1.30",
+		client: newMockClient(func(req *http.Request) (*http.Response, error) {
+			if !strings.HasPrefix(req.URL.Path, expectedURL) {
+				return nil, fmt.Errorf("Expected URL '%s', got '%s'", expectedURL, req.URL)
+			}
+			return &http.Response{
+				StatusCode: http.StatusOK,
+				Body:       io.NopCloser(strings.NewReader(msg)),
+			}, nil
+		}),
+	}
+
+	resultC, errC := client.ContainerWait(context.Background(), "container_id", "")
+	select {
+	case err := <-errC:
+		// LimitReader limiting isn't exact, because of how the Readers do chunking.
+		if len(err.Error()) > containerWaitErrorMsgLimit*2 {
+			t.Fatalf("Expected error to be limited around %d, actual length: %d", containerWaitErrorMsgLimit, len(err.Error()))
+		}
+	case result := <-resultC:
+		t.Fatalf("Unexpected result: %v", result)
+	}
+}
+
 func ExampleClient_ContainerWait_withTimeout() {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()