fix: return an error when docker host cannot be retrieved#3613
Open
ash2k wants to merge 1 commit intotestcontainers:mainfrom
Open
fix: return an error when docker host cannot be retrieved#3613ash2k wants to merge 1 commit intotestcontainers:mainfrom
ash2k wants to merge 1 commit intotestcontainers:mainfrom
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughThe changes replace panic-prone Docker host extraction calls ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker_client.go`:
- Around line 80-83: The cache flag dockerInfoSet is being set before host
resolution, so if core.ExtractDockerHost(ctx) fails the function returns an
error while leaving dockerInfoSet true and causing future Info() calls to return
a cached success; update Info() so dockerInfoSet is only set after
ExtractDockerHost succeeds (and any subsequent logging/processing completes) —
e.g., move the assignment that flips dockerInfoSet (or the code that writes
dockerInfo) to after the successful call to core.ExtractDockerHost(ctx) and any
error checks, or explicitly clear/unset dockerInfoSet on error paths of
ExtractDockerHost to ensure the cache is only marked initialized on success.
In `@internal/core/docker_host.go`:
- Around line 96-101: The code uses dockerHostOnce to cache both success and
failure in ExtractDockerHost, causing dockerHostErrCache to be sticky; change
the logic so dockerHostOnce (and dockerHostCache) only records on successful
extraction: call extractDockerHost inside a retryable path and only assign to
dockerHostCache and mark initialization (or call dockerHostOnce.Do) when
extractDockerHost returns nil error; if extractDockerHost returns an error,
return it directly without setting dockerHostErrCache so future calls can retry.
Update the related variables dockerHostOnce, dockerHostCache, dockerHostErrCache
and the call site of extractDockerHost to follow this pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64d4242b-1b52-498c-9c55-6f8f05a93aa9
📒 Files selected for processing (4)
docker_client.gointernal/core/client.gointernal/core/docker_host.goprovider.go
Comment on lines
+80
to
+83
| host, err := core.ExtractDockerHost(ctx) | ||
| if err != nil { | ||
| return dockerInfo, err | ||
| } |
There was a problem hiding this comment.
Avoid marking docker info cache as initialized on host-resolution failure.
Line 82 introduces a normal error return path, but dockerInfoSet is already set earlier (Line 56). If host extraction fails once, subsequent Info() calls can incorrectly return cached success.
Suggested fix (set cache only after successful host extraction/logging path)
info, err := c.Client.Info(ctx)
if err != nil {
return info, fmt.Errorf("failed to retrieve docker info: %w", err)
}
- dockerInfo = info
- dockerInfoSet = true
@@
host, err := core.ExtractDockerHost(ctx)
if err != nil {
- return dockerInfo, err
+ return info, err
}
log.Printf(infoMessage, packagePath,
@@
core.ProcessID(),
)
+ dockerInfo = info
+ dockerInfoSet = true
return dockerInfo, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker_client.go` around lines 80 - 83, The cache flag dockerInfoSet is being
set before host resolution, so if core.ExtractDockerHost(ctx) fails the function
returns an error while leaving dockerInfoSet true and causing future Info()
calls to return a cached success; update Info() so dockerInfoSet is only set
after ExtractDockerHost succeeds (and any subsequent logging/processing
completes) — e.g., move the assignment that flips dockerInfoSet (or the code
that writes dockerInfo) to after the successful call to
core.ExtractDockerHost(ctx) and any error checks, or explicitly clear/unset
dockerInfoSet on error paths of ExtractDockerHost to ensure the cache is only
marked initialized on success.
Comment on lines
+96
to
101
| func ExtractDockerHost(ctx context.Context) (string, error) { | ||
| dockerHostOnce.Do(func() { | ||
| dockerHostCache, dockerHostErrCache = extractDockerHost(ctx) | ||
| }) | ||
|
|
||
| return dockerHostCache | ||
| return dockerHostCache, dockerHostErrCache | ||
| } |
There was a problem hiding this comment.
Do not cache Docker host extraction errors process-wide.
At Line 97, sync.Once stores the first failure forever (dockerHostErrCache). A transient startup failure becomes unrecoverable for the lifetime of the process, even if Docker/environment is fixed later.
Suggested fix (cache success only, allow retry on errors)
var (
- dockerHostCache string
- dockerHostErrCache error
- dockerHostOnce sync.Once
+ dockerHostCache string
+ dockerHostCached bool
+ dockerHostMu sync.Mutex
)
@@
func ExtractDockerHost(ctx context.Context) (string, error) {
- dockerHostOnce.Do(func() {
- dockerHostCache, dockerHostErrCache = extractDockerHost(ctx)
- })
- return dockerHostCache, dockerHostErrCache
+ dockerHostMu.Lock()
+ defer dockerHostMu.Unlock()
+
+ if dockerHostCached {
+ return dockerHostCache, nil
+ }
+
+ host, err := extractDockerHost(ctx)
+ if err != nil {
+ return "", err
+ }
+
+ dockerHostCache = host
+ dockerHostCached = true
+ return dockerHostCache, nil
}Also applies to: 34-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/docker_host.go` around lines 96 - 101, The code uses
dockerHostOnce to cache both success and failure in ExtractDockerHost, causing
dockerHostErrCache to be sticky; change the logic so dockerHostOnce (and
dockerHostCache) only records on successful extraction: call extractDockerHost
inside a retryable path and only assign to dockerHostCache and mark
initialization (or call dockerHostOnce.Do) when extractDockerHost returns nil
error; if extractDockerHost returns an error, return it directly without setting
dockerHostErrCache so future calls can retry. Update the related variables
dockerHostOnce, dockerHostCache, dockerHostErrCache and the call site of
extractDockerHost to follow this pattern.
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
1 participant
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.
What does this PR do?
When I run some of my tests without Docker running, they panic. I'd like them to just return an error from
testcontainers.Run()(and everything that is built on top).Why is it important?
It's not useful to get a panic when just a normal error is enough. Panic should be reserved for programming errors/etc - "Docker is not running" is not that.
Related issues