module: allow cycles in require() in the CJS handling in ESM loader

When --import is used, the ESM loader is used to handle even pure
CJS entry points, and it can run into CJS module facades in the
evaluating state when the parent CJS module is being evaluated.
In this case it should be allowed, since the ESM <-> CJS cycles
that are meant to be disallowed (for the time being) should
already be detected before evaluation and wouldn't get here,
and CJS <-> CJS cycles are fine.

PR-URL: nodejs#58598
Fixes: nodejs#58515
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>

Author: joyeecheung

lib/internal/modules/esm/module_job.js

@@ -25,6 +25,7 @@ const {
   ModuleWrap,
   kErrored,
   kEvaluated,
+  kEvaluating,
   kInstantiated,
   kUninstantiated,
 } = internalBinding('module_wrap');
@@ -301,8 +302,14 @@ class ModuleJob extends ModuleJobBase {
         return { __proto__: null, module: this.module, namespace };
       }
       throw this.module.getError();
-
-    } else if (status === kEvaluated) {
+    } else if (status === kEvaluating || status === kEvaluated) {
+      // kEvaluating can show up when this is being used to deal with CJS <-> CJS cycles.
+      // Allow it for now, since we only need to ban ESM <-> CJS cycles which would be
+      // detected earlier during the linking phase, though the CJS handling in the ESM
+      // loader won't be able to emit warnings on pending circular exports like what
+      // the CJS loader does.
+      // TODO(joyeecheung): remove the re-invented require() in the ESM loader and
+      // always handle CJS using the CJS loader to eliminate the quirks.
       return { __proto__: null, module: this.module, namespace: this.module.getNamespaceSync() };
     }
     assert.fail(`Unexpected module status ${status}.`);

src/module_wrap.cc

@@ -694,11 +694,10 @@ void ModuleWrap::GetNamespaceSync(const FunctionCallbackInfo<Value>& args) {
       return realm->env()->ThrowError(
           "Cannot get namespace, module has not been instantiated");
     case v8::Module::Status::kInstantiated:
+    case v8::Module::Status::kEvaluating:
     case v8::Module::Status::kEvaluated:
     case v8::Module::Status::kErrored:
       break;
-    case v8::Module::Status::kEvaluating:
-      UNREACHABLE();
   }
 
   if (module->IsGraphAsync()) {

test/es-module/test-import-preload-require-cycle.js

@@ -0,0 +1,20 @@
+'use strict';
+
+// This tests that --import preload does not break CJS entry points that contains
+// require cycles.
+
+require('../common');
+const fixtures = require('../common/fixtures');
+const { spawnSyncAndAssert } = require('../common/child_process');
+
+spawnSyncAndAssert(
+  process.execPath,
+  [
+    '--import',
+    fixtures.fileURL('import-require-cycle/preload.mjs'),
+    fixtures.path('import-require-cycle/c.js'),
+  ],
+  {
+    stdout: /cycle equality true/,
+  }
+);

test/fixtures/import-require-cycle/a.js

@@ -0,0 +1 @@
+module.exports.b = require('./b.js');

test/fixtures/import-require-cycle/b.js

@@ -0,0 +1 @@
+module.exports.a = require('./a.js');

test/fixtures/import-require-cycle/c.js

@@ -0,0 +1,3 @@
+const obj = require('./b.js');
+
+console.log('cycle equality', obj.a.b === obj);

test/fixtures/import-require-cycle/preload.mjs

@@ -0,0 +1,10 @@
+import * as mod from "module";
+
+// This API is not available on v20.x. We are just checking that a
+// using a --import preload to force the ESM loader to load CJS
+// still handles CJS <-> CJS cycles just fine.
+// mod.registerHooks({
+//   load(url, context, nextLoad) {
+//     return nextLoad(url, context);
+//   },
+// });