Response: Remove odd response body discard reader#563
Response: Remove odd response body discard reader#563roosterfish wants to merge 1 commit intocanonical:v3from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies the ParseResponse function by removing unnecessary response body draining logic and associated error logging. Since all HTTP clients in the codebase have DisableKeepAlives: true (preventing connection reuse), draining the response body before closing is redundant. The json.NewDecoder already reads the necessary data, and the defer resp.Body.Close() call (now moved to the beginning) ensures proper cleanup.
Key changes:
- Moved
defer resp.Body.Close()to the beginning ofParseResponsefunction for earlier resource cleanup - Removed redundant
io.Copy(io.Discard, resp.Body)call since connection reuse is disabled - Removed inappropriate error logging from a library function (logging should be handled by callers)
- Cleaned up unused imports (
io,log/slog, and internal log package)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The ParseResponse function always reads from the response which invalidates the purpose of the body discard reader. Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
roosterfish
force-pushed
the
fix_response_parser
branch
from
51be833 to
411f5a8
Compare
tugbataluy
left a comment
tugbataluy
left a comment
There was a problem hiding this comment.
Approved. As fat as I understand, this is a focused cleanup that eliminates unnecessary logic, improves architectural separation, and ensures errors are handled properly at the caller level rather than being logged internally.
|
I'll leave it open as I want to verify it further. Also I want to investigate why this was added in the first place. |
markylaing
left a comment
markylaing
left a comment
There was a problem hiding this comment.
Good to double check where this came from though I suspect it was just copied from LXD or elsewhere.
Good to get rid of this cruft though, any errors that result from reading the response body would be caught during the JSON decoding.
| if err != nil { | ||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("Failed to fetch %q: %q", resp.Request.URL.String(), resp.Status) | ||
| } |
There was a problem hiding this comment.
Can we also remove this? Or at least allow a range of response codes 200-299? It seems weird that if someone adds a microcluster endpoint like an operation returning a 202 then this function will behave differently in the error scenario
3 participants
The ParseResponse function always reads from the response which invalidates the purpose of the body discard reader.
It's good Go practice to always read the response and then close the body but not anymore required in this case.
As a neat side effect we remove the logging statement from the client func which is odd anyway as such an error should rather be returned directly and not logged as this func is not only used as part of the daemon (which has an initialized logger) but can also be called by clients directly.