Revert "Don't read entire HTTP response body until necessary."

See fsouza#378. This reverts commit edc2bd3.
mikemackintosh · Sep 11, 2015 · c9ad0ce · c9ad0ce
1 parent 34eaaf5
commit c9ad0ce
Show file tree

Hide file tree

Showing 9 changed files with 209 additions and 249 deletions.
diff --git a/auth.go b/auth.go
@@ -127,10 +127,12 @@ func (c *Client) AuthCheck(conf *AuthConfiguration) error {
 	if conf == nil {
 		return fmt.Errorf("conf is nil")
 	}
-	resp, err := c.do("POST", "/auth", doOptions{data: conf})
+	body, statusCode, err := c.do("POST", "/auth", doOptions{data: conf})
 	if err != nil {
 		return err
 	}
-	resp.Body.Close()
+	if statusCode > 400 {
+		return fmt.Errorf("auth error (%d): %s", statusCode, body)
+	}
 	return nil
 }
diff --git a/client.go b/client.go
@@ -334,27 +334,27 @@ func (c *Client) checkAPIVersion() error {
 // See https://goo.gl/kQCfJj for more details.
 func (c *Client) Ping() error {
 	path := "/_ping"
-	resp, err := c.do("GET", path, doOptions{})
+	body, status, err := c.do("GET", path, doOptions{})
 	if err != nil {
 		return err
 	}
-	if resp.StatusCode != http.StatusOK {
-		return newError(resp)
+	if status != http.StatusOK {
+		return newError(status, body)
 	}
 	return nil
 }
 
 func (c *Client) getServerAPIVersionString() (version string, err error) {
-	resp, err := c.do("GET", "/version", doOptions{})
+	body, status, err := c.do("GET", "/version", doOptions{})
 	if err != nil {
 		return "", err
 	}
-	defer resp.Body.Close()
-	if resp.StatusCode != http.StatusOK {
-		return "", fmt.Errorf("Received unexpected status %d while trying to retrieve the server version", resp.StatusCode)
+	if status != http.StatusOK {
+		return "", fmt.Errorf("Received unexpected status %d while trying to retrieve the server version", status)
 	}
 	var versionResponse map[string]interface{}
-	if err := json.NewDecoder(resp.Body).Decode(&versionResponse); err != nil {
+	err = json.Unmarshal(body, &versionResponse)
+	if err != nil {
 		return "", err
 	}
 	if version, ok := (versionResponse["ApiVersion"]).(string); ok {
@@ -368,24 +368,24 @@ type doOptions struct {
 	forceJSON bool
 }
 
-func (c *Client) do(method, path string, doOptions doOptions) (*http.Response, error) {
+func (c *Client) do(method, path string, doOptions doOptions) ([]byte, int, error) {
 	var params io.Reader
 	if doOptions.data != nil || doOptions.forceJSON {
 		buf, err := json.Marshal(doOptions.data)
 		if err != nil {
-			return nil, err
+			return nil, -1, err
 		}
 		params = bytes.NewBuffer(buf)
 	}
 	if path != "/version" && !c.SkipServerVersionCheck && c.expectedAPIVersion == nil {
 		err := c.checkAPIVersion()
 		if err != nil {
-			return nil, err
+			return nil, -1, err
 		}
 	}
 	req, err := http.NewRequest(method, c.getURL(path), params)
 	if err != nil {
-		return nil, err
+		return nil, -1, err
 	}
 	req.Header.Set("User-Agent", userAgent)
 	if doOptions.data != nil {
@@ -400,29 +400,33 @@ func (c *Client) do(method, path string, doOptions doOptions) (*http.Response, e
 		var dial net.Conn
 		dial, err = c.Dialer.Dial(protocol, address)
 		if err != nil {
-			return nil, err
+			return nil, -1, err
 		}
 		defer dial.Close()
 		breader := bufio.NewReader(dial)
 		err = req.Write(dial)
 		if err != nil {
-			return nil, err
+			return nil, -1, err
 		}
 		resp, err = http.ReadResponse(breader, req)
 	} else {
 		resp, err = c.HTTPClient.Do(req)
 	}
 	if err != nil {
 		if strings.Contains(err.Error(), "connection refused") {
-			return nil, ErrConnectionRefused
+			return nil, -1, ErrConnectionRefused
 		}
-		return nil, err
+		return nil, -1, err
+	}
+	defer resp.Body.Close()
+	body, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		return nil, -1, err
 	}
-
 	if resp.StatusCode < 200 || resp.StatusCode >= 400 {
-		return nil, newError(resp)
+		return nil, resp.StatusCode, newError(resp.StatusCode, body)
 	}
-	return resp, nil
+	return body, resp.StatusCode, nil
 }
 
 type streamOptions struct {
@@ -508,7 +512,11 @@ func (c *Client) stream(method, path string, streamOptions streamOptions) error
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode < 200 || resp.StatusCode >= 400 {
-		return newError(resp)
+		body, err := ioutil.ReadAll(resp.Body)
+		if err != nil {
+			return err
+		}
+		return newError(resp.StatusCode, body)
 	}
 	if streamOptions.useJSONDecoder || resp.Header.Get("Content-Type") == "application/json" {
 		// if we want to get raw json stream, just copy it back to output
@@ -742,13 +750,8 @@ type Error struct {
 	Message string
 }
 
-func newError(resp *http.Response) *Error {
-	defer resp.Body.Close()
-	data, err := ioutil.ReadAll(resp.Body)
-	if err != nil {
-		return &Error{Status: resp.StatusCode, Message: fmt.Sprintf("cannot read body, err: %v", err)}
-	}
-	return &Error{Status: resp.StatusCode, Message: string(data)}
+func newError(status int, body []byte) *Error {
+	return &Error{Status: status, Message: string(body)}
 }
 
 func (e *Error) Error() string {

diff --git a/client_test.go b/client_test.go
@@ -5,7 +5,6 @@
 package docker
 
 import (
-	"bytes"
 	"fmt"
 	"io/ioutil"
 	"net"
@@ -187,12 +186,7 @@ func TestGetURL(t *testing.T) {
 }
 
 func TestError(t *testing.T) {
-	fakeBody := ioutil.NopCloser(bytes.NewBufferString("bad parameter"))
-	resp := &http.Response{
-		StatusCode: 400,
-		Body:       fakeBody,
-	}
-	err := newError(resp)
+	err := newError(400, []byte("bad parameter"))
 	expected := Error{Status: 400, Message: "bad parameter"}
 	if !reflect.DeepEqual(expected, *err) {
 		t.Errorf("Wrong error type. Want %#v. Got %#v.", expected, *err)