fix(ext/node) worker_threads ESM handling #22841

mash-graz · 2024-03-11T02:54:55Z

I still have to look for better test example, because for the very simple parallel/test-worker-esmodule.js and its *.mjs worker file it doesn't make difference if this patch gets applied, but my projects, which use .js files in ordinary packages, do not work anymore without this modification.

I'll leave it as draft in the meanwhile...

dsherret

Thanks! Let me know if you need further help on this one.

Probably a test could be added in tests/integration/npm_tests.rs

dsherret · 2024-03-11T14:34:47Z

ext/node/polyfills/worker_threads.ts

        const cwdFileUrl = toFileUrl(Deno.cwd());
        specifier =
          `data:text/javascript,(async function() {const { createRequire } = await import("node:module");const require = createRequire("${cwdFileUrl}");require("${specifier}");})();`;
-      } else {
-        specifier = toFileUrl(specifier as string);
      }


Nevertheless, a more general solution for the file type detection...

There is a function, url_to_node_resolution on NodeResolver. Probably that should be used everywhere. Also, all files outside a node_modules directory are considered ESM in Deno (there is a in_npm_package function on NodeResolver to tell that).

The url_to_node_resolution handles only an URL to path resp. string conversion.
But it doesn't provide any file type related differentiation, if I'm not wrong.

Right now the format decision is done by looking at 1.) the file suffix and 2.) op_require_read_closest_package_json to check, if a typ: module entry is present somewhere in a package.json file above the script.

But the code without my patch always seemed to choose the exactly opposite consequence based on these checks! ESM files got wrapped in require(...) fragments and CJS files became imported by their URL, not the other way around! (<= But please double-check this claim!)

But .mjs files still seemed to work all the time somehow and simply ignored this defect.
That's why the unit test doesn't report an error.

The url_to_node_resolution handles only an URL to path resp. string conversion.
But it doesn't provide any file type related differentiation, if I'm not wrong.

It works for file:/// urls.

That's interesting!
Which property of the URL object could be used for a more useful import type specific proceeding?

@dsherret: I think, I finally understood now your url_to_node_resolution! :)

I'll try to use it as replacement here as well to minimize the code duplication and unexpected behavior.

ext/node/polyfills/worker_threads.ts

bartlomieju · 2024-03-11T22:29:47Z

@mash-graz can you link to some of the package you said don't work without this patch? We can boil down a test case out of them.

mash-graz · 2024-03-11T22:53:21Z

I noticed the issue while working on this jco porting attempts, but this working tree is in a such a horrible chaotic state right now, that I can hardly share it.

But I'll look, if I can extract a working example till tomorrow...

It's really crazy -- I can somehow understand, what's wrong with this particular code passage, but I can not grasp, why most examples, and the *.mjs files in particular, nevertheless work most of the time, although the get imported in an utterly inadequate way.

mash-graz · 2024-03-12T00:59:39Z

o.k. -- I have prepared a practical demonstration of this issue for you:

checkout the deno_demo branch of https://github.com/mash-graz/jco
at the top level directory use this command to start the mocha test run for the preview2-shim workspace:

❯ ../deno/target/debug/deno run -A npm:mocha -u tdd ./packages/preview2-shim/test/test.js

  Node.js Preview2
test stdouttest stderr    ✔ Stdio
error: Uncaught (in worker "") (in promise) Error: require() of ES Module /home/mash/Projects/jco/packages/preview2-shim/lib/io/worker-thread.js from /home/mash/Projects/jco not supported. Instead change the require to a dynamic import() which is available in all CommonJS modules.
    at createRequireEsmError (node:module:1084:15)
    at Object.Module._extensions..js (node:module:1065:13)
    at Module.load (node:module:919:32)
    at Function.Module._load (node:module:749:12)
    at Module.require (node:module:941:19)
    at require (node:module:1126:16)
    at data:text/javascript,(async function() {c......er-thread.js");})();:1:138

you will get this error, if you execute the unpatched deno git upstream version (47f3182)

but if you utilize this PR everything should work as expected and you will see something like this:

❯ ../deno/target/debug/deno run -A npm:mocha -u tdd ./packages/preview2-shim/test/test.js


  Node.js Preview2
test stdouttest stderr    ✔ Stdio
    1) FS read
    ✔ WASI HTTP (744ms)
    Clocks
      ✔ Wall clock
      ✔ Monotonic clock now
      ✔ Monotonic clock immediately resolved polls
      ✔ Monotonic clock subscribe duration
      ✔ Monotonic clock subscribe instant
    WASI Sockets (TCP)
      ✔ sockets.instanceNetwork() should be a singleton
      ✔ sockets.tcpCreateSocket() should throw not-supported
      ✔ tcp.bind(): should bind to a valid ipv4 address
      2) tcp.bind(): should bind to a valid ipv6 address and port=0
      ✔ tcp.bind(): should throw invalid-argument when invalid address family
      ✔ tcp.bind(): should throw invalid-state when already bound
      ✔ tcp.listen(): should listen to an ipv4 address
      3) tcp.connect(): should connect to a valid ipv4 address and port=0
    WASI Sockets (UDP)
      ✔ sockets.udpCreateSocket() should be a singleton
