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

Better support for binary data #1020

Closed
na-- opened this issue May 15, 2019 · 18 comments · Fixed by #1841
Closed

Better support for binary data #1020

na-- opened this issue May 15, 2019 · 18 comments · Fixed by #1841
Assignees
Labels
enhancement evaluation needed proposal needs to be validated or tested before fully implementing it in k6 high prio js-compat ux
Milestone

Comments

@na--
Copy link
Member

na-- commented May 15, 2019

We currently don't have a good way to deal with binary data on the JS side yet 😞. We rely on []byte from the Go code, which translates to int arrays in JS, which is far from optimal. These are the places where we currently have binary data that I can think of:

We need a native analogue of the node.js Buffer, or better yet - to properly support ES ArrayBuffer objects. We actually have ArrayBuffer, the code below compiles and runs as you'd expect:

export default function () {
    var buffer = new ArrayBuffer(16);

    // Create a couple of views
    var view1 = new DataView(buffer);
    var view2 = new DataView(buffer, 12, 4); //from byte 12 for the next 4 bytes
    view1.setInt8(12, 42); // put 42 in slot 12

    console.log(view2.getInt8(0));

    var z = new Int32Array(buffer, 0, 4);
    console.log(z);
};

but I'm not sure if this ArrayBuffer comes from the bundled core.js or from goja. It seems like the ArrayBuffer is from goja (though I wouldn't rely on it very much), but the DataView and Int32Array seem to be from core.js 😕

So we need to investigate exactly what the situation is and what we can use to handle binary data sensibly...

Related issues: #856, #874, #873

@na-- na-- added enhancement ux js-compat evaluation needed proposal needs to be validated or tested before fully implementing it in k6 labels May 15, 2019
@na-- na-- changed the title Better support for binary objects Better support for binary data May 16, 2019
@na-- na-- mentioned this issue May 16, 2019
@na-- na-- added this to the v1.0.0 milestone Aug 27, 2019
@na--
Copy link
Member Author

na-- commented Oct 24, 2019

A user found out that the current websocket API lacks support for binary messages: https://community.k6.io/t/converting-audio-file-into-bytes-and-send-over-web-socket/272/

But in order to fix the websocket code, it would be best if we first standardized how we support binary data in general, i.e. this issue... So, because of that and all of the other dependent issues, I'm upping the priority of this issue.

@na-- na-- added the high prio label Oct 24, 2019
@na-- na-- modified the milestones: v1.0.0, v0.27.0 Oct 24, 2019
@imiric imiric self-assigned this Nov 11, 2019
@imiric
Copy link
Contributor

imiric commented Nov 13, 2019

I took a brief look at this, and the current ArrayBuffer implementation comes from core.js, as do DataView and all typed arrays. The Goja implementation is actually disabled, and doesn't seem to have ever been used. (You can now confirm this easily with --compatibility-mode base. ;)

As suggested in #420 and dop251/goja#51, the way forward seems to be having native support in Goja, which would mean resurrecting the currently unused ArrayBuffer implementation, and ensuring it works transparently with core.js polyfills.

It doesn't seem like a gargantuan amount of work, but I'm a bit out of my depths here, as Goja internals are quite complex. I can give this a shot if you agree with the approach, let me know.

@caseylucas
Copy link

I found this issue after needing to post some binary data. I ended up making a change to k6 that would allow me to use Uint8Array in the js code but it's a little hacky. Just wanted to share here in case anyone else needs it or it sparks any other ideas. Basically, I just look at the body parameter and if it quacks like an Uint8Array, I call the goja Get func to get all the bytes and put them in a byte[].

