Fix the WASM Worker cannot start in node.js environment #20188

medns · 2023-09-05T03:19:19Z

emscripten_malloc_wasm_worker and
emscripten_create_wasm_worker functions
adapted to the nodejs environment and
modified the core related tests.

sbc100

I'll let @juj do the main review here.

src/library_wasm_worker.js

test/test_core.py

src/preamble.js

src/wasm_worker.js

src/library_wasi.js

sbc100 · 2023-09-27T15:05:05Z

src/parseTools.js

@@ -1023,3 +1035,7 @@ function getPerformanceNow() {
    return 'performance.now';
  }
 }
+
+function implicitSelf() {
+  return ENVIRONMENT.includes('node') ? 'self.' : '';


What happens if the environment includes both node and web and we run the result on the web? i.e. isn't choosing to use self. or not here at compile time too early.. don't we need to decide at runtime maybe?

In the web environment, it can run with or without the self. prefix, but in the node environment, it must have the self. prefix to run.
So I think the above code is correct.

Ah I see. It looks like we have the same pattern in src/worker.js but we simply always use self there.

I wonder if we could find a way to avoid the self. completely by using defineProperty on the global object under node? Maybe that would be more code overall?

I think this issue is the same as the issue below, it can create a separate PR after merging this to deal with these two issues together.

src/preamble.js

src/library_wasi.js

sbc100 · 2023-09-27T15:40:30Z

src/wasm_worker.js

+    removeEventListener: (name, handler) => parentPort.off(name, handler),
+  });
+}
+#endif // ENVIRONMENT_MAY_BE_NODE


This seems like duplication of the code in src/worker.js .. is it worth factoring out into a separate file that can be included in both places. Something like src/node_webworker_polyfill.py?

I think this issue is the same as the issue above, can create a separate PR after merging this to deal with these two issues together.

src/library_wasi.js

sbc100

lgtm % comments from me. I'll leave @juj to approve

src/preamble.js

src/parseTools.js

src/library_wasm_worker.js

src/preamble.js

juj · 2023-10-16T12:27:29Z

Looks very nice, great work. I had one comment about the subtle distinction between SHARED_MEMORY and PTHREADS/WASM_WORKERS. Otherwise LGTM to land.

kripken · 2023-10-16T22:24:30Z

src/library_wasm_worker.js

-    let data = e.data, wasmCall = data['_wsc'];
+#if ENVIRONMENT_MAY_BE_NODE
+    // In Node.js environment, message event 'e' containing the actual data sent,
+    // while in the browser environment it's contained by 'e.data'.


Why is this the case, though? If it arrives from a Node.js or Web API then that means this workaround is necessary, but if not, then if it arrives from our own code, can we send data in the same format?

Oh, reading that again, I think you're saying it's the first? We send data the same way, but Node and the Web end up providing it differently? If so then lgtm.

Oh, reading that again, I think you're saying it's the first? We send data the same way, but Node and the Web end up providing it differently? If so then lgtm.

yeah, you are right! 👍

`emscripten_malloc_wasm_worker` and `emscripten_create_wasm_worker` functions adapted to the nodejs environment and modified the core related tests.

kripken

lgtm and tests now passed.

@sbc100 did you want to take another look before we land?

sbc100

I would like see the node worker stuff factored out as a followup, but otherwise lgtm

Back in #21701 I moved the creation of wasmOffsetConvert and wasmOffset into runtime_pthread.js but I didn't update libwasm_worker.js which still uses the old name `wasmOffsetData` in its initial post message. When trying to update this just now I noticed that the offset converter (and santiizers in general) don't seem to work on `WASM_WORKERS` so this change simply disables them. The passing of `wasmOffsetData` and `wasmSourceMapData` was added back in #20188 but it doesn't looks like support for santizers was supposed to be part of that.

medns force-pushed the fix/create_wasm_worker branch 4 times, most recently from 4d72e2d to bbf542b Compare September 5, 2023 08:08

medns marked this pull request as draft September 5, 2023 13:26

medns force-pushed the fix/create_wasm_worker branch 3 times, most recently from 909b453 to 17705e2 Compare September 6, 2023 07:16

medns marked this pull request as ready for review September 6, 2023 08:44

medns closed this Sep 6, 2023

medns reopened this Sep 6, 2023

sbc100 reviewed Sep 6, 2023