Warning: setTTL on UDP sockets ignored
      ✔ udp.bind(): should bind to a valid ipv4 address and port=0
Warning: setTTL on UDP sockets ignored
      ✔ udp.bind(): should bind to a valid ipv6 address and port=0
Warning: setTTL on UDP sockets ignored
      4) udp.stream(): should connect to a valid ipv4 address
Warning: setTTL on UDP sockets ignored
      ✔ udp.stream(): should connect to a valid ipv6 address (208ms)


  17 passing (2s)
  4 failing
  [...]

I really can't explain, why the import will not work in this case, but seems to work in many other situations. That's why I couldn't find a more suitable unit test.

But the reported Error: require() of ES Module was rather clear and further debug sessions lead me finally to this section in the source code resp. the suggested changes.

mash-graz · 2024-03-13T01:29:34Z

I just added two additional unit tests.

But creating them, I had to notice, that exceptions generated during Worker creations, will not be caught by these tests! :(

for example, when I try to create a worker with a non-existing path specifier (here: ./cjs-file.xjs), I will see this output:

[worker_threads] inheritances ...error: Uncaught (in worker "$DENO_STD_NODE_WORKER_THREAD") Module not found "file:///home/mash/Projects/deno/tests/unit_node/testdata/worker_module/cjs-file.xjs".
 ok (118ms)

But the test will nevertheless pass and report:

ok | 17 passed | 0 failed (911ms)

mash-graz · 2024-03-13T17:45:50Z

I finally figured out an indirect check for correct worker script import, which will fail on errors.
It's using a combination of error and message event expectation.

This should now at least cover all the possible processing paths within the patched sequence, although the CJS test is rather weak. I don't know if it should accept require(...) or __filename instructions in the imported content?

This commit fixes race condition in "node:worker_threads" module were the first message did a setup of "threadId", "workerData" and "environmentData". Now this data is passed explicitly during workers creation and is set up before any user code is executed. Closes #22783 Closes #22672 --------- Co-authored-by: Satya Rohith <me@satyarohith.com>

bartlomieju

Thank you for the PR @mash-graz, I'm going to mark it as "Request changes" so that we can first fix regressions in #22934 and #22935 to not introduce more moving pieces and potential regressions. We'll land this one as soon as we fix these two problems.

mash-graz · 2024-03-14T23:58:12Z

That's perfectly fine!

I try to prepare another improved/corrected version in the meanwhile. ;)

lino-levan · 2024-03-16T05:20:04Z

With both of the linked issues now being resolved, would this PR be up for another review?

mash-graz · 2024-03-16T05:40:32Z

Please wait just one or two days more!

I'm working on a much more satisfying rewrite, which handles both input options (ULRs or path entries) equally well. That's unfortunately not case with all these earlier variants. Therefore, this temporary draft flag again.

mash-graz · 2024-03-18T02:37:08Z

It's finally ready for review (again)! :)

I spend a lot of time into input verification and early exception throwing, because initialization errors on the worker side are usually only reported as uncaught exception and hard to handle in unit-tests, etc.

File and Data-URL-Objects should be accepted equally than absolute and relative path strings, in compliance to the expectations defined by the node documentation.

Although I did my best to handle also CJS scripts sufficiently by wrapping them in helper scripts, async mechanisms, Promises and timers will not work in this case. If you enable this line in the unit-test, the whole process will freeze:

deno/tests/unit_node/testdata/worker_module/cjs-file.cjs

Line 13 in 7a799d2

//const p = await missing_toplevel_async();

CJS support isn't utterly irrelevant in this particular case, because the support for ESM in workers in VMs in node is extraordinarily problematic. You'll therefore find much code, that still use these old fashioned standards especially for these tasks.

