Dont share setup data #799
Conversation
This removes the ability to change setupData between calls of the default function. This removes a race condition where multiple VUs are changin the setupData at the same time. This change also makes certain that each call to the default function and teardown get a setupData that has the same contents making it less confusing if you change the setupData inside any of them for some reason.
The behaviour we want is if there is a setup function and it returns a value it should be provided to the default function and the teardown. Otherwise they should be provided with a null value.
|
|
js/runner.go
Outdated
| // the local executor further by deferring SetVUsMax() calls to within the Run() function. | ||
| if u.setupData == nil && u.Runner.setupData != nil { | ||
| u.setupData = u.Runtime.ToValue(u.Runner.setupData) | ||
| // Always unmarshall the setupData so that it doesn't change between calls |
There was a problem hiding this comment.
Hmm I don't think that we should Unmarshal the setup data before each iteration, it seems like it would be a waste of CPU resources in the middle of the test, especially when the setup data is a big object. Using something similar to the old logic, where the setupData is injected before the beginning of the first iteration of a VU, should work better even when we decode it from JSON.
There was a problem hiding this comment.
This will mean that changes between iterations will be seen withing the same VU
There was a problem hiding this comment.
Yep, shouldn't be an issue, since there couldn't be any data races that way. I think we shouldn't advertise it in the docs, since we might want to change it in the future, but it should be fine - other things in VUs stick around between iterations as well.
js/runner_test.go
Outdated
|
|
||
| func testSetupDataHelper(t *testing.T, src *lib.SourceData) { | ||
| expScriptOptions := lib.Options{SetupTimeout: types.NullDurationFrom(1 * time.Second)} | ||
| r1, err := New(src, afero.NewMemMapFs(), lib.RuntimeOptions{Env: map[string]string{"expectedSetupTimeout": "1s"}}) |
There was a problem hiding this comment.
expectedSetupTimeout isn't actually used
js/runner_test.go
Outdated
| require.NoError(t, err) | ||
| require.Equal(t, expScriptOptions, r1.GetOptions()) | ||
|
|
||
| r2, err := NewFromArchive(r1.MakeArchive(), lib.RuntimeOptions{Env: map[string]string{"expectedSetupTimeout": "3s"}}) |
There was a problem hiding this comment.
Same here, expectedSetupTimeout seems like a copy-paste error. To be honest, I'm not sure if we actually need r2 at all. I know that a lot of the other tests make an archive from a script like this and test both, but not sure we need to do that everywhere, the only thing it would probably gain from it is extended test execution times.
js/runner_test.go
Outdated
| return | ||
| } | ||
|
|
||
| r2, err := NewFromArchive(r1.MakeArchive(), lib.RuntimeOptions{}) |
There was a problem hiding this comment.
Same note about archives in this test as in the other - not sure testing both archives and scripts would help us catch any errors in this context.
js/runner_test.go
Outdated
| Data: []byte(` | ||
| export let options = { setupTimeout: "1s", myOption: "test" }; | ||
| export default function(data) { | ||
| if (data != null) { |
There was a problem hiding this comment.
If we want to be pedantically JS compatible, data should probably be undefined. goja supports it as easily enough - https://godoc.org/github.com/dop251/goja#Undefined
js/runner_test.go
Outdated
| export let options = { setupTimeout: "1s", myOption: "test" }; | ||
| export function setup() { } | ||
| export default function(data) { | ||
| if (data != null) { |
There was a problem hiding this comment.
Probably data should be undefined here as well. This script prints undefined in browsers as well as node.js:
function f() { };
console.log(f());This is somewhat a performance change so that we don't have to unmarshal possibly huge JSONs in between iterations.
mstoykov
force-pushed
the
dontShareSetupData
branch
from
2a889de to
793e0e1
Compare
Codecov Report
@@ Coverage Diff @@
## master #799 +/- ##
==========================================
+ Coverage 63.66% 63.74% +0.07%
==========================================
Files 103 103
Lines 8464 8489 +25
==========================================
+ Hits 5389 5411 +22
- Misses 2714 2716 +2
- Partials 361 362 +1
Continue to review full report at Codecov.
|
as in #799(which was about the same issue with setup data), we don't actually have problems with a VU seeing changes from previous iterations. It will be too expensive to constantly copy, it won't panic and there are no issues with distributed execution.
as in #799(which was about the same issue with setup data), we don't actually have problems with a VU seeing changes from previous iterations. It will be too expensive to constantly copy, it won't panic and there are no issues with distributed execution.
as in #799(which was about the same issue with setup data), we don't actually have problems with a VU seeing changes from previous iterations. It will be too expensive to constantly copy, it won't panic and there are no issues with distributed execution.
No description provided.