Skip to content

Using MessageChannel pulls in undici inappropriately #57581

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

Open
webstrand opened this issue Mar 21, 2025 · 7 comments
Open

Using MessageChannel pulls in undici inappropriately #57581

webstrand opened this issue Mar 21, 2025 · 7 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@webstrand
Copy link

Version

v22.13.1

Platform

Linux localhost 6.13.1-1-default #1 SMP PREEMPT_DYNAMIC Mon Feb  3 05:33:25 UTC 2025 (1918d13) x86_64 x86_64 x86_64 GNU/Linux

Subsystem

worker_threads

What steps will reproduce the bug?

Run this command:

node --jitless -e 'const { port1, port2 } = new MessageChannel(); port1.addEventListener("message", () => {});  port2.postMessage(null);'

or run this script with --jitless

const { port1, port2 } = new MessageChannel();
port1.addEventListener("message", () => {});
port2.postMessage(null);

How often does it reproduce? Is there a required condition?

Every time

What is the expected behavior? Why is that the expected behavior?

No exception should be thrown, undici should not be loaded when fetch is not needed.

What do you see instead?

> node --jitless -e 'const { port1, port2 } = new MessageChannel(); port1.addEventListener("message", () => {});  port2.postMessage(null);'
Warning: disabling flag --expose_wasm due to conflicting flags
node:internal/deps/undici/undici:5827
        mod = await WebAssembly.compile(llhttpWasmData || require_llhttp_wasm());
        ^

ReferenceError: WebAssembly is not defined
    at lazyllhttp (node:internal/deps/undici/undici:5827:9)
    at lib/dispatcher/client-h1.js (node:internal/deps/undici/undici:5873:25)
    at __require (node:internal/deps/undici/undici:6:50)
    at lib/dispatcher/client.js (node:internal/deps/undici/undici:7607:21)
    at __require (node:internal/deps/undici/undici:6:50)
    at lib/dispatcher/pool.js (node:internal/deps/undici/undici:8068:18)
    at __require (node:internal/deps/undici/undici:6:50)
    at lib/dispatcher/agent.js (node:internal/deps/undici/undici:8151:16)
    at __require (node:internal/deps/undici/undici:6:50)
    at lib/global.js (node:internal/deps/undici/undici:8251:17)

Node.js v22.13.1

Additional information

I believe this is due to constructing the MessageEvent, which appears to come from undici:

fastCreateMessageEvent ??= require('internal/deps/undici/undici').createFastMessageEvent;

@marco-ippolito
Copy link
Member

cc @nodejs/undici I think also typescript has the same issue since amaro lazy loads a wasm

@mcollina
Copy link
Member

MessageEvent is part of the HTML standard and it's at basis of WebSocket.

https://html.spec.whatwg.org/multipage/comms.html

That's why the global comes from undici. What I'm a bit puzzled about is why for non-spec compliant objects we are using it in

// createFastMessageEvent skips webidl argument validation when the arguments
.

This appears to come from #52370.

Wdyt @KhafraDev?

@KhafraDev
Copy link
Member

What I'm a bit puzzled about is why for non-spec compliant objects we are using it in

It was exposed globally where the real issue arose from, when <message event from websocket> instanceof MessageEvent would be false. It was not a good implementation, and since undici already had a fully spec-compliant, faster implementation, it was easier to replace node's with undici's.

@mcollina
Copy link
Member

The only place where MessageEvent is mentioned in our docs is https://nodejs.org/api/worker_threads.html#broadcastchannelonmessage.

I'm a bit puzzled why we use it here to begin with. @jasnell @addaleax do you recall?

@jasnell
Copy link
Member

jasnell commented Mar 25, 2025

Puzzled about using it where exactly?

@mcollina
Copy link
Member

About why our MessagePort and MessageChannel are using the spec-compliant MessageEvent. Do you recall what the design motivation was?

I think we can add a fallback if there is no fetch/undici for MessageEvent.

@mcollina mcollina added the feature request Issues that request new features to be added to Node.js. label Mar 25, 2025
@github-project-automation github-project-automation bot moved this to Awaiting Triage in Node.js feature requests Mar 25, 2025
@jasnell
Copy link
Member

jasnell commented Mar 25, 2025

well, MessagePort would have had a separate MessageEvent originally since that predates the introduction of undici and WebSockets but I guess it got replaced at some point with the undici one. The code in worker/io.js was updated about 11 months ago was updated (#52370) to switch to using undici's MessageEvent rather than the original implementation that still exists in the runtime in lib/internal/per_context/messageport.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
Status: Awaiting Triage
Development

No branches or pull requests

5 participants