module: prevent main thread exiting before esm worker ends

[islandryu]

This fix ensures the main thread does not terminate before an ESM worker's handleMessage function completes by moving the atomic lock release to the end of the function. As far as I can see, there doesn’t seem to be any necessary processing after the lock release.

Problem

The test-esm-loader-spawn-promisified.mjs test occasionally fails in debug builds. If processing after the lock release exceeds the stack limit, it results in a fatal error:

✖ can predetermine the format in the custom loader resolve hook (162.586375ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  + actual - expected
  
  + '\n' +
  +   '\n' +
  +   '#\n' +
  +   '# Fatal error in ../../deps/v8/src/api/api.cc, line 5021\n' +
  +   '# Debug check failed: !i_isolate->is_execution_terminating().\n' +
... 

Even if it fails in a debug build, it is desirable for the error to be a JavaScript error, such as Maximum call stack size exceeded, rather than a debug check error.

Although it involves modifying the internal code, the issue can be reproduced as follows:

// Insert code that intentionally exceeds the stack limit at line 220 of lib/internal/modules/esm/worker.js before the fix.
    if(method === "load" && args[0].includes("imported.mjs")) {
      function func() {
          func();
      }
      func();
    } 

// index.mjs
await import("./imported.mjs")
console.log("done")

// run index.mjs with any loader and any imported.mjs

Test

Testing this precisely is difficult due to timing sensitivity. A new test verifies that a deliberate stack overflow results in the correct Maximum call stack size exceeded error, ensuring the lock behavior is correct.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.49%. Comparing base (ac7fea6) to head (b0c0ece).
Report is 25 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56183      +/-   ##
==========================================
+ Coverage   88.01%   88.49%   +0.48%     
==========================================
  Files         656      656              
  Lines      189136   189263     +127     
  Branches    36004    36348     +344     
==========================================
+ Hits       166461   167497    +1036     
+ Misses      15842    14977     -865     
+ Partials     6833     6789      -44     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/worker.js	91.35% <100.00%> (+0.06%)	⬆️

... and 101 files with indirect coverage changes

[mcollina]

lgtm with a nit

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63941/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63944/

[JakobJingleheimer]

Sorry I'm swamped this week.

This sounds like something we tried back when that led to a deadlock in some circumstances. I was going to ask Antoine, who I think worked on that specific scenario with me; I see he has reviewed this though.

[JakobJingleheimer]

I can't think of the specific issue; we surely would have added a test-case for it before landing, so if the tests are passing, it should be okay!

Thanks for this!

Does this also address console.logs getting "lost"? If so, the API docs should to be updated to remove the caveat. I think that should happen in this PR for an atomic change.

[nodejs-github-bot]

Landed in 2063245

[islandryu]

@JakobJingleheimer

Sorry for the delayed response.
Regarding "console.logs getting lost," are you referring to this part?
https://nodejs.org/api/module.html#hooks

Hooks are run in a separate thread, isolated from the main thread where application code runs. That means it is a different realm. The hooks thread may be terminated by the main thread at any time, so do not depend on asynchronous operations (like console.log) to complete.

I made the change to lock the main thread until handleMessage completes, but I overlooked asynchronous considerations.
There might still be a possibility of logs getting lost in the sections executed after handleMessage.