diff --git a/js/modules/k6/http/request.go b/js/modules/k6/http/request.go
index d448c3a0..07fec21f 100644
--- a/js/modules/k6/http/request.go
+++ b/js/modules/k6/http/request.go
@@ -29,6 +29,7 @@ import (
        "net/textproto"
        "net/url"
        "reflect"
+       "strconv"
        "strings"
        "sync"
        "time"
@@ -97,7 +98,24 @@ func (h *HTTP) Request(ctx context.Context, method string, url goja.Value, args
        var params goja.Value

        if len(args) > 0 {
-               body = args[0].Export()
+               // check to see if body param looks like a Uint8Array and if so, copy bytes out of it into a []byte.
+               if obj, ok := args[0].(*goja.Object); ok {
+                       bytesPerElementVal := obj.Get("BYTES_PER_ELEMENT")
+                       byteLengthVal := obj.Get("byteLength")
+                       if bytesPerElementVal != nil &&
+                               bytesPerElementVal.ToInteger() == 1 &&
+                               byteLengthVal != nil {
+                               byteLength := byteLengthVal.ToInteger()
+                               byteBuf := make([]byte, byteLength)
+                               for i := int64(0); i < byteLength; i++ {
+                                       byteBuf[i] = (byte)(obj.Get(strconv.FormatInt(i, 10)).ToInteger() & 0xff)
+                               }
+                               body = byteBuf
+                       }
+               }
+               if body == nil {
+                       body = args[0].Export()
+               }
        }
        if len(args) > 1 {
                params = args[1]

With this change, you can use like:

      const binaryBuf = new Uint8Array(4);
      binaryBuf[0] = 0;
      binaryBuf[1] = 1;
      binaryBuf[2] = 2;
      binaryBuf[3] = 3;
      http.post(
        someURL,
        binaryBuf,
      );

@caseylucas
Copy link

@imiric Did you have any luck with the goja ArrayBuffer implementation? Is this still the preferred strategy vs something like I did above? Need any help?

@imiric
Copy link
Contributor

imiric commented Dec 19, 2019

@caseylucas No, I didn't make much progress with exposing the native ArrayBuffer. Our top priority right now is getting #1007 merged, so while this issue is high on the priority list, it's unlikely to be worked on for a few weeks at least.

Your approach looks interesting, though I'm curious about the performance, and it should probably be done outside of HTTP.Request, as k6 would benefit from a more generic and lower-level implementation. Pull requests are always welcome, so if you think this approach could work for the other issues mentioned here (open(), crypto, etc.), feel free to contribute. :)

@sirianni
Copy link

sirianni commented May 9, 2020

Is there a workaround that can be used currently, even if it is inefficient? I am using protobufjs which encodes protobuf messages as a Uint8Array. When I try to pass this as a request body I get this error. It seems that the golang http code has support for byte[], but I can't figure out how to pass that from the javascript side.

ERRO[0000] GoError: unknown request body type []interface {}
	at native

@na--
Copy link
Member Author

na-- commented Sep 2, 2020

Prompted by this forum topic, it might not be a bad idea if we also support "generator functions" when we have binary data. So, instead of http.post(url, someBigChunkOfData), users should be able to use http.post(url, someJSFunctionThatGeneratesDataOnDemand)...

imiric pushed a commit that referenced this issue Mar 29, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Mar 29, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Mar 29, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Mar 29, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Mar 29, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Mar 29, 2021
The previous default behavior of returning string should now be
specified as an additional "s" argument.

This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Mar 31, 2021
I experimented with overriding the first argument passed to the
'message' handler to be a String object with a `buffer` data property,
but it doesn't expose the string primitive properties (.length, etc.) so
it would be a breaking change.

Passing the ArrayBuffer as the second argument is backwards compatible,
and hopefully doesn't cause a doubling of memory usage (I'm expecting it
to be garbage collected if the argument isn't used). We could detect the
number of arguments defined on the handler and only pass the ArrayBuffer
if expected, but this would be awkward to implement.

Part of #1020
imiric pushed a commit that referenced this issue Apr 6, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 6, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 6, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 6, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 6, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 6, 2021
The previous default behavior of returning string should now be
specified as an additional "s" argument.

This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 6, 2021
I experimented with overriding the first argument passed to the
'message' handler to be a String object with a `buffer` data property,
but it doesn't expose the string primitive properties (.length, etc.) so
it would be a breaking change.

Passing the ArrayBuffer as the second argument is backwards compatible,
and hopefully doesn't cause a doubling of memory usage (I'm expecting it
to be garbage collected if the argument isn't used). We could detect the
number of arguments defined on the handler and only pass the ArrayBuffer
if expected, but this would be awkward to implement.

Part of #1020
imiric pushed a commit that referenced this issue Apr 7, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 7, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 7, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 7, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 7, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 7, 2021
The previous default behavior of returning string should now be
specified as an additional "s" argument.

This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 7, 2021
I experimented with overriding the first argument passed to the
'message' handler to be a String object with a `buffer` data property,
but it doesn't expose the string primitive properties (.length, etc.) so
it would be a breaking change.

Passing the ArrayBuffer as the second argument is backwards compatible,
and hopefully doesn't cause a doubling of memory usage (I'm expecting it
to be garbage collected if the argument isn't used). We could detect the
number of arguments defined on the handler and only pass the ArrayBuffer
if expected, but this would be awkward to implement.

Part of #1020
imiric pushed a commit that referenced this issue Apr 28, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 28, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 28, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 28, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 28, 2021
This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 28, 2021
The previous default behavior of returning string should now be
specified as an additional "s" argument.

This is a breaking change part of #1020.
imiric pushed a commit that referenced this issue Apr 28, 2021
I experimented with overriding the first argument passed to the
'message' handler to be a String object with a `buffer` data property,
but it doesn't expose the string primitive properties (.length, etc.) so
it would be a breaking change.

Passing the ArrayBuffer as the second argument is backwards compatible,
and hopefully doesn't cause a doubling of memory usage (I'm expecting it
to be garbage collected if the argument isn't used). We could detect the
number of arguments defined on the handler and only pass the ArrayBuffer
if expected, but this would be awkward to implement.

Part of #1020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement evaluation needed proposal needs to be validated or tested before fully implementing it in k6 high prio js-compat ux
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants