fix(ext/node): MessagePort works #22999

satyarohith · 2024-03-20T09:06:40Z

Work in progress.

TODO:

fix the order of events generated
add a test

Closes #22951
Closes #23001

bartlomieju · 2024-03-24T05:56:30Z

@satyarohith do you need any help with landing this PR? Would be great if we could land it before v1.42 this week.

satyarohith · 2024-03-24T06:52:05Z

@satyarohith do you need any help with landing this PR? Would be great if we could land it before v1.42 this week.

Yes. I'm trying to figure out how close event would be emitted when either of the ports get closed.

bartlomieju · 2024-03-24T06:54:48Z

@satyarohith do you need any help with landing this PR? Would be great if we could land it before v1.42 this week.

Yes. I'm trying to figure out how close event would be emitted when either of the ports get closed.

I think you can override the MessagePort::close to emit the proper event?

bartlomieju · 2024-03-24T06:57:40Z

Additionally you can emit the event in MessagePort::start method when the Interrupted error is thrown (that tells that the other side has been closed).

To do that you probably want to add a private API that allows to call a callback in either of these situations - you'd install the callback if the MessagePort is created from the Node polyfills.

This patch allows the example code to work but theoritically close() should emit "close" event on both the ports of a channel (see https://nodejs.org/api/worker_threads.html#portclose) but that's not being done here. We need to address that.

satyarohith · 2024-03-25T07:20:34Z

This PR is working but the order of events fired is not similar to node:

➜  tst git:(fix_message_port_node_compat) ✗ node a.mjs        
worker: Hello from worker on parentPort!
mainPort: Hello from worker on workerPort!
worker port closed
mainPort closed
➜  tst git:(fix_message_port_node_compat) ✗ denod run -A a.mjs
worker: Hello from worker on parentPort!
worker port closed
mainPort: Hello from worker on workerPort!
mainPort closed

And according to https://nodejs.org/api/worker_threads.html#portclose, we need to emit "close" event on both the ports of a channel when either of the port is closed which is not happening right now.

ext/node/polyfills/worker_threads.ts

ext/web/13_message_port.js

ext/node/polyfills/worker_threads.ts

ext/web/13_message_port.js

bartlomieju · 2024-04-02T02:26:11Z

tests/integration/worker_tests.rs

+itest_flaky!(node_worker_message_port {
+ args: "run --quiet --allow-read workers/node_worker_message_port.mjs",
+ output: "workers/node_worker_message_port.mjs.out",
+ exit_code: 0,
+});


So it passes sometimes?

I marked it as flaky because the order is not predictable. But it doesn't match the output of Node even in a single instance.

runtime/js/99_main.js

ext/web/internal.d.ts

bartlomieju · 2024-04-02T02:27:10Z

tests/unit_node/worker_threads_test.ts

@@ -16,7 +16,7 @@ Deno.test("[node/worker_threads] BroadcastChannel is exported", () => {
 });

 Deno.test("[node/worker_threads] MessageChannel are MessagePort are exported", () => {
- assertEquals<unknown>(workerThreads.MessageChannel, MessageChannel);


Why this change?

Because the MessageChannel exported from worker_threads is now NodeMessageChannel

bartlomieju

LGTM, let's address the ordering problem in a separate PR.

Closes #22951 Closes #23001 Co-authored-by: Divy Srivastava <[email protected]>

satyarohith added 3 commits March 25, 2024 12:44

add support for port.on

047dab0

tools/format.ts

e4479b4

patch MessagePort for node

653d268

This patch allows the example code to work but theoritically close() should emit "close" event on both the ports of a channel (see https://nodejs.org/api/worker_threads.html#portclose) but that's not being done here. We need to address that.

satyarohith force-pushed the fix_message_port_node_compat branch from c1074c3 to 653d268 Compare March 25, 2024 07:15

satyarohith marked this pull request as ready for review March 25, 2024 07:16

satyarohith changed the title ~~fix(ext/node): MessagePort works~~ [WIP] fix(ext/node): MessagePort works Mar 25, 2024

fix build

175fc4a

satyarohith changed the title ~~[WIP] fix(ext/node): MessagePort works~~ fix(ext/node): MessagePort works Mar 25, 2024

use null as termination signal

872e3b3

satyarohith commented Mar 25, 2024

View reviewed changes

ext/node/polyfills/worker_threads.ts Outdated Show resolved Hide resolved

satyarohith commented Mar 25, 2024

View reviewed changes

ext/web/13_message_port.js Outdated Show resolved Hide resolved

satyarohith added 2 commits March 25, 2024 18:35

disable assertEqual on MessageChannel and MessagePort instances

282c868

fix lint

4ac8366

bartlomieju reviewed Mar 31, 2024

View reviewed changes

ext/web/13_message_port.js Outdated Show resolved Hide resolved

ext/web/13_message_port.js Outdated Show resolved Hide resolved

ext/web/13_message_port.js Outdated Show resolved Hide resolved

ext/web/13_message_port.js Outdated Show resolved Hide resolved

satyarohith added 6 commits April 1, 2024 09:45

Merge branch 'main' into fix_message_port_node_compat

ca07b90

patch MessagePort in worker_threads

18971cd

update test

cdb32d8

add integration test

51ad805

mark properties as enumerable

c3fd281

mark test as flaky as log orders are not guaranteed

11014fd

bartlomieju reviewed Apr 2, 2024

View reviewed changes

ext/node/polyfills/worker_threads.ts Outdated Show resolved Hide resolved

bartlomieju reviewed Apr 2, 2024

View reviewed changes

ext/node/polyfills/worker_threads.ts Outdated Show resolved Hide resolved

bartlomieju reviewed Apr 2, 2024

View reviewed changes

satyarohith added 2 commits April 2, 2024 10:20

address review feedback

9449689

fix: transfer port using postMessage works

9a7d8ff

bartlomieju approved these changes Apr 2, 2024

View reviewed changes

satyarohith merged commit 4d66ec9 into main Apr 2, 2024
17 checks passed

satyarohith deleted the fix_message_port_node_compat branch April 2, 2024 11:36

bartlomieju mentioned this pull request Apr 2, 2024

Bug: MessageChannel port.on is not a function (vitest) #23185

Closed

satyarohith added a commit that referenced this pull request Apr 11, 2024

fix(ext/node): MessagePort works (#22999)

36808d8

Closes #22951 Closes #23001 Co-authored-by: Divy Srivastava <[email protected]>

marvinhagemeister mentioned this pull request Apr 12, 2024

(DENO - VITEST) : Error running: deno -A --node-modules-dir npm:vitest #19767

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ext/node): MessagePort works #22999

fix(ext/node): MessagePort works #22999

satyarohith commented Mar 20, 2024 •

edited

Loading

bartlomieju commented Mar 24, 2024

satyarohith commented Mar 24, 2024

bartlomieju commented Mar 24, 2024

bartlomieju commented Mar 24, 2024

satyarohith commented Mar 25, 2024

bartlomieju Apr 2, 2024

satyarohith Apr 2, 2024 •

edited

Loading

bartlomieju Apr 2, 2024

satyarohith Apr 2, 2024

bartlomieju left a comment

fix(ext/node): MessagePort works #22999

fix(ext/node): MessagePort works #22999

Conversation

satyarohith commented Mar 20, 2024 • edited Loading

bartlomieju commented Mar 24, 2024

satyarohith commented Mar 24, 2024

bartlomieju commented Mar 24, 2024

bartlomieju commented Mar 24, 2024

satyarohith commented Mar 25, 2024

bartlomieju Apr 2, 2024

Choose a reason for hiding this comment

satyarohith Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

bartlomieju Apr 2, 2024

Choose a reason for hiding this comment

satyarohith Apr 2, 2024

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

satyarohith commented Mar 20, 2024 •

edited

Loading

satyarohith Apr 2, 2024 •

edited

Loading