Skip to content
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

"Cannot extend Go slice" when modifying setup data #684

Closed
MichaelWeirWeather opened this issue Jun 29, 2018 · 4 comments
Closed

"Cannot extend Go slice" when modifying setup data #684

MichaelWeirWeather opened this issue Jun 29, 2018 · 4 comments
Assignees

Comments

@MichaelWeirWeather
Copy link

When I create setup-data containing an array in the setup() function, I get an exception when I try to add to that array in either the test function or the teardown function.

This code will demonstrate the problem:

export function setup() {
	const setupData = { array: [] };
	setupData.array.push("Can push in setup()");
	console.log("setup returning=" + JSON.stringify(setupData, null, 2));
	return setupData;
}

export function teardown(setupData) {
	//setupData.array.push("Cannot push in teardown function");
	console.log("teardown. setupData=" + JSON.stringify(setupData, null, 2));
}

export default function(setupData) {
	console.log("test function. setupData before=" + JSON.stringify(setupData, null, 2));
	setupData.array.push("Cannot push in test function");
	console.log("test function. setupData after=" + JSON.stringify(setupData, null, 2));
}

When I execute this test, I get the following in sysout:

  execution: local
     output: -
     script: ./k6_bug_demo.js

    duration: -,  iterations: 1
         vus: 1, max: 1

INFO[0000] setup returning={
  "array": [
    "Can push in setup()"
  ]
}
INFO[0000] test function. setupData before={
  "array": [
    "Can push in setup()"
  ]
}
ERRO[0000] TypeError: Cannot extend Go slice
        at push (native)
        at C:\...\k6_bug_demo.js:16:23(19)

INFO[0000] teardown. setupData={
  "array": [
    "Can push in setup()"
  ]
}
    done [==========================================================] 1 / 1

    iterations...: 1 +Inf/s
    vus..........: 1 min=1 max=1
    vus_max......: 1 min=1 max=1
@na--
Copy link
Member

na-- commented Jul 2, 2018

Thanks for the report, I confirmed this locally and briefly investigated the possible cause. It's strange since we use the goja's ToValue function to transform the Go setupData value to a JS one... I think the reason for the bug might be found in the following bit of goja ToValue documentation:

[]interface{} is converted into a host object that behaves largely like a JavaScript Array, however it's not extensible because extending it can change the pointer so it becomes detached from the original.
*[]interface{} same as above, but the array becomes extensible.
I think this may be related

As I recently mentioned in another somewhat related issue, we encode the result of setup() to JSON and immediately back to a interface{} to get rid of any non-data elements in the result. But it seems like this might make the arrays in that data non-extensible...

To be fair, changes to the setupData from the default function won't actually propagate to other VUs (or subsequently to the teardown() call), since the different VUs run on different runtimes (and in the cloud/cluster execution, on totally different machines), so this isn't a very damaging bug. I'll remove the high prio label, but we probably should still fix this when we improve the other things mentioned in the issue I referenced above.

@na-- na-- removed the high prio label Jul 2, 2018
@MichaelWeirWeather
Copy link
Author

I encountered the problem while trying to figure a way of getting data from the many __VU/__ITER executions back to the teardown function.

It would be nice if it were possible for the default function to return data, and for that data to be collected and passed to the teardown function.

@na--
Copy link
Member

na-- commented Jul 4, 2018

Unfortunately even if we fix the specific issue you pointed out (TypeError: Cannot extend Go slice), you still wouldn't be able to do this. It's a whole another issue, somewhat related to this one (and the ones linked in it). It would require some major architectural changes of the current codebase and it would be especially challenging to implement in the distributed cloud and cluster execution modes.

Still, I can see the value in that functionality and the potential use cases. Please create another issue for it, since it's quite possible that if we implement the other issues I linked to, we could similarly implement this as well. Or maybe we can think of another solution that accomplishes the same things but would have better performance characteristics and be easier to implement reliably for distributed execution. For example, something messaging-related for explicit pushing of things you want to have access to later.

@na--
Copy link
Member

na-- commented Oct 10, 2018

Unfortunately this doesn't appear very fixable right now 😞 . We either have to change the goja internals, or, more likely, write our own generic JSON unmarshalling code that returns any JSON arrays as *[]interface{} instead of []interface{}.

Considering that if we could, I'd prefer to actually make the data in export default function(data) {/* ... */} immutable (goja doesn't seem to support that as well), I don't think this is a big issue... Generally, users shouldn't have any reason to modify the setup data, you can use VU-local variables instead. For performance reasons we also can't simply reset the setup data before every iteration, so the current semi-immutable state will have to suffice - this can just be something undefined that we won't officially support...

@na-- na-- closed this as completed Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants