Skip to content

cli/config: add synchronisation for configDir (Dir, SetDir)#4384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thaJeztah merged 1 commit into from
Jun 28, 2023
Merged

cli/config: add synchronisation for configDir (Dir, SetDir) #4384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions cli/config/config.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,13 @@ var (
configDir string
)

// resetConfigDir is used in testing to reset the "configDir" package variable
// and its sync.Once to force re-lookup between tests.
func resetConfigDir() {
configDir = ""
initConfigDir = new(sync.Once)
}

// Dir returns the directory the configuration file is stored in
func Dir() string {
initConfigDir.Do(func() {
Expand All@@ -45,6 +52,8 @@ func ContextStoreDir() string {

// SetDir sets the directory the configuration file is stored in
func SetDir(dir string) {
// trigger the sync.Once to synchronise with Dir()
initConfigDir.Do(func() {})

@vvolandvvolandJun 28, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't look too closely how these are called, but if both Dir and SetDir are called at the same time from different goroutines, we still might have a race condition:

  1. Thread X enters SetDir
56: initConfigDir.Do(func() {})
  1. Thread Y enters Dir
39: initConfigDir.Do(func() {
configDir=os.Getenv("DOCKER_CONFIG")
ifconfigDir=="" {
configDir=filepath.Join(homedir.Get(), configFileDir)
}
})
returnconfigDir
}
  1. Thread X enters the Do and "locks" initConfigDir first.
  2. Thread Y is blocked and waits for X to finish the Do block.
  3. Thread X leaves the initConfigDir and unblocks Y, but doesn't enter the 57: configDir = filepath.Clean(dir) yet
  4. Thread Y executes 45: return configDir and returns empty directory.

@vvolandvvolandJun 28, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be ignored if we definitely don't (and won't) call these concurrently though.

@thaJeztahthaJeztahJun 28, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we would hit that case in our code, so I'm "fine" with it not 100% being race-free (per our discussion on slack; there's no deadlock, so not "worse" than before).

FWIW; I went through a couple of options;

  • a mutex for the value
  • using sync/atomic.Value

Both of those performed a lot less, and both are still somewhat tapering over the "big elephant in the room"; we shouldn't use a package-level variable for this.

So I'm also looking at removing this variable altogether in favour of this being a config for the client that's configured, as (looking at the code) that's where it's used, and I think the expectation is to set the location for the client that's constructed;

cli/cli/command/cli.go

Lines 209 to 221 in a26e601

// Initialize the dockerCli runs initialization that must happen after command
// line flags are parsed.
func (cli*DockerCli) Initialize(opts*cliflags.ClientOptions, ops...InitializeOpt) error {
for_, o:=rangeops {
iferr:=o(cli); err!=nil {
returnerr
}
}
cliflags.SetLogLevel(opts.LogLevel)
ifopts.ConfigDir!="" {
config.SetDir(opts.ConfigDir)
}

configDir = filepath.Clean(dir)
}

Expand Down
9 changes: 9 additions & 0 deletions cli/config/config_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -382,3 +382,12 @@ func TestConfigPath(t *testing.T) {

SetDir(oldDir)
}

// TestSetDir verifies that Dir() does not overwrite the value set through
// SetDir() if it has not been run before.
func TestSetDir(t *testing.T) {
const expected = "my_config_dir"
resetConfigDir()
SetDir(expected)
assert.Check(t, is.Equal(Dir(), expected))
}