Always run async loader hooks synchronously from the non-loader thread

[joyeecheung]

This was an idea discussed in the loaders group a few times in the past as well as in the last collaboration summit. Opening an issue to track it properly.

This is related to #55782 and will allow us to

1. Eliminate the races in ESM cache handling when require(esm) needs the linking to be synchronous but import esm may populate an asynchronous linking job in the cache. There were multiple band-aids applied to work around this, like esm: link modules synchronously when no async loader hooks are used #59519 and module: handle cached linked async jobs in require(esm) #57187, but to fully eliminate the races, this would be necessary to finish Tracking Issue: Syncify the ESM Loader #55782
2. Go back to using the authentic require() in imported CJS instead of re-inventing a separate require() and eliminate its quirks, which currently includes
   1. The re-invented require lacks a lot of properties like require.cache, which is a documented caveat https://nodejs.org/api/module.html#caveat-in-the-asynchronous-load-hook
   2. The source code for CJS ended up being loaded twice

      node/lib/internal/modules/esm/load.js

      Lines 97 to 99 in 0fd1ecd

      	// For backward compatibility reasons, we need to discard the source in
      	// order for the CJS loader to re-fetch it.
      	source = null;

   3. As a side-effect of the previous quirk, when the module format is CJS, the user hook has to also deal with source being null and usually also has to load it twice or more, like what's being done in import-in-the-middle, see ERR_INVALID_RETURN_PROPERTY_VALUE when using module.register and module.registerHooks #57327 (comment) - if they don't override the nullish value, the require in imported CommonJS won't trigger the hooks.
   4. It gets import in its conditions array, instead of require (because it's using ESM resolution)
   5. The resolve hook gets the full URL, not the specifier passed to require()
   6. Invoking the sync hooks twice for CJS, one from the re-invented require, one from the Module._load invocation in the evaluation step.

Refs: #57327
Refs: #55808
Refs: #52697
Refs: #56241
Refs: #55782

cc @nodejs/loaders

[GeoffreyBooth]

Then we will be able to:

Yes please get rid of all that awfulness.

How do you intend to achieve this, though? Use everysync to add the off-thread aspects, to mimic the current behavior?

[joyeecheung]

How do you intend to achieve this, though? Use everysync to add the off-thread aspects, to mimic the current behavior?

Basically just the pseudocode in the OP.

I am not sure how the module hooks should behave on the loader thread though - the module loading is currently already extra quirky on the loader thread, for example require() is not customized on the loader thread at all because, well, you can't block on yourself. I wonder if there are that many people actually relying on the loaders affecting the loading of other loaders or do most existing users just don't care. IMO it would be less of a footgun to just let the off-thread loaders not affect the loading of each other and only affect the loading on the main thread. But if we really want to maintain compatibility with the existing behavior, we can leave the module loader on the loader thread as-is..with all the existing quirks, and only fix the main thread loading behavior.

[GeoffreyBooth]

But if we really want to maintain compatibility with the existing behavior

I think this is the crux of the issue. If you want to keep module.register at all, then presumably it should be preserved without breaking changes until it's ready to be removed when registerHooks is ready; but if you're going to refactor it so significantly that the changes will break most module.register users, then why bother? Maybe it's better to just remove module.register completely now, even if registerHooks is still experimental, rather than breaking register a few more times before its eventual removal?

I fully support whatever you want to do, and if the work you describe here somehow helps us get to a stable registerHooks faster, then by all means please proceed. Maybe the changes you're describing here are technically breaking but in practical terms they won't break too many users, and so it's worth doing this small break rather than the larger break of just deleting register. I trust your judgment.

[joyeecheung]

From what I can tell, it doesn't have to be breaking - just that if we want to maintain the current loader thread behavior, the quirks mentioned in the OP would still exist in the module loading of the loader thread (i.e. if an async loader depends on require(esm) in its own dependency, this is still subject to races; also all CJS dependencies of async loaders still won't be affected by other async loaders). We'll just only fix module loading for the main thread, and if any loader author want to avoid those quirks in their own graph, the best they can do is to switch to module.registerHooks.

[joyeecheung]

I tried to refactor the loader locally and I have a clearer picture of how feasible this is: I think we can achieve 1 and 2 for the non-loader thread. For module loading within the loader thread, 1 will still remain (but that might be a bit niche - to make that happen a async loader hook would need to hit require(esm) on the loader thread). 2 just never worked on the loader thread anyway (i.e. hooks can't customize require() in the module loading on the loader thread, all they can do is to customize require() on the non-loader thread) and we can get rid of that on the non-loader thread. I think 3 would be difficult to do - the loader thread needs the ability to "asynchronously customize the module loading of the current thread from the current thread" (i.e. the old loader behavior) back. Internally, we can kind of make it work by "if this is running on the loader thread, allow these paths to return a promise and do things asynchrnously and when it comes from a use case where sychronicity is needed e.g. require() and import.meta.resolve(), just ignore the async hooks", which is also the current behavior. But hoisting this part out to user land might reveal too many internal details that it's better to keep it internal.

[joyeecheung]

A few more quirks I discovered with the existing async hook behavior for the re-invented require() in imported CJS (it goes all the way to v20)

1. It gets import in its conditions array, instead of require (because it's using ESM resolution)
2. The resolve hook gets the full URL, not the specifier
3. The load hook must override the nullish source for CommonJS by e.g. reading the code from disk - if not, it uses the original require instead of the re-invented one, so the async hooks won't get triggered for require().

import { inspect } from 'node:util';
export async function resolve(specifier, context, nextResolve) {
  fs.writeSync(1, `resolve ${specifier} with ${inspect(context)}\n`);
  return nextResolve(specifier, context);
}


export async function load(url, context, nextLoad) {
  const result = await nextLoad(url, context);
  // Without this, async hooks won't get triggered for `require()` in imported CJS
  result.source ??= fs.readFileSync(new URL(url), 'utf8');
  return result;
}

// The resolve hook gets file://path/to/cjs.cjs as the specifier, not `./cjs.cjs`
// and context.conditions is [ 'node', 'import', 'module-sync', 'node-addons' ]
require('./cjs.cjs');

I think we can potentially fix them (for the non-loader-hook thread) if we move the async hook invocation to within the sync hook invoked in the original require() implementation

[joyeecheung]

With this branch I've made the async hooks always run synchronously from the non-loader-hook thread, which fixes the async hook invocation for require(esm) previously it would usually not work due to the races caused by having both async and sync loading sharing the same cache. Also verified that import-in-the-middle tests still pass after the change. Opened #60278 as the first step as the paths need to be clarified a bit before being updated.