Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 54 additions & 8 deletions internal/js/modules/k6/websockets/listeners.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ type eventListener struct {
// this return sobek.value *and* error in order to return error on exception instead of panic
// https://pkg.go.dev/github.com/dop251/goja#hdr-Functions
on func(sobek.Value) (sobek.Value, error)
list []func(sobek.Value) (sobek.Value, error)
list []listenerEntry
}

// listenerEntry represents a single listener entry in the list of listeners
type listenerEntry struct {
val sobek.Value
fn func(sobek.Value) (sobek.Value, error)
}

// newListener creates a new listener of a certain type
Expand All @@ -48,8 +54,24 @@ func newListener(eventType string) *eventListener {
}

// add adds a listener to the listener list
func (l *eventListener) add(fn func(sobek.Value) (sobek.Value, error)) {
l.list = append(l.list, fn)
func (l *eventListener) add(entry listenerEntry) {
l.list = append(l.list, entry)
}

// remove removes all listeners matching the provided JavaScript value
func (l *eventListener) remove(target sobek.Value) {
if len(l.list) == 0 {
return
}

newList := make([]listenerEntry, 0, len(l.list))
for _, entry := range l.list {
if !entry.val.SameAs(target) {
newList = append(newList, entry)
}
}

l.list = newList
}

// setOn sets a listener for the on* properties, like onopen, onmessage, etc.
Expand All @@ -64,11 +86,22 @@ func (l *eventListener) getOn() func(sobek.Value) (sobek.Value, error) {

// return all possible listeners for a certain event type
func (l *eventListener) all() []func(sobek.Value) (sobek.Value, error) {
if l.on == nil {
return l.list
size := len(l.list)
if l.on != nil {
size++
}

fns := make([]func(sobek.Value) (sobek.Value, error), 0, size)

if l.on != nil {
fns = append(fns, l.on)
}

return append([]func(sobek.Value) (sobek.Value, error){l.on}, l.list...)
for _, entry := range l.list {
fns = append(fns, entry.fn)
}

return fns
}

// getTypes return event listener of a certain type
Expand All @@ -92,14 +125,27 @@ func (l *eventListeners) getType(t string) *eventListener {
}

// add adds a listener to the listeners
func (l *eventListeners) add(t string, f func(sobek.Value) (sobek.Value, error)) error {
func (l *eventListeners) add(t string, entry listenerEntry) error {
list := l.getType(t)

if list == nil {
return fmt.Errorf("unknown event type: %s", t)
}

list.add(entry)

return nil
}

// remove removes a listener from the listeners
func (l *eventListeners) remove(t string, target sobek.Value) error {
list := l.getType(t)

if list == nil {
return fmt.Errorf("unknown event type: %s", t)
}

list.add(f)
list.remove(target)

return nil
}
Expand Down
37 changes: 33 additions & 4 deletions internal/js/modules/k6/websockets/websockets.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,8 @@ const (
func defineWebsocket(rt *sobek.Runtime, w *webSocket) {
must(rt, w.obj.DefineDataProperty(
"addEventListener", rt.ToValue(w.addEventListener), sobek.FLAG_FALSE, sobek.FLAG_FALSE, sobek.FLAG_TRUE))
must(rt, w.obj.DefineDataProperty(
"removeEventListener", rt.ToValue(w.removeEventListener), sobek.FLAG_FALSE, sobek.FLAG_FALSE, sobek.FLAG_TRUE))
must(rt, w.obj.DefineDataProperty(
"send", rt.ToValue(w.send), sobek.FLAG_FALSE, sobek.FLAG_FALSE, sobek.FLAG_TRUE))
must(rt, w.obj.DefineDataProperty(
Expand Down Expand Up @@ -919,16 +921,43 @@ func (w *webSocket) callEventListeners(eventType string) error {
return nil
}

func (w *webSocket) addEventListener(event string, handler func(sobek.Value) (sobek.Value, error)) {
func (w *webSocket) addEventListener(event string, handler sobek.Value) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:
The spec actually says to dedup here:

If eventTarget’s event listener list does not contain an event listener whose type is listener’s type, callback is listener’s callback, and capture is listener’s capture, then append listener to eventTarget’s event listener list.

Now that you introduced comparability, could you also add the dedup? This would also get rid of this somewhat strange behavior that one remove() call could wipe multiple listeners at once.

@ankur22 wdyt? IMO we could do this in this PR but on paper it would be a breaking change and somewhat out of scope.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janHildebrandt98 True true. I'll definitely add those test cases and I'd be happy to also work on dedup here itself (or under a new issue for it) if @ankur22 is happy with that.

// TODO support options https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#parameters

if handler == nil {
if common.IsNullish(handler) {
common.Throw(w.vu.Runtime(), fmt.Errorf("handler for event type %q isn't a callable function", event))
}

fnCallable, isFunc := sobek.AssertFunction(handler)
if !isFunc {
common.Throw(w.vu.Runtime(), fmt.Errorf("handler for event type %q isn't a callable function", event))
}

if err := w.eventListeners.add(event, handler); err != nil {
execFn := func(v sobek.Value) (sobek.Value, error) {
return fnCallable(sobek.Undefined(), v)
}

entry := listenerEntry{
val: handler,
fn: execFn,
}

if err := w.eventListeners.add(event, entry); err != nil {
w.vu.State().Logger.Warnf("can't add event handler: %s", err)
}
}

// TODO add remove listeners
func (w *webSocket) removeEventListener(event string, handler sobek.Value) {
if common.IsNullish(handler) {
return
}

_, isFunc := sobek.AssertFunction(handler)
if !isFunc {
return
}

if err := w.eventListeners.remove(event, handler); err != nil {
w.vu.State().Logger.Warnf("can't remove event handler: %s", err)
}
}
36 changes: 36 additions & 0 deletions internal/js/modules/k6/websockets/websockets_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1758,3 +1758,39 @@ func TestPingHandlerDeadlock(t *testing.T) {
`))
assert.NoError(t, err)
}

func TestRemoveEventListener(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add cases for removeEventListener with a non-function/null handler, and for removing a listener that was registered twice via addEventListener? Both are silent no-ops right now and neither is covered here.

t.Parallel()
ts := newTestState(t)
sr := ts.tb.Replacer.Replace
_, err := ts.runtime.RunOnEventLoop(sr(`
var ws = new WebSocket("WSBIN_URL/ws-echo")
var handlerToKeepCount = 0

function handlerToRemove() {
call("removed-handler-called")
}
function handlerToKeep() {
handlerToKeepCount++
call("kept-handler-count:" + handlerToKeepCount)
ws.close()
}

ws.addEventListener("open", () => {
ws.addEventListener("pong", handlerToRemove)
ws.addEventListener("pong", handlerToKeep)
ws.removeEventListener("pong", handlerToRemove)

ws.removeEventListener("message", handlerToRemove)
ws.removeEventListener("error", handlerToRemove)

ws.ping()
})
`))
require.NoError(t, err)

recorded := ts.callRecorder.Recorded()
assert.NotContains(t, recorded, "removed-handler-called")
assert.Contains(t, recorded, "kept-handler-count:1")
assert.NotContains(t, recorded, "kept-handler-count:2")
}