Expand ArrayBuffer support

[imiric]

This addresses the backwards compatible changes of #1020, and showcases a workaround for better handling of multipart requests (ref. #747, #843, #1571).

[na--]

hmm while returning the JS equivalent to []byte from open() is not ideal, it was somewhat usable. So I am not sure if we should break it, since some scripts probably depend on the current strange behavior. Why not just add another flag value for an ArrayBuffer response?

[imiric]

Yeah, I also wasn't sure if we should break it.

Why not just add another flag value for an ArrayBuffer response?

:-/ Sure, maybe ab?

[imiric]

Done, see c593025.

[na--]

Currently lib/netext/httpext/ doesn't depend on goja, but more importantly I don't see where we could initialize a throwaway Runtime for this purpose, since doing that in readResponseBody or MakeRequest would be too expensive. We'd have to pre-initialize it in the context or lib.State. :-/

Technically, there's a JS wrapper around the httpext.Response, though it's a very thin one at the moment: https://github.com/loadimpact/k6/blob/c00a85bfea21d748f0f9d11fdbc326087f34c411/js/modules/k6/http/response.go#L36-L44

And here (and in batch()) you should be able to access the Runtime with common.GetRuntime(ctx): https://github.com/loadimpact/k6/blob/c00a85bfea21d748f0f9d11fdbc326087f34c411/js/modules/k6/http/request.go#L116

But yeah, ArrayBuffer as the Response.body should wait for v0.31.0, it's too complicated a change at this stage. I am even hesitating if we should merge this PR or wait for it as well... @mstoykov what do you think?

Uint8Array(res.body)

This is a viable workaround, but we have to benchmark it... I think it would probably involve copying the whole body, so not great about CPU and memory usage. Better than nothing, but I hope we could find something more efficient.

[imiric]

@na-- Thanks, I'll take a look at accessing the Runtime that way.

As for delaying this PR, give me the rest of the day to address these two issues. If we can't get it merged today or early tomorrow, or @mstoykov finds other issues, I'm OK with postponing it.

[codecov-io]

Codecov Report

Merging #1800 (bfdf164) into master (46d53d9) will decrease coverage by 0.03%.
The diff coverage is 85.36%.

@@            Coverage Diff             @@
##           master    #1800      +/-   ##
==========================================
- Coverage   71.61%   71.58%   -0.04%     
==========================================
  Files         181      180       -1     
  Lines       13939    13962      +23     
==========================================
+ Hits         9982     9994      +12     
- Misses       3324     3331       +7     
- Partials      633      637       +4     

Flag	Coverage Δ
ubuntu	71.58% <85.36%> (+<0.01%)	⬆️
windows	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
js/initcontext.go	90.10% <ø> (-2.20%)	⬇️
js/modules/k6/encoding/encoding.go	87.50% <75.00%> (-5.61%)	⬇️
js/modules/k6/crypto/crypto.go	93.96% <80.00%> (-3.24%)	⬇️
js/common/util.go	66.66% <100.00%> (+19.99%)	⬆️
js/modules/k6/http/file.go	100.00% <100.00%> (+20.00%)	⬆️
js/common/initenv.go	86.66% <0.00%> (-13.34%)	⬇️
lib/testutils/minirunner/minirunner.go	76.08% <0.00%> (-4.35%)	⬇️
js/runner.go	80.86% <0.00%> (-0.58%)	⬇️
cmd/ui_windows.go
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46d53d9...bfdf164. Read the comment docs.

[imiric]

I added the response body ArrayBuffer support somewhat in a hurry, especially the tests, so let me know if it can be improved.

[mstoykov]

I am fairly hard for this being post v0.30.0

I am also of the opinion that we should:

1. start with things that take as arguments []byte to make them work with ArrayBuffer instead of start with things that return []byte to return ArrayBuffer.
2. Possibly break all the current things returning []byte to return ArrayBuffer after the previous step is done...

As far as I am concern having 2 binary formats, one of which([]byte) is terrible for usage and not JS-y, is way worse than the (unlikely) breaking of some user scripts(that will also be fixed by a 1 lines).

Also ... I have barely seen anyone use the binary "open" or HTTP responses, and likely they are used just to push them back into something(HTTP, crypto, encoding), on this I am pretty certain because you barely can do anything else with it either way. So as long as those work with ArrayBuffer this will be seamless for people, without us needing to explain that ... "yeah you should just use ArrayBuffer, more or less always, but by default, we shoot you in the foot so we don't break a script we haven't seen that likely was broken and hacky to begin with"

[imiric]

@na-- @mstoykov I added the k6/crypto and k6/encoding changes to support ArrayBuffer, so I think that covers everything mentioned in #1020. I added a couple of new tests, but mostly edited existing ones to deal with ArrayBuffer properly. Let me know if more tests are missing.

We should discuss the breaking changes here, if having open() always return ArrayBuffer is worth it, etc. I agree with Mihail that we'd probably be fine with that for most use cases (e.g. that pass the return value directly to http.post(), etc.), but it will break scripts that did some manual work on []byte, and will be annoying to fix. We probably should anyway, with a big disclaimer in the release notes and docs?

[na--]

I haven't looked at the latest code yet, but have some comments that I hope are not too uninformed...