View reviewed changes

src/library_wasm_worker.js Outdated Show resolved Hide resolved

test/test_core.py Outdated Show resolved Hide resolved

sbc100 requested a review from juj September 6, 2023 23:10

medns force-pushed the fix/create_wasm_worker branch 4 times, most recently from 7452988 to 858f974 Compare September 8, 2023 17:32

sbc100 reviewed Sep 8, 2023

View reviewed changes

test/test_core.py Outdated Show resolved Hide resolved

sbc100 reviewed Sep 8, 2023

View reviewed changes

test/test_core.py Show resolved Hide resolved

medns force-pushed the fix/create_wasm_worker branch 2 times, most recently from 2cfc714 to 3b0a440 Compare September 9, 2023 06:18

juj reviewed Sep 12, 2023

View reviewed changes

src/preamble.js Outdated Show resolved Hide resolved

juj reviewed Sep 12, 2023

View reviewed changes

src/wasm_worker.js Outdated Show resolved Hide resolved

medns force-pushed the fix/create_wasm_worker branch 3 times, most recently from 2b79d96 to 1a92216 Compare September 19, 2023 03:12

medns requested review from sbc100 and juj September 19, 2023 04:40

medns force-pushed the fix/create_wasm_worker branch from 1a92216 to b581535 Compare September 20, 2023 02:57

kripken reviewed Sep 25, 2023

View reviewed changes

src/wasm_worker.js Outdated Show resolved Hide resolved

medns force-pushed the fix/create_wasm_worker branch 3 times, most recently from cc68e65 to 0771c46 Compare September 27, 2023 14:04

sbc100 reviewed Sep 27, 2023

View reviewed changes

medns force-pushed the fix/create_wasm_worker branch 3 times, most recently from e13171e to f4c6219 Compare September 27, 2023 16:21

sbc100 reviewed Sep 27, 2023

View reviewed changes

src/library_wasi.js Outdated Show resolved Hide resolved

medns force-pushed the fix/create_wasm_worker branch 3 times, most recently from 3c57233 to a23f797 Compare September 27, 2023 19:18

medns force-pushed the fix/create_wasm_worker branch from a23f797 to 5412c93 Compare October 7, 2023 07:02

sbc100 reviewed Oct 9, 2023

View reviewed changes

src/preamble.js Show resolved Hide resolved

medns force-pushed the fix/create_wasm_worker branch from 5412c93 to 0d0e9c9 Compare October 10, 2023 08:18

sbc100 reviewed Oct 10, 2023

View reviewed changes

src/parseTools.js Outdated Show resolved Hide resolved

medns force-pushed the fix/create_wasm_worker branch from 0d0e9c9 to d272679 Compare October 12, 2023 06:37

kripken reviewed Oct 12, 2023

View reviewed changes

src/library_wasm_worker.js Show resolved Hide resolved

medns force-pushed the fix/create_wasm_worker branch from d272679 to b5e0782 Compare October 13, 2023 09:19

juj reviewed Oct 16, 2023

View reviewed changes

src/preamble.js Outdated Show resolved Hide resolved

kripken reviewed Oct 16, 2023

View reviewed changes

medns added 2 commits October 17, 2023 11:12

Fix the WASM Worker cannot start in node.js environment

29ecfce

`emscripten_malloc_wasm_worker` and `emscripten_create_wasm_worker` functions adapted to the nodejs environment and modified the core related tests.

Fix the WASM Worker cannot start in node.js environment

55b0af7

`emscripten_malloc_wasm_worker` and `emscripten_create_wasm_worker` functions adapted to the nodejs environment and modified the core related tests.

medns force-pushed the fix/create_wasm_worker branch from b5e0782 to 55b0af7 Compare October 17, 2023 03:14

kripken approved these changes Oct 17, 2023

View reviewed changes

sbc100 approved these changes Oct 18, 2023

View reviewed changes

kripken merged commit 8a67aaf into emscripten-core:main Oct 18, 2023

sbc100 mentioned this pull request Apr 22, 2025

Disallow -sWASM_WORKERS with sanitizers #24171

Merged

Fix the WASM Worker cannot start in node.js environment #20188

Fix the WASM Worker cannot start in node.js environment #20188

Uh oh!

Conversation

medns commented Sep 5, 2023

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juj commented Oct 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!