client: fix datarace when accessing cli.Version field#50448
Merged
vvoland merged 1 commit intomoby:masterfrom Jul 22, 2025
Merged
client: fix datarace when accessing cli.Version field#50448vvoland merged 1 commit intomoby:masterfrom
vvoland merged 1 commit intomoby:masterfrom
Conversation
Originally I've found this datarace on a project I'm working at. I'm not
able to consistently reproduce this. But by looking at the codebase I
took a chance to fix other 2 possible function that might produce such
data race.
Original stack trace produced when running `go test -race` on GH CI:
```
WARNING: DATA RACE
Write at 0x00c0005dc688 by goroutine 43:
github.com/docker/docker/client.(*Client).negotiateAPIVersionPing()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/client.go:389 +0x12f
github.com/docker/docker/client.(*Client).checkVersion()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/client.go:298 +0x249
github.com/docker/docker/client.(*Client).getAPIPath()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/client.go:307 +0x76
github.com/docker/docker/client.(*Client).sendRequest()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/request.go:111 +0x9b
github.com/docker/docker/client.(*Client).get()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/request.go:28 +0x736
github.com/docker/docker/client.(*Client).ContainerList()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/container_list.go:47 +0x6f0
Previous read at 0x00c0005dc688 by goroutine 42:
github.com/docker/docker/client.(*Client).ContainerList()
/home/runner/go/pkg/mod/github.com/docker/docker@v28.2.2+incompatible/client/container_list.go:39 +0x5ef
```
Co-authored-by: Luca Rinaldi <lucarin@protonmail.com>
Signed-off-by: Alessio Perugini <alessio@perugini.xyz>
vvoland
reviewed
Jul 21, 2025
Contributor
vvoland
left a comment
There was a problem hiding this comment.
Not sure if it's correct solution - perhaps this should be handled by cli.ClientVersion() and then we should change all cli.version usages to use the accessor?
Overall, I'm concerned about having to explicitly call the checkVersion.
Contributor
Author
|
@vvoland I 💯 agree. Before coming up with this PR, I looked at the codebase, and it seems we have plenty of other places where we're doing the exact same thing. That's how I came up with this PR. But it is definitely error-prone to handle this case like that. For reference, below I'm linking the same usage in a different part of the repo (might be useful for a future refactoring):
|
Contributor
|
Oh, good point! I didn't notice we already do this 🙈 We should take a look into it as a follow up. |
Member
|
This one made changes to the client module, so needed a re-vendor (as we vendor the client module) |
Merged
This was referenced Sep 5, 2025
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
3 participants
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.
Originally I've found this datarace on a project I'm working at. I'm not able to consistently reproduce this. But by looking at the codebase I took a chance to fix other 2 possible function that might produce such data race.
Original stack trace produced when running
go test -raceon GH CI: