fix: resolve context leak, unseeded jitter, and broken test utility#14
fix: resolve context leak, unseeded jitter, and broken test utility#14lupppig wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughTwo files modified to improve resource cleanup and randomness handling. In Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Updated signed commit message template in CLA workflow.
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/testutil/testutil.go (1)
11-24: Close the HTTP response body to avoid leaking connections.The response body from
http.Get(url)must be closed to prevent connection leaks and allow the transport to reuse keep-alive connections.🐛 Proposed fix
- if _, err := http.Get(url); err != nil { + resp, err := http.Get(url) + if resp != nil { + defer resp.Body.Close() + } + if err != nil {internal/requestconfig/requestconfig.go (1)
472-484: Canceling the timeout context immediately before reading the response body will cause context-related errors during body reads.When
cancel()is called at line 480 right afterhandler(req), the context becomes cancelled and its error state becomes non-nil. This interferes with subsequent body reading operations (at lines 529 and 560), which may fail withcontext cancellederrors. According to Go'snet/httpsemantics, the request context controls the entire lifetime including response body reading, so the cancellation must be deferred until after the body is fully read or before retrying.The fix should:
- Only cancel the timeout context if it was actually cancelled during the request (move the cancel calls into the error-handling paths)
- Delay cancellation until after body reading completes (after
io.ReadAllcalls)- Cancel before retrying (to prevent accumulation of context resources)
- Rely on the existing
bodyWithTimeoutwrapper for cases where the body is returned without being read immediatelyProposed fix (defer cancel until after body operations)
res, err = handler(req) - - // Close the timeout context for this attempt if one was created - if cancel != nil { - cancel() - cancel = nil - } if ctx != nil && ctx.Err() != nil { + if cancel != nil { + cancel() + cancel = nil + } return ctx.Err() } // Close the response body before retrying to prevent connection leaks if res != nil && res.Body != nil { res.Body.Close() } + if cancel != nil { + cancel() + cancel = nil + } time.Sleep(retryDelay(res, retryCount)) if err != nil { + if cancel != nil { + cancel() + cancel = nil + } return err } contents, err := io.ReadAll(res.Body) res.Body.Close() + if cancel != nil { + cancel() + cancel = nil + } contents, err := io.ReadAll(res.Body) res.Body.Close() + if cancel != nil { + cancel() + cancel = nil + }
🤖 Fix all issues with AI agents
In @.github/workflows/cla.yml:
- Around line 36-37: Replace the hard-coded CLA commit message under the
signed-commit-message key so it uses CLA Assistant Lite template variables
(e.g., $contributorName and $pullRequestNo) instead of "Darasimi Kelani" and
"#14"; update the string to reference $contributorName and $pullRequestNo
(optionally include $owner and $repo) so each signature records the correct
contributor and PR number.
There was a problem hiding this comment.
@lupppig The PR is still showing this file is being modified, try rebasing the PR.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/requestconfig/requestconfig.go`:
- Around line 479-483: In the ctx.Err() and handler error-return paths, ensure
any non-nil *http.Response returned by the custom HTTPDoer is closed to avoid
connection leaks: before calling cancel() or returning on error, check if resp
!= nil and call resp.Body.Close() (ignoring the close error) and then set resp =
nil; apply this change in the ctx.Err() branch (where cancel is cleared) and in
the handler error-return branch (where handler returns err) so both error paths
mirror the retry-loop cleanup that currently closes resp.Body.
🧹 Nitpick comments (1)
internal/testutil/testutil.go (1)
11-14: Add a timeout to the test server probe.
http.Getuses the default client with no timeout, which can hang tests on stalled DNS/connection attempts. Consider a small timeout or context deadline.🔧 Suggested change
import ( "net/http" "os" "strconv" "testing" + "time" ) func CheckTestServer(t *testing.T, url string) bool { - res, err := http.Get(url) + client := &http.Client{Timeout: 5 * time.Second} + res, err := client.Get(url) if err == nil { res.Body.Close() }
| if ctx != nil && ctx.Err() != nil { | ||
| if cancel != nil { | ||
| cancel() | ||
| cancel = nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/requestconfig/requestconfig.go | sed -n '470,495p'Repository: logchimp/logchimp-go
Length of output: 773
🏁 Script executed:
cat -n internal/requestconfig/requestconfig.go | sed -n '450,520p'Repository: logchimp/logchimp-go
Length of output: 2469
🏁 Script executed:
cat -n internal/requestconfig/requestconfig.go | sed -n '520,560p'Repository: logchimp/logchimp-go
Length of output: 1371
🏁 Script executed:
cat -n internal/requestconfig/requestconfig.go | sed -n '400,450p'Repository: logchimp/logchimp-go
Length of output: 1931
🌐 Web query:
http.Client.Do returns response with error Go standard library behavior
💡 Result:
In Go’s standard library (net/http), (*http.Client).Do(req) follows this contract:
- HTTP status codes (including 4xx/5xx) do not produce an error. You get
resp != nil, err == niland must read/closeresp.Body. [1] err != nilmeans either:- a client policy error (most commonly redirect handling via
CheckRedirect), or - a failure to speak HTTP (network/protocol/TLS, etc.). [1]
- a client policy error (most commonly redirect handling via
- The only standard-library case where both
resp != nilanderr != niloccur is whenCheckRedirectfails (including the default redirect policy such as “stopped after too many redirects”). In that case,resp.Bodyis already closed. [1][2] - Any error returned by
Dois a*url.Error(useful forTimeout()checks, etc.). [1]
Practical handling pattern:
resp, err := client.Do(req)
if err != nil {
// resp may be non-nil only on CheckRedirect failure; Body is already closed then.
return err
}
defer resp.Body.Close()Sources: [1] net/http docs for Client.Do (pkg.go.dev) (pkg.go.dev), [2] source-level explanation/examples of the redirect special-case (stackoverflow.com)
Citations:
- 1: https://pkg.go.dev/net/http?utm_source=openai
- 2: https://stackoverflow.com/questions/46423385/when-using-go-http-client-do-method-can-the-httpresponse-and-the-error-be-not-n?utm_source=openai
🏁 Script executed:
rg -n "CustomHTTPDoer" internal/requestconfig/requestconfig.goRepository: logchimp/logchimp-go
Length of output: 166
🏁 Script executed:
rg -B 5 -A 5 "interface.*HTTPDoer\|type HTTPDoer" internal/Repository: logchimp/logchimp-go
Length of output: 46
🏁 Script executed:
rg -n "HTTPDoer" --type goRepository: logchimp/logchimp-go
Length of output: 465
🏁 Script executed:
cat -n internal/requestconfig/requestconfig.go | sed -n '230,240p'Repository: logchimp/logchimp-go
Length of output: 561
🏁 Script executed:
cat -n internal/requestconfig/requestconfig.go | sed -n '1,30p'Repository: logchimp/logchimp-go
Length of output: 889
Close response bodies in all error paths to avoid connection leaks with custom HTTPDoer implementations.
While http.Client.Do typically doesn't return non-nil response with context errors, this code supports custom HTTPDoer implementations that may violate this contract. Additionally, the code inconsistently closes response bodies: it properly closes in the retry loop (line 506-508) but not in error paths.
Two locations need fixing:
- Line 479-484:
ctx.Err()return path - Line 529-534:
handlererror return path
🐛 Suggested fix
if ctx != nil && ctx.Err() != nil {
+ if res != nil && res.Body != nil {
+ res.Body.Close()
+ }
if cancel != nil {
cancel()
cancel = nil
}
return ctx.Err()
}
...
if err != nil {
+ if res != nil && res.Body != nil {
+ res.Body.Close()
+ }
if cancel != nil {
cancel()
cancel = nil
}
return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ctx != nil && ctx.Err() != nil { | |
| if cancel != nil { | |
| cancel() | |
| cancel = nil | |
| } | |
| if ctx != nil && ctx.Err() != nil { | |
| if res != nil && res.Body != nil { | |
| res.Body.Close() | |
| } | |
| if cancel != nil { | |
| cancel() | |
| cancel = nil | |
| } |
🤖 Prompt for AI Agents
In `@internal/requestconfig/requestconfig.go` around lines 479 - 483, In the
ctx.Err() and handler error-return paths, ensure any non-nil *http.Response
returned by the custom HTTPDoer is closed to avoid connection leaks: before
calling cancel() or returning on error, check if resp != nil and call
resp.Body.Close() (ignoring the close error) and then set resp = nil; apply this
change in the ctx.Err() branch (where cancel is cleared) and in the handler
error-return branch (where handler returns err) so both error paths mirror the
retry-loop cleanup that currently closes resp.Body.
2 participants
This fixes several issues in the SDK: 1. A context leak in Execute() where the cancel function was deferred incorrectly. 2. Unseeded random jitter in retryDelay(). 3. Improved error reporting in CheckTestServer.
Summary by CodeRabbit
Release Notes
Performance & Reliability
Tests
✏️ Tip: You can customize this high-level summary in your review settings.