Skip to content

fix innocuous data-race when config.Load called in parallel - #3264

Merged
thaJeztah merged 1 commit into
docker:masterfrom
coryb:fix-data-race
Jan 27, 2022
Merged

fix innocuous data-race when config.Load called in parallel#3264
thaJeztah merged 1 commit into
docker:masterfrom
coryb:fix-data-race

Conversation

@coryb

Copy link
Copy Markdown
Contributor

Locking was removed in #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.

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>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #3264 (b5f4a6e) into master (c758c3e) will increase coverage by 0.00%.
The diff coverage is 88.88%.

@@ Coverage Diff @@## master #3264 +/- ##
=======================================
Coverage 57.98% 57.98% =======================================
Files 302 302 Lines 21754 21757 +3 =======================================
+ Hits 12613 12616 +3 
Misses 8218 8218 Partials 923 923 

@thaJeztah

Copy link
Copy Markdown
Member

I recall we reverted it after a post-merge review, and we thought it wasn't needed. But the context load code is overly complicated with various conditions, so it doesn't surprise me it was needed after all 😁

@thaJeztahthaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@tonistiigi PTAL

@crazy-maxcrazy-max left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also encountered this one in a test suite.

@thaJeztah

Copy link
Copy Markdown
Member

Yes, let's get this one in (we can probably remove this stuff after the next release)

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.

4 participants

@coryb@codecov-commenter@thaJeztah@crazy-max