Skip to content
New issue

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

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

Already on GitHub? # to your account

fix config data race #5634

Merged
merged 2 commits into from
Dec 20, 2018
Merged

fix config data race #5634

merged 2 commits into from
Dec 20, 2018

Conversation

Stebalien
Copy link
Member

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.

@Stebalien Stebalien requested a review from Kubuxu as a code owner October 23, 2018 17:10
@ghost ghost assigned Stebalien Oct 23, 2018
@ghost ghost added the status/in-progress In progress label Oct 23, 2018
@Stebalien
Copy link
Member Author

The tests won't pass till the config change has been bubbled.

@Stebalien Stebalien force-pushed the fix/config-data-race branch from 7407a66 to e9f98ed Compare December 14, 2018 00:20
@Stebalien Stebalien changed the title [WIP]: fix config data race fix config data race Dec 14, 2018
@Stebalien Stebalien requested a review from magik6k December 14, 2018 00:24
@Stebalien Stebalien force-pushed the fix/config-data-race branch from e9f98ed to 87b6615 Compare December 14, 2018 00:24
*r.config = *updated // copy so caller cannot modify this private config
// Do not use `*r.config = ...`. This will modify the *shared* config
// returned by `r.Config`.
r.config = updated
Copy link
Member Author

Choose a reason for hiding this comment

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

The actual race.

@Stebalien Stebalien force-pushed the fix/config-data-race branch 2 times, most recently from 9219ec8 to 9e0bc2d Compare December 20, 2018 16:35
Copy link
Contributor

@eingenito eingenito left a comment

Choose a reason for hiding this comment

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

Looks good.

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
This fixes a data-race in the config.

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

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@Stebalien Stebalien force-pushed the fix/config-data-race branch from 9e0bc2d to 56b6277 Compare December 20, 2018 18:07
@Stebalien Stebalien merged commit 9e8cc06 into master Dec 20, 2018
@ghost ghost removed the status/in-progress In progress label Dec 20, 2018
@Stebalien Stebalien deleted the fix/config-data-race branch December 20, 2018 19:09
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config is not thread-safe
2 participants