Repository navigation
fix(client): close response bodies in sendRequest to stop connection-pool exhaustion - #61
Draft
spbsoluble wants to merge 1 commit into
Draft
spbsoluble wants to merge 1 commit into
spbsoluble wants to merge 1 commit into
Conversation
…pool exhaustion sendRequest returned successful responses with the body open and most callers never closed it, and the error paths never closed it either. With a per-host connection cap on the shared transport, each leaked body pinned a pooled connection, and once the cap was reached the next request blocked forever in net/http.(*Transport).getConn. On success the body is now buffered, the original closed, and an in-memory reader returned (Close is a no-op, so callers may still call it). Error paths drain a bounded amount and close the body. Adds a regression test using a TLS httptest server and a transport with MaxConnsPerHost: 2; four cases wedge before the fix and all pass after.
Collaborator
Author
|
Cross-reference: terraform-provider-keyfactor#219 / #223 depend on stable long-running client behavior. This fix needs a release, followed by a version bump in the provider. Validation note: validated end-to-end: with the patched client a provider run that previously stalled and took ~2h43m completed in ~18 min. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Client.sendRequestreturns successful (200/204) responses withresp.Bodystill open, and most callers decode the body without closing it. The non-success branches also return an error without ever closing the body. Each unclosed, not-fully-read body pins its connection in the transport's pool.The auth-client transports that this client builds on cap connections per host (
MaxConnsPerHost, 10 inkeyfactor-auth-client-gov1.6.1-rc.0), and the client is cached once per process. After the cap is reached by leaked bodies, the next request blocks indefinitely innet/http.(*Transport).getConn, so a long-running consumer (for example a Terraform provider run) hangs instead of failing.Fix
sendRequestreads the whole body, closes the original (returning the connection to the pool), and setsresp.Bodyto an in-memory reader whoseCloseis a no-op. Callers may still callClose; they just no longer have to.sendRequestowns the body, so it drains (bounded to 64 KiB) and closes it before returning.Tradeoff
One extra in-memory copy of each response for the duration of the call. Existing callers already decode the full body into memory, so peak usage rises by at most one raw copy of the response. If very large streaming downloads are added later, they would need a separate path.
Tests
New
v3/api/response_body_leak_test.gouses a TLShttptestserver and a transport withMaxConnsPerHost: 2, then issues more requests than the cap. It coverssendRequestfor 200 (unread, chunked, fully decoded), 204, and 400/401/404/500 responses, plusDownloadCertificate. Four cases wedge before the fix; all pass after.cd v3 && GOWORK=off go test ./...passes.Possible follow-up
A per-request/configurable client timeout so that any future pool exhaustion surfaces as an error instead of an indefinite wait.