-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Data race fixes & enable testing with -race #564
Conversation
Codecov Report
@@ Coverage Diff @@
## master #564 +/- ##
==========================================
+ Coverage 62.27% 63.03% +0.76%
==========================================
Files 97 97
Lines 7298 7302 +4
==========================================
+ Hits 4545 4603 +58
+ Misses 2504 2451 -53
+ Partials 249 248 -1
Continue to review full report at Codecov.
|
This is just a temporary fix, we should probably remove the shared DefaultCompiler altogether if possible. If the babel parsing is taking the majority of the time, we could just do that in an init function and then create "compiler" runtimes based on it.
I don't think this would have been a problem with real-world usage of k6 (for now), but the way metrics are defined should probably be refactored to avoid shared global variables.
Resumed this after a brief pause... I believe that all data races that I've been able to find have been fixed, so merging this pull request should close #536 now. I think we'll very likely find more race conditions when we start on #537, but we'll see. Also, I don't like some of the fixes I've done (like this and especially this), but fixing them properly would involve a lot of refactoring and should probably be in separate tasks/pull requests. I've left |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, well done, just a tiny typo in a comment!
lib/models.go
Outdated
return group, nil | ||
} | ||
return group, nil | ||
} | ||
|
||
// Create a check belonging to this group. | ||
// Check creates a chold check belonging to this group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "chold" supposed to be "child" perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep... will fix in a minute and merge
@na-- Also, don't forget to add entries in |
This fixes several race conditions like #207 and this one (though with atomics rather than locks or an even loop) and allows us to run the test suite with
-race
by default.After attempting to run some tests with
t.Parallel()
, a new issue surfaced. If we uncomment this line, a other seemingly even more serious race conditions are revealed. So either my assumptions about the concurrency expectations are incorrect and we can't implement a lot of the optimizations in #537, or we have more race conditions to fix.Fixes #207 and also fixes a big part of #536