Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

add a Clone function #16

Merged
merged 3 commits into from
Dec 14, 2018
Merged

add a Clone function #16

merged 3 commits into from
Dec 14, 2018

Conversation

Stebalien
Copy link
Member

The user must call this before modifying the config. Given that the config contains slices/maps modifications can modified shared state, even after dereferencing.

(also adds some tests I failed to commit in my Strings-type PR)

@ghost ghost assigned Stebalien Oct 23, 2018
@ghost ghost added the status/in-progress In progress label Oct 23, 2018
Stebalien added a commit to ipfs/kubo that referenced this pull request Oct 23, 2018
This fixes the data-race in the config. Depends
on: ipfs/go-ipfs-config#16.

This does not fix #4942 as there's still a
logical race: parallel config updates clobber each other.

However, we'll still need the Clone function for the new `--dry-run` flag
introduced in #5455.

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

LGTM. I would add one more test though, just to be complete.

config_test.go Outdated Show resolved Hide resolved
@ghost ghost assigned magik6k Nov 6, 2018
Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

(pushed the test @kevina requested)

config.go Outdated
@@ -110,3 +110,19 @@ func ToMap(conf *Config) (map[string]interface{}, error) {
}
return m, nil
}

// Clone copies the config. Use when when updating.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: remove the extra when

Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

One typo in comment but otherwise LGTM

Stebalien and others added 3 commits December 13, 2018 16:03
The user must call this before modifying the config. Given that the config
contains slices/maps modifications can modified *shared* state, even after
dereferencing.
(missed in my previous PR)
@Stebalien Stebalien merged commit e04e9fd into master Dec 14, 2018
@ghost ghost removed the status/in-progress In progress label Dec 14, 2018
@Stebalien Stebalien deleted the fix/thread-safety branch December 14, 2018 00:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants