Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
43 changes: 18 additions & 25 deletions src/js/node/worker_threads.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,6 @@
return event.data;
}

function errorEventHandler(event: ErrorEvent) {
return event.error;
}

function customEventHandler(event) {
return event.detail;
}
Expand All @@ -134,14 +130,16 @@
};
}

// These extractors recover the raw value for .on() listeners from whatever
// event shape dispatchEvent delivered. They must match both the native
// MessagePort events (message/messageerror are MessageEvent carrying .data)
// and the events emit() constructs below. Everything else (including "error",
// which MessagePort never fires natively) rides CustomEvent.detail, which is
// what node's NodeEventTarget[kCreateEvent] produces for non-message types.
Comment thread
robobun marked this conversation as resolved.
Outdated
function functionForEventType(event, listener) {
switch (event) {
case "error":
case "message":
case "messageerror": {
return wrapped(errorEventHandler, listener);
}

case "message": {
return wrapped(messageEventHandler, listener);
}

Expand All @@ -151,14 +149,6 @@
}
}

function EventClass(eventName) {
if (eventName === "error" || eventName === "messageerror") {
return ErrorEvent;
}

return MessageEvent;
}

// EventTarget dedupes on (type, callback), so in node the FIRST registration of
// a listener wins outright -- including its once-ness -- and later adds of the
// same function are no-ops. Keying wrappers per listener reproduces that.
Expand Down Expand Up @@ -203,20 +193,23 @@
return this;
}

function emit(event, ...args) {
// node's NodeEventTarget.prototype.emit(type, arg): carry a single raw arg,
// hand it as-is to node-style listeners, lazily wrap it as a typed event for
// addEventListener listeners, and return whether any listeners were
// registered. The extractors in functionForEventType undo the wrapping for
// .on() listeners, so the init dict here must round-trip through them.
function emit(event, arg) {
const hadListeners = listenerCount.$call(this, event) > 0;

Check warning on line 202 in src/js/node/worker_threads.ts

View check run for this annotation

Claude / Claude Code Review

emit() returns false when only addEventListener listeners are registered

`emit()` now returns `false` when the only listeners were registered via `addEventListener()` (not `.on()`), whereas Node returns `true` — `listenerCount()` here only reads the JS-side `.on()`/`.once()` registry, not the native EventTarget's listener map that `addEventListener` writes to. The underlying `listenerCount()` gap is pre-existing (it already returned 0 for such listeners), so a proper fix belongs there rather than in this PR; noting it because the new return value now exposes it where
Comment thread
robobun marked this conversation as resolved.
switch (event) {
case "error":
case "messageerror":
case "message":
this.dispatchEvent(new (EventClass(event))(event, ...args));
case "messageerror":
this.dispatchEvent(new MessageEvent(event, { data: arg }));
break;
default:
// Non-standard events surface as CustomEvent (detail = first arg) to
// addEventListener and as the raw argument to .on(), matching node.
this.dispatchEvent(new CustomEvent(event, { detail: args[0] }));
this.dispatchEvent(new CustomEvent(event, { detail: arg }));
break;
}
return this;
return hadListeners;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
}

const kMaxListeners = Symbol("kMaxListeners");
Expand Down
86 changes: 86 additions & 0 deletions test/js/node/worker_threads/worker_threads.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1312,6 +1312,92 @@ test("MessagePort NodeEventTarget methods", () => {
port1.close();
});

// emit() previously did `new MessageEvent("message", payload)` / `new
// ErrorEvent("error", err)`, so the payload was (mis)read as an init dict and
// .on() listeners received null/undefined. It also returned `this`. Verified
// against node: .on() sees the raw arg by identity; addEventListener sees a
// MessageEvent (data = arg) for message/messageerror and a CustomEvent
// (detail = arg) otherwise; emit() returns a boolean.
test("MessagePort emit() delivers the raw payload and returns a boolean", () => {
// message: .on() gets the payload by identity, addEventListener gets a
// MessageEvent whose .data is the payload; emit() -> true.
{
const { port1: p } = new MessageChannel();
const payload = { id: 42 };
let onArg: unknown = "unset";
let aelData: unknown = "unset";
let aelCtor = "";
p.on("message", x => (onArg = x));
p.addEventListener("message", e => ((aelData = e.data), (aelCtor = e.constructor.name)));
const ret = p.emit("message", payload);
expect({ onArg, aelData, aelCtor, ret }).toEqual({
onArg: payload,
aelData: payload,
aelCtor: "MessageEvent",
ret: true,
});
expect(onArg).toBe(payload);
expect(aelData).toBe(payload);
p.close();
}

// error: .on() gets the Error by identity; addEventListener gets a
// CustomEvent whose .detail is the Error (node's NodeEventTarget default).
{
const { port1: p } = new MessageChannel();
const err = new Error("boom");
let onArg: unknown = "unset";
let aelDetail: unknown = "unset";
let aelCtor = "";
p.on("error", x => (onArg = x));
p.addEventListener("error", (e: any) => ((aelDetail = e.detail), (aelCtor = e.constructor.name)));
const ret = p.emit("error", err);
expect({ onArg, aelDetail, aelCtor, ret }).toEqual({
onArg: err,
aelDetail: err,
aelCtor: "CustomEvent",
ret: true,
});
expect(onArg).toBe(err);
p.close();
}

// messageerror: same shape as message (MessageEvent, .data = arg).
{
const { port1: p } = new MessageChannel();
const err = new Error("boom");
let onArg: unknown = "unset";
let aelData: unknown = "unset";
p.on("messageerror", x => (onArg = x));
p.addEventListener("messageerror", (e: any) => (aelData = e.data));
p.emit("messageerror", err);
expect(onArg).toBe(err);
expect(aelData).toBe(err);
p.close();
}

// No listeners: emit() -> false (not `this`).
{
const { port1: p } = new MessageChannel();
expect(p.emit("message", 1)).toBe(false);
expect(p.emit("custom", 1)).toBe(false);
p.close();
}

// Custom events (unchanged behaviour, guards against regression): .on() gets
// the arg, addEventListener gets CustomEvent with .detail = arg.
{
const { port1: p } = new MessageChannel();
let onArg: unknown = "unset";
let aelDetail: unknown = "unset";
p.on("foo", (x: unknown) => (onArg = x));
p.addEventListener("foo", (e: any) => (aelDetail = e.detail));
p.emit("foo", 123);
expect({ onArg, aelDetail }).toEqual({ onArg: 123, aelDetail: 123 });
p.close();
}
});

// jsRef() only gated on m_isDetached, so .ref()/onmessage= after the peer closed
// re-took an event-loop ref that nothing releases and the process hung. Node no-ops
// both. Spawned, because the symptom is "the process never exits".
Expand Down
Loading