client: remove serverResponse and use http.Response directly

Looking in history to learn why this struct existed, shows that this type
was mostly the result of tech-debt accumulating over time;

- originally ([moby@1aa7f13]) most of the request handling was internal;
  the [`call()` function][1] would make a request, read the `response.Body`,
  and return it as a `[]byte` (or an error if one happened).
- some features needed the statuscode, so [moby@a4bcf7e] added an extra
  output variable to return the `response.StatusCode`.
- some new features required streaming, so [moby@fdd8d4b] changed the
  function to return the `response.Body` as a `io.ReadCloser`, instead
  of a `[]byte`.
- some features needed access to the content-type header, so a new
  `clientRequest` method was introduced in [moby@6b2eeaf] to read the
  `Content-Type` header from `response.Headers` and return it as a string.
- of course, `Content-Type` may not be the only header needed, so [moby@0cdc3b7]
  changed the signature to return `response.Headers` as a whole as a
  `http.Header`
- things became a bit unwieldy now, with the function having four (4) output
  variables, so [moby@126529c] chose to refactor this code, introducing a
  `serverResponse` struct to wrap them all, not realizing that all these
  values were effectively deconstructed from the `url.Response`, so now
  re-assembling them into our own "URL response", only preserving a subset
  of the information available.
- now that we had a custom struct, it was possible to add more information
  to it without changing the signature. When there was a need to know the
  URL of the request that initiated the response, [moby@27ef09a] introduced
  a `reqURL` field to hold the `request.URL` which notably also is available
  in `response.Request.URL`.

In short;

- The original implementation tried to (pre-maturely) abstract the underlying
  response to provide a simplified interface.
- While initially not needed, abstracting caused relevant information from
  the response (and request) to be unavailable to callers.
- As a result, we ended up in a situation where we are deconstructing the
  original `url.Response`, only to re-assemble it into our own, custom struct
  (`serverResponsee`) with only a subset of the information preserved.

This patch removes the `serverResponse` struct, instead returning the
`url.Response` as-is, so that all information is preserved, allowing callers
to use the information they need.

There is one follow-up change to consider; commit [moby@589df17] introduced
a `ensureReaderClosed` utility. Before that commit, the response body would
be closed in a more idiomatic way through a [`defer serverResp.body.Close()`][2].
A later change in [docker/engine-api@5dd6452] added an optimization to that
utility, draining the response to allow connections to be reused. While
skipping that utility (and not draining the response) would not be a critical
issue, it may be easy to overlook that utility, and to close the response
body in the "idiomatic" way, resulting in a possible performance regression.

We need to check if that optimization is still relevant or if later changes
in Go itself already take care of this; we should also look if context
cancellation is handled correctly for these. If it's still relevant, we could

- Wrap the the `url.Response` in a custom struct ("drainCloser") to provide
  a `Close()` function handling the draining and closing; this would re-
  introduce a custom type to be returned, so perhaps not what we want.
- Wrap the `url.Response.Body` in the response returned (so, calling)
  `response.Body.Close()` would call the wrapped closer.
- Change the signature of `Client.sendRequest()` (and related) to return
  a `close()` func to handle this; doing so would more strongly encourage
  callers to close the response body.

[1]: 1aa7f1392d/commands.go (L1008-L1027)
[2]: 589df17a1a/api/client/ps.go (L84-L89)
[moby@1aa7f13]: 1aa7f1392d
[moby@a4bcf7e]: a4bcf7e1ac
[moby@fdd8d4b]: fdd8d4b7d9
[moby@6b2eeaf]: 6b2eeaf896
[moby@0cdc3b7]: 0cdc3b7539
[moby@126529c]: 126529c6d0
[moby@27ef09a]: 27ef09a46f
[moby@589df17]: 589df17a1a
[docker/engine-api@5dd6452]: 5dd6452d4d

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This commit is contained in:
Sebastiaan van Stijn
2025-02-09 13:17:41 +01:00
parent 6856bdd5a6
commit 72c91e378d
72 changed files with 259 additions and 243 deletions

View File

@@ -17,14 +17,14 @@ func (cli *Client) ContainerInspect(ctx context.Context, containerID string) (co
return container.InspectResponse{}, err
}
serverResp, err := cli.get(ctx, "/containers/"+containerID+"/json", nil, nil)
defer ensureReaderClosed(serverResp)
resp, err := cli.get(ctx, "/containers/"+containerID+"/json", nil, nil)
defer ensureReaderClosed(resp)
if err != nil {
return container.InspectResponse{}, err
}
var response container.InspectResponse
err = json.NewDecoder(serverResp.body).Decode(&response)
err = json.NewDecoder(resp.Body).Decode(&response)
return response, err
}
@@ -39,13 +39,13 @@ func (cli *Client) ContainerInspectWithRaw(ctx context.Context, containerID stri
if getSize {
query.Set("size", "1")
}
serverResp, err := cli.get(ctx, "/containers/"+containerID+"/json", query, nil)
defer ensureReaderClosed(serverResp)
resp, err := cli.get(ctx, "/containers/"+containerID+"/json", query, nil)
defer ensureReaderClosed(resp)
if err != nil {
return container.InspectResponse{}, nil, err
}
body, err := io.ReadAll(serverResp.body)
body, err := io.ReadAll(resp.Body)
if err != nil {
return container.InspectResponse{}, nil, err
}