Skip to content

config.Load() remove unneeded locks - #3025

Merged
tiborvass merged 1 commit into
docker:masterfrom
thaJeztah:remove_unneeded_locks
Apr 1, 2021
Merged

config.Load() remove unneeded locks#3025
tiborvass merged 1 commit into
docker:masterfrom
thaJeztah:remove_unneeded_locks

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

These were added in b83bc67 (#2666), but I'm not sure why I added these; they're likely not needed.
- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

These were added in b83bc67, but
I'm not sure why I added these; they're likely not needed.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

Failing because of #2986


=== Failed
=== FAIL: e2e/container TestRunAttachedFromRemoteImageAndRemove (1.91s)
run_test.go:28: assertion failed: --- expected
+++ actual
@@ -3,3 +3,4 @@
Digest: sha256:641b95ddb2ea9dc2af1a0113b6b348ebc20872ba615204fbe12148e98fd6f23d
Status: Downloaded newer image for registry:5000/alpine:test-run-pulls
+Unsupported signal: <nil>. Discarding.
You can run 'go test . -test.update-golden' to automatically update testdata/run-attached-from-remote-and-remove.golden to the new expected value.'

@thaJeztah
thaJeztah marked this pull request as ready for review March 25, 2021 21:53
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@tonistiigi@silvin-lubecki PTAL

@coryb

Copy link
Copy Markdown
Contributor

@thaJeztah I think I figured out why you added this originally, I now get a race error when I run my tests with -race:

WARNING: DATA RACE
Write at 0x00000415c60d by goroutine 511:
github.com/docker/cli/cli/config.Load()
/go/pkg/mod/github.com/docker/cli@v20.10.7+incompatible/cli/config/config.go:114 +0x6b
github.com/docker/cli/cli/config.LoadDefaultConfigFile()
/go/pkg/mod/github.com/docker/cli@v20.10.7+incompatible/cli/config/config.go:152 +0xa4

so the mutex was preventing the mutation of global state. Not sure if there is a better strategy to handle this or if we should add back in the mutex to prevent the race.

coryb added a commit to coryb/cli that referenced this pull request Aug 21, 2021
Locking was removed docker#3025 which
allows for parallel calls to config.Load to modify global state.
The consequence in this case is very mild, but it does trigger a
DATA RACE exception when tests run with `-race` option.
coryb added a commit to coryb/cli that referenced this pull request Aug 21, 2021
Locking was removed docker#3025 which
allows for parallel calls to config.Load to modify global state.
The consequence in this case is very mild, but it does trigger a
DATA RACE exception when tests run with `-race` option.
Signed-off-by: coryb <cbennett@netflix.com>
coryb added a commit to coryb/cli that referenced this pull request Aug 21, 2021
Locking was removed in docker#3025 which
allows for parallel calls to config.Load to modify global state.
The consequence in this case is innocuous, but it does trigger a
`DATA RACE` exception when tests run with `-race` option.
Signed-off-by: coryb <cbennett@netflix.com>
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

Ah! Nice find!

thaJeztah pushed a commit to thaJeztah/cli that referenced this pull request Jan 27, 2022
Locking was removed in docker#3025 which
allows for parallel calls to config.Load to modify global state.
The consequence in this case is innocuous, but it does trigger a
`DATA RACE` exception when tests run with `-race` option.
Signed-off-by: coryb <cbennett@netflix.com>
(cherry picked from commit b5f4a6e)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
gibson042 pushed a commit to gibson042/docker-cli that referenced this pull request Feb 1, 2022
Locking was removed in docker#3025 which
allows for parallel calls to config.Load to modify global state.
The consequence in this case is innocuous, but it does trigger a
`DATA RACE` exception when tests run with `-race` option.
Signed-off-by: coryb <cbennett@netflix.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah@coryb@tiborvass