I hope, the changes are mostly acceptable.

And thanks again for all other help!

bartlomieju · 2024-03-18T17:41:17Z

@littledivy @satyarohith please review this PR, this might be overlapping with #22977 you're working on.

mash-graz · 2024-03-18T21:15:56Z

Now it's finally passing the CI pipeline, but one important test (execution of the CJS wrapped code) is disabled on windows! :(

That's a really inconvenient situation, because fixing this necessary differences in handling of ESM vs. CJS scripts was the main goal of this PR.

How should we go on?

Should just open another PR with a much simpler fix for the original issue as an interim solution, to quickly solve the troubles reported in Support for SvelteKit #17248 and my first issue report?
How do you value this attempt to rewrite this section in rust, instead of just using the existing TS code? All the platform specific troubles in regard to URL and file path handling increased my doubts about these attempts.
If you are willing to merge this kind of PR, I would at least add a runtime platform check and disable the CMS support on windows.

It's very hard for me to debug the cause of these windows specific bugs. I do not have a setup to reproduce them locally and always waiting for the CI results is an enormous waste of time...

Please give me just a little bit of honest feedback, what would look as the most appreciable solution to you?

mash-graz · 2024-03-18T21:58:28Z

The windows bug seems to be outside of my code changes in require():

 [worker_threads_test 011.97] thread 'worker-7' panicked at ext\node\ops\require.rs:198:57:
[worker_threads_test 011.97] called `Result::unwrap()` on an `Err` value: ()

I'll open another issue and Fix for this problem...
But It shouldn't be seen as a blocker for this PR, because it will happen with all other ad hoc fix attempts as well.

littledivy · 2024-03-19T04:20:26Z

Should just open another PR with a much simpler fix for the original issue as an interim solution, to quickly solve the troubles reported in #17248 and my first issue report?

Just tested this PR on Nuxt and Sveltekit. It solves the problem for both, let's try to get this merged.

How do you value this attempt to rewrite this section in rust, instead of just using the existing TS code? All the platform specific troubles in regard to URL and file path handling increased my doubts about these attempts.

I like the port to Rust. Rewrites like these are welcome since they reduce the startup snapshot size.

I just noticed you enabled the tests on Windows. Does it work now?

mash-graz · 2024-03-19T04:43:56Z

I just noticed you enabled the tests on Windows. Does it work now?

Yes -- it should now work on all platforms!
The CI system just stumbles over other flaky WPT problems.

I hope, it's more or less o.k. by now, but somebody should definitely take careful look on the code, because this windows related path issues nearly drove my crazy.

mash-graz · 2024-03-19T04:53:29Z

@bartlomieju could you please take another look at this code.

It should now work on all platforms, but im sure, many thinks could be done better.
If you find code-issues, that should be changed before merging -- no problem!

littledivy

LGTM

satyarohith

LGTM. Nice to have more operations done on the rust side.

mash-graz · 2024-03-19T05:46:18Z

thanks for the approvals! :)

not relevant anymore

fix node:worker_threads ESM handling

9c12fc0

mash-graz mentioned this pull request Mar 11, 2024

Consider unstable flag to make cjs files be treated as esm #22818

Closed

dsherret reviewed Mar 11, 2024

View reviewed changes

add .cjs check

b4b983c

mash-graz marked this pull request as ready for review March 11, 2024 15:51

add unit tests for CJS and nested ESM detection

8c07b89

mash-graz added 3 commits March 13, 2024 07:15

cleaner CJS unit test files

0685d89

fix fmt

924fdc3

unit test will now also fail on import errors

4d03a60

mash-graz mentioned this pull request Mar 14, 2024