it will break scripts that did some manual work on []byte, and will be annoying to fix.

I am coming around to open(filename, 'b') returning an ArrayBuffer, i.e. having a breaking change for k6 v0.31.0. Mostly because the fix would probably be quite easy, just wrapping the open() call in new Uint8Array() and then using .buffer in the HTTP request call. Or at least that's what I think...

On another topic, as you know I've gone through most of the old k6 issues, and here is a list of ones related to binary handling or request bodies that I found: #747, #843, #1375, #1382, #1571, #1770, #1784, #1798

I didn't spend a ton of time investigating each issue, but I think that after this PR and grafana/jslib.k6.io#21 are merged, we should go through all of them with a fine-toothed comb. If an issue could be solved by the FormData JS builder, we should post a comment on how to do it and then close the issue. I also made a note that we should add some proper FormData examples in the docs: grafana/k6-docs#203

[mstoykov]

I am coming around to open(filename, 'b') returning an ArrayBuffer, i.e. having a breaking change for k6 v0.31.0. Mostly because the fix would probably be quite easy, just wrapping the open() call in new Uint8Array() and then using .buffer in the HTTP request call. Or at least that's what I think...

you don't need to wrap it or anything - we should be taking ArrayBuffer everywhere we are currently taking []byte so if your code is

var b = open("something.dat", "b");
export default function() {
   http.post(url, b);
}

That will just work if we change to just returning ArrayBuffer.
Which was my whole proposal:

1. wherever we are taking []byte take Arraybuffer (at least make it possible to take ArrayBuffer as well)
2. wherever we are returning []byte, just return ArrayBuffer

as long as users just pushed []byte around it will just work - no breaking change.

The breaking change comes from the fact that you can take the length of a []byte as well as modify individual elements with b[1] = 12 or just get them, but you can't change the length of the []byte either up or down without it becoming []interface{} which then can't be used anywhere 😭 . ArrayBuffer on the other side has byteLength and you need a view like Uint8Array to actually edit the values.

Given my experiments with []byte trying to fix this before we introduced taking ArrayBuffer as an HTTP body ... you can't do much with the above things, and I am certain that whoever hacked something with it will be glad to use ArrayBuffer instead.

My main concern actually is that length is likely to be used in checks and such and will just now ... not work and possibly it will just fail checks without anything.
But I am still learning (even not heavily) toward just biting the bullet and breaking this, so we don't have 2 binary data supports, where the default is useless and not JS-y.

I agree on the issues, I also would prefer if we go through the documentation of the current "magical" ways to use formdata and document that they are not great and possibly should be abandoned for the FormData polyfill. After some testing of the polyfill of course :)

[na--]

@mstoykov, I know, I was responding specifically to this sentence by @imiric 😉

it will break scripts that did some manual work on []byte, and will be annoying to fix.

[mstoykov]

Also a relevant problem to some of the changes #1770 (comment)

If we are taking []byte and the user provides us with a string depending on whether it is asciiString or unicodeString we are then doing crypto on either utf8 or utf16 strings ... which as you can guess have totally different outputs.

So we might need to change this to always go to utf8 for now and add encoding argument, which will be more in line with what nodejs does

[mstoykov]

I am of the opinion that we should drop the changes where we add a way to return ArrayBuffer And keep the rest where an ArrayBuffer is now an appropriate argument for stuff.

This way we can see and test better if we can completely drop returning []byte for returning ArrayBuffer instead in it's place. This IMO will require some writing of scripts, possibly going through issues connected with it and trying them.

I myself went down the rabbit whole of what encoding is a string if we take it as []byte vs as string in argument which ate too much time.

So as I am of the opinion that taking ArrayBuffer is more important and "probably" backwards compatible it should be done first, and then we can figure out what we do next from there

[imiric]

@mstoykov Makes sense, I'll revert some of the changes then.

[imiric]

Reverting got messy, so I reworked the commits and added some tests. Let me know.

BTW, if we support passing ArrayBuffer to b64encode, then we should support getting ArrayBuffer from b64decode as well.

The current string return value is unfortunate, but if we don't want to break compatibility, then we should add a new encoding option that does it, similarly to how we handled the open() return before I reverted it.

[mstoykov]

LGTM, I would want to play with it more, but given #1770 (comment) my main concern is gone so I am fine with this getting merged and fixing it more later

[imiric]

@mstoykov @na-- What do you think about changing b64decode (#1800 (comment))? Right now you can b64encode an ArrayBuffer, but you can't get it back from b64decode, which seems rather limiting. Same goes for []byte, though I guess if no one has asked for it, the current API is fine?

[mstoykov]

I think we should have an issue where we discuss those breaking changes.

I am of the opinion it is the correct thing for them to return ArrayBuffer, and breaking it will possibly help users more than not. But maybe we should see some user scripts that use it and whether at least some of them will keep working even if we change them. I haven't come across many, as the ones that have been reported don't work for this exact reason so 🤷‍♂️

Do you want to try and make an issue for this with some code showing usage that will:

1. keep working
2. break because we now don't return a string
3. will now work(this should be the easiest part as there is code that isn't working currently exactly because of this ;) )

[na--]

LGTM