More ArrayBuffer support #1841

imiric · 2021-02-04T14:34:28Z

These are the leftover changes to close #1020. Most of them break backwards compatibility, as discussed in the issue.

It might be easier to review by commit.

codecov-io · 2021-02-04T14:45:15Z

Codecov Report

Merging #1841 (d60b986) into master (e5d4d82) will increase coverage by 0.06%.
The diff coverage is 89.90%.

❗ Current head d60b986 differs from pull request most recent head 23e3683. Consider uploading reports for the commit 23e3683 to get more accurate results

@@            Coverage Diff             @@
##           master    #1841      +/-   ##
==========================================
+ Coverage   71.28%   71.34%   +0.06%     
==========================================
  Files         183      181       -2     
  Lines       14274    14293      +19     
==========================================
+ Hits        10175    10198      +23     
+ Misses       3479     3474       -5     
- Partials      620      621       +1

Flag	Coverage Δ
ubuntu	`71.32% <89.90%> (+0.10%)`	⬆️
windows	`70.94% <89.90%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
lib/netext/httpext/compression.go	`86.45% <ø> (ø)`
lib/netext/httpext/response.go	`100.00% <ø> (+8.88%)`	⬆️
js/common/util.go	`68.75% <77.77%> (+3.53%)`	⬆️
js/modules/k6/http/response.go	`78.57% <86.36%> (+4.34%)`	⬆️
js/modules/k6/encoding/encoding.go	`89.74% <88.88%> (+2.24%)`	⬆️
js/modules/k6/ws/ws.go	`83.15% <93.93%> (+1.58%)`	⬆️
js/initcontext.go	`92.39% <100.00%> (+0.08%)`	⬆️
js/modules/k6/crypto/crypto.go	`94.16% <100.00%> (+0.20%)`	⬆️
js/modules/k6/http/request.go	`80.20% <100.00%> (+0.13%)`	⬆️
lib/netext/httpext/batch.go	`100.00% <100.00%> (ø)`
... and 19 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 e5d4d82...23e3683. Read the comment docs.

mstoykov

Seems good to me - I will need to try using it probably.

Can you drop the support for []byte everywhere as well so that we can see if something breaks when we no longer support it.

lib/netext/httpext/response.go

Kind of resolves #1841 (comment)

na-- · 2021-03-04T18:45:28Z

Set the milestone to v0.32.0. Also, a reminder that we discussed websocket binary support, which might be a part of this PR or another similar one.

imiric · 2021-03-29T15:23:13Z

OK, so I reverted the WIP changes discussed here, and pushed the agreed breaking changes for v0.32.0.

Some notes:

I moved Response.JSON() to the JS k6/http module, where it arguably should've been to begin with, so also supporting ArrayBuffer bodies does not introduce a dependency on httpext anymore, and we can use common.ToBytes() to convert all supported types. So there are no breaking changes for Response.HTML() or Response.JSON() anymore.
b64decode() now returns ArrayBuffer by default, and the previous default string is returned when specifying an additional "s" (format) argument.
I didn't drop support for []byte everywhere (ToBytes() should be the only place now?), mainly since we might want to support that internally, or well, just in case... :)
ArrayBuffer support in k6/ws is still pending, and I intend to push it in this PR as well, unless you think it's large enough already, let me know.

js/initcontext.go

js/modules/k6/http/response.go

Return goja.Value from open() Resolves #1841 (comment)

Resolves #1841 (comment)

imiric · 2021-03-31T10:57:21Z

js/modules/k6/ws/ws.go

@@ -292,7 +292,8 @@ func (*WS) Connect(ctx context.Context, url string, args ...goja.Value) (*WSHTTP
 				Tags:   socket.sampleTags,
 				Value:  1,
 			})
-			socket.handleEvent("message", rt.ToValue(string(readData)))
+			ab := rt.NewArrayBuffer(readData)
+			socket.handleEvent("message", rt.ToValue(string(readData)), rt.ToValue(&ab))


Would this be prohibitively expensive memory wise?

I experimented with overriding the first argument to be a String object with an additional buffer property, but that doesn't expose the string primitive properties (.length, etc.) so it would be a breaking change.

Passing it as a second argument is backwards compatible, and I'm expecting it to be garbage collected if it's not used, but haven't run any benchmarks. One optimization we could do is to only pass it if the message handler function expects a second argument, but it would be awkward to do and would require additional refactoring.

I think it will be better to just have binaryMessage event or something like that instead of ... this ;)

This is not how it's done in other tools but given the current way it is done in k6 I think this is better and then we can fix it properly in the next implementation when it is more async

Sure, see e44fb6d.

But I also think this is a rough bandaid that adds more work for users once we move to a better implementation and deprecate it.

mstoykov · 2021-03-31T14:49:33Z

js/modules/k6/ws/ws.go

+func (s *Socket) Send(message interface{}) {
 	rt := common.GetRuntime(s.ctx)

-	writeData := []byte(message)
-	if err := s.conn.WriteMessage(websocket.TextMessage, writeData); err != nil {
+	var (
+		msgType int
+		msg     []byte
+	)
+	switch m := message.(type) {
+	case string:
+		msgType = websocket.TextMessage
+		msg = []byte(m)
+	case goja.ArrayBuffer:
+		msgType = websocket.BinaryMessage
+		msg = m.Bytes()
+	default:
+		err := fmt.Errorf("unsupported message type: %T, expected string or ArrayBuffer", message)
+		common.Throw(common.GetRuntime(s.ctx), err)
+	}
+
+	if err := s.conn.WriteMessage(msgType, msg); err != nil {


I think this should be done with a separate method as it was done in the original forum thread this was discussed

This IMO is a cleaner as a solution and also will mean that if a user runs a script using, they will get an error instead of it silently being converted to a string.

I disagree that it's cleaner, but here you go: b80813c.

Expanding the API to now include another method that does the same thing for a different data type is hardly a good idea. It might've worked fine for that user and it's an isolated solution, but it's another method we'll have to eventually deprecate.

@na-- WDYT?

mstoykov · 2021-03-31T16:14:36Z

js/modules/k6/ws/ws.go

@@ -292,7 +292,8 @@ func (*WS) Connect(ctx context.Context, url string, args ...goja.Value) (*WSHTTP
 				Tags:   socket.sampleTags,
 				Value:  1,
 			})
-			socket.handleEvent("message", rt.ToValue(string(readData)))
+			ab := rt.NewArrayBuffer(readData)
+			socket.handleEvent("message", rt.ToValue(string(readData)), rt.ToValue(&ab))


I think it will be better to just have binaryMessage event or something like that instead of ... this ;)

This is not how it's done in other tools but given the current way it is done in k6 I think this is better and then we can fix it properly in the next implementation when it is more async

Resolves #1841 (comment)

mstoykov · 2021-04-01T15:39:02Z

js/modules/k6/ws/ws.go

+			if _, ok := socket.eventHandlers["binaryMessage"]; ok {
+				ab := rt.NewArrayBuffer(readData)
+				socket.handleEvent("binaryMessage", rt.ToValue(&ab))
+			}


No matter whether it is 1 or 2 events I do think that we need to check the messagetype and call either the correct method or if we choose to have only message events change the type of the value to ArrayBuffer or string based on it.

This will require more work in readPump which I think should probably be better handled in a separate PR , possibly even in a following release so ... I am mostly for dropping the ws changes for now

WDYT @na-- @imiric

You're right, we should only call the binaryMessage handler if the messageType == BinaryMessage, I overlooked that.

It shouldn't be much work to refactor, I'll look into it today. Don't think we should delay this because of that change.

@mstoykov Let me know if 5e29bdf is what you had in mind. I'll fix the new linter errors, and probably rebase on master.

Resolves #1841 (comment)

mstoykov · 2021-04-06T12:34:11Z

js/modules/k6/ws/ws.go

+		}
+	} else {
+		common.Throw(common.GetRuntime(s.ctx),
+			fmt.Errorf("expected ArrayBuffer as argument, received: %T", msg))


Ideally, this should be the type of message but that will require a lot more code and practically can have a case where we can get one (if it's nil/undefined 🤔 )

I did try it and

message.ToObject(common.GetRuntime(s.ctx)).ClassName()

seems to work okay and blows up with TypeError if message is null/undefined which is more or less what we want so ... maybe we can just add it ? But I would prefer if more testing is done that it won't panic the whole VU in some strange case

I'm not sure if we should bother with that. Sure, it would be good to log the JS type instead, but not everything can be converted to an object. ~~For example, socket.sendBinary(1) throws expected ArrayBuffer as argument, received: string for some reason, which is more confusing than expected ArrayBuffer as argument, received: int64~~ (Nevermind, I'm dumb and kept the %T 🤦). And we'd have to handle the null/undefined case as well.

~~I don't see any way in the Go API to call typeof so we'd have to RunString() it.~~ Do you think all this is worth it?

EDIT: Actually it's not that big of a problem, will change, thanks.

OK, should be resolved by 2e3624f. I think I was exhaustive in the error tests, but let me know if I missed anything.

Also I rebased on master and cleaned up the fixup commit, but didn't want to squash the changes for your suggestions yet, in case @na-- disagrees with it.

Resolves #1841 (comment)

This allows us to use ToBytes to process any body type without adding the js/common dependency to httpext, and it arguably should've been part of the http module to begin with.

Resolves #1841 (comment)

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

Resolves #1841 (comment)

This is a more user friendly error if the user passes anything else other than an ArrayBuffer object, otherwise it would output a panic stack trace from bridge.go.

Resolves #1841 (comment)

imiric · 2021-04-28T15:55:07Z

Force-pushed because of master rebase.

closes #2670

Resolves grafana/k6#1841 (comment)

imiric requested review from mstoykov and na-- February 4, 2021 14:34

imiric added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Feb 4, 2021

mstoykov requested changes Feb 5, 2021

View reviewed changes

lib/netext/httpext/response.go Outdated Show resolved Hide resolved

imiric pushed a commit that referenced this pull request Feb 11, 2021

Implement dynamic body property for HTTP responses

412cbd1

Kind of resolves #1841 (comment)

na-- added this to the v0.32.0 milestone Mar 4, 2021

na-- mentioned this pull request Mar 28, 2021

Shared-memory read-only files #1931

Open

imiric force-pushed the feat/1020-arraybuffer-breaking branch from 412cbd1 to 6a9ea79 Compare March 29, 2021 15:13

imiric requested a review from mstoykov March 29, 2021 15:25

mstoykov reviewed Mar 29, 2021

View reviewed changes

js/initcontext.go Outdated Show resolved Hide resolved

mstoykov reviewed Mar 29, 2021

View reviewed changes

js/modules/k6/http/response.go Outdated Show resolved Hide resolved

imiric pushed a commit that referenced this pull request Mar 31, 2021

fixup! Return ArrayBuffer from open()

46fffe7

Return goja.Value from open() Resolves #1841 (comment)

imiric pushed a commit that referenced this pull request Mar 31, 2021

Add ToString function to avoid unnecesary conversion from []byte

e84dacc

Resolves #1841 (comment)

imiric commented Mar 31, 2021

View reviewed changes

imiric requested a review from mstoykov March 31, 2021 10:59

mstoykov reviewed Mar 31, 2021

View reviewed changes

imiric pushed a commit that referenced this pull request Apr 1, 2021

Send binary WS message using a separate method

b80813c

Resolves #1841 (comment)

imiric pushed a commit that referenced this pull request Apr 1, 2021

Handle binary WS message using a separate event handler

e44fb6d

Resolves #1841 (comment)

imiric requested a review from mstoykov April 1, 2021 13:54

mstoykov reviewed Apr 1, 2021

View reviewed changes

imiric pushed a commit that referenced this pull request Apr 6, 2021

Call WS message handlers depending on the message type

5e29bdf

Resolves #1841 (comment)

imiric requested a review from mstoykov April 6, 2021 11:13

mstoykov reviewed Apr 6, 2021

View reviewed changes

mstoykov previously approved these changes Apr 6, 2021

View reviewed changes

imiric pushed a commit that referenced this pull request Apr 6, 2021

Add ToString function to avoid unnecesary conversion from []byte

a5c7378

Resolves #1841 (comment)

imiric pushed a commit that referenced this pull request Apr 6, 2021

Send binary WS message using a separate method

828e9cd

Resolves #1841 (comment)

imiric pushed a commit that referenced this pull request Apr 6, 2021

Handle binary WS message using a separate event handler

c9bf4cf

Resolves #1841 (comment)

Ivan Mirić added 9 commits April 28, 2021 17:54

Move JSON response body processing to JS http module

23af28f

This allows us to use ToBytes to process any body type without adding the js/common dependency to httpext, and it arguably should've been part of the http module to begin with.

Add ToString function to avoid unnecesary conversion from []byte

c211b9e

Resolves #1841 (comment)

Send binary WS message using a separate method

3d8b343

Resolves #1841 (comment)

Handle binary WS message using a separate event handler

77f8530

Resolves #1841 (comment)

Call WS message handlers depending on the message type

95337c0

Resolves #1841 (comment)

Check WS SendBinary argument and raise error if not ArrayBuffer

777155d

This is a more user friendly error if the user passes anything else other than an ArrayBuffer object, otherwise it would output a panic stack trace from bridge.go.

Fix and silence linter issues

d3dcbcd

Error with JS type in WS SendBinary

9f623d8

Resolves #1841 (comment)

imiric dismissed stale reviews from na-- and mstoykov via 9f623d8 April 28, 2021 15:54

imiric force-pushed the feat/1020-arraybuffer-breaking branch from 23e3683 to 9f623d8 Compare April 28, 2021 15:54

imiric requested review from mstoykov and na-- April 28, 2021 15:55

na-- approved these changes Apr 29, 2021

View reviewed changes

imiric merged commit af1e032 into master Apr 29, 2021

imiric deleted the feat/1020-arraybuffer-breaking branch April 29, 2021 07:59

mstoykov mentioned this pull request Jun 29, 2021

encoding.b64decode/encode fails on binary data #1798

Closed

mstoykov mentioned this pull request Sep 8, 2021

got "incorrect header check" when invoke pako.inflate during websocket test #594

Closed

mstoykov mentioned this pull request Oct 4, 2021

Refactor http.Response to have httpext.Response embeded #946

Closed

mstoykov mentioned this pull request Jun 28, 2022

Support for websocket with protobuf binary #1799

Closed

mstoykov added a commit that referenced this pull request Sep 1, 2022

Update samples/base64.js to work after #1841

42cae7c

mstoykov added a commit that referenced this pull request Sep 1, 2022

Update samples/base64.js to work after #1841

7db9144

closes #2670

mstoykov mentioned this pull request Sep 1, 2022

base64 decoding is broken (k6/encoding package) #2670

Closed

harrytwigg pushed a commit to APITeamLimited/globe-test that referenced this pull request Jan 11, 2023

Add ToString function to avoid unnecesary conversion from []byte

03585d3

Resolves grafana/k6#1841 (comment)

harrytwigg pushed a commit to APITeamLimited/globe-test that referenced this pull request Jan 11, 2023

Send binary WS message using a separate method

253d268

Resolves grafana/k6#1841 (comment)

harrytwigg pushed a commit to APITeamLimited/globe-test that referenced this pull request Jan 11, 2023

Handle binary WS message using a separate event handler

ed06a08

Resolves grafana/k6#1841 (comment)

harrytwigg pushed a commit to APITeamLimited/globe-test that referenced this pull request Jan 11, 2023

Call WS message handlers depending on the message type

c614ab5

Resolves grafana/k6#1841 (comment)

harrytwigg pushed a commit to APITeamLimited/globe-test that referenced this pull request Jan 11, 2023

Error with JS type in WS SendBinary

4c221cc

Resolves grafana/k6#1841 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More ArrayBuffer support #1841

More ArrayBuffer support #1841

imiric commented Feb 4, 2021 •

edited

Loading

codecov-io commented Feb 4, 2021 •

edited

Loading

mstoykov left a comment

na-- commented Mar 4, 2021

imiric commented Mar 29, 2021 •

edited

Loading

imiric Mar 31, 2021

mstoykov Mar 31, 2021

imiric Apr 1, 2021

mstoykov Mar 31, 2021

imiric Apr 1, 2021

mstoykov Mar 31, 2021

mstoykov Apr 1, 2021

imiric Apr 6, 2021

imiric Apr 6, 2021

mstoykov Apr 6, 2021

imiric Apr 6, 2021 •

edited

Loading

imiric Apr 6, 2021 •

edited

Loading

imiric commented Apr 28, 2021

More ArrayBuffer support #1841

More ArrayBuffer support #1841

Conversation

imiric commented Feb 4, 2021 • edited Loading

codecov-io commented Feb 4, 2021 • edited Loading

Codecov Report

mstoykov left a comment

Choose a reason for hiding this comment

na-- commented Mar 4, 2021

imiric commented Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

imiric Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

imiric commented Apr 28, 2021

imiric commented Feb 4, 2021 •

edited

Loading

codecov-io commented Feb 4, 2021 •

edited

Loading

imiric commented Mar 29, 2021 •

edited

Loading

imiric Apr 6, 2021 •

edited

Loading

imiric Apr 6, 2021 •

edited

Loading