Regression: make worker setup synchronous (#22815) may lead to freeze #22911

Closed

mash-graz marked this pull request as draft March 14, 2024 11:10

bartlomieju previously requested changes Mar 14, 2024

View reviewed changes

work in progress

6f3af97

mash-graz added 4 commits March 16, 2024 12:53

interim

3d9e4ee

rewritten node:worker_threads filename processing

322f4ac

Merge remote-tracking branch 'upstream/main' into esm-workers

1c70efe

fixing unit tests

7a799d2

mash-graz marked this pull request as ready for review March 18, 2024 02:37

extend error report to debug windows CI issue

5b3d8c9

mash-graz marked this pull request as draft March 18, 2024 03:53

mash-graz force-pushed the esm-workers branch from 331654f to fb97a49 Compare March 18, 2024 13:49

bartlomieju mentioned this pull request Mar 18, 2024

Support for SvelteKit #17248

Open

replace windows path separators in CJS-require() to '/'

4d02769

mash-graz force-pushed the esm-workers branch from fb97a49 to 4d02769 Compare March 18, 2024 19:12

temporary disable CJS test on windows

b03c6a6

mash-graz force-pushed the esm-workers branch 4 times, most recently from e8f7257 to f50c52f Compare March 19, 2024 01:55

let it crash again to figure out the reason

1d1ec76

mash-graz force-pushed the esm-workers branch from f50c52f to 1d1ec76 Compare March 19, 2024 02:06

mash-graz added 2 commits March 19, 2024 04:10

rervert remove of path split + replace separators

8b56479

this seems to work on all platforms

5fab2ce

mash-graz force-pushed the esm-workers branch from 87435a6 to 5fab2ce Compare March 19, 2024 04:16

satyarohith mentioned this pull request Mar 19, 2024

fix(ext/node): worker_threads load as commonjs if pkg.type is not module #22977

Closed

mash-graz requested a review from bartlomieju March 19, 2024 04:53

littledivy approved these changes Mar 19, 2024

View reviewed changes

satyarohith approved these changes Mar 19, 2024

View reviewed changes

mash-graz force-pushed the esm-workers branch from b870900 to b66ce2a Compare March 19, 2024 20:49

more simple relative path creation for CJS wrapper

a472413

mash-graz force-pushed the esm-workers branch from b66ce2a to a472413 Compare March 19, 2024 21:18

satyarohith merged commit 0d43a63 into denoland:main Mar 20, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ext/node) worker_threads ESM handling #22841

fix(ext/node) worker_threads ESM handling #22841

mash-graz commented Mar 11, 2024 •

edited by littledivy

dsherret left a comment

dsherret Mar 11, 2024

mash-graz Mar 11, 2024 •

edited

dsherret Mar 13, 2024

mash-graz Mar 13, 2024

mash-graz Mar 14, 2024

bartlomieju commented Mar 11, 2024

mash-graz commented Mar 11, 2024

mash-graz commented Mar 12, 2024

mash-graz commented Mar 13, 2024

mash-graz commented Mar 13, 2024

bartlomieju left a comment

mash-graz commented Mar 14, 2024

lino-levan commented Mar 16, 2024

mash-graz commented Mar 16, 2024

mash-graz commented Mar 18, 2024 •

edited

bartlomieju commented Mar 18, 2024

mash-graz commented Mar 18, 2024

mash-graz commented Mar 18, 2024

littledivy commented Mar 19, 2024

mash-graz commented Mar 19, 2024

mash-graz commented Mar 19, 2024

littledivy left a comment

satyarohith left a comment

mash-graz commented Mar 19, 2024

fix(ext/node) worker_threads ESM handling #22841

fix(ext/node) worker_threads ESM handling #22841

Conversation

mash-graz commented Mar 11, 2024 • edited by littledivy

dsherret left a comment

Choose a reason for hiding this comment

dsherret Mar 11, 2024

Choose a reason for hiding this comment

mash-graz Mar 11, 2024 • edited

Choose a reason for hiding this comment

dsherret Mar 13, 2024

Choose a reason for hiding this comment

mash-graz Mar 13, 2024

Choose a reason for hiding this comment

mash-graz Mar 14, 2024

Choose a reason for hiding this comment

bartlomieju commented Mar 11, 2024

mash-graz commented Mar 11, 2024

mash-graz commented Mar 12, 2024

mash-graz commented Mar 13, 2024

mash-graz commented Mar 13, 2024

bartlomieju left a comment

Choose a reason for hiding this comment

mash-graz commented Mar 14, 2024

lino-levan commented Mar 16, 2024

mash-graz commented Mar 16, 2024

mash-graz commented Mar 18, 2024 • edited

bartlomieju commented Mar 18, 2024

mash-graz commented Mar 18, 2024

mash-graz commented Mar 18, 2024

littledivy commented Mar 19, 2024

mash-graz commented Mar 19, 2024

mash-graz commented Mar 19, 2024

littledivy left a comment

Choose a reason for hiding this comment

satyarohith left a comment

Choose a reason for hiding this comment

mash-graz commented Mar 19, 2024

mash-graz commented Mar 11, 2024 •

edited by littledivy

mash-graz Mar 11, 2024 •

edited

mash-graz commented Mar 18, 2024 •

edited