modules: prevent race condition while combining import and require

[BridgeAR]

This switches the async file read to the sync version to prevent
the file potentially being read multiple times during the actual
loading phase (that could happen if the translator is called multiple
times on the same file while the file read is not complete).

This is not critical (reading the files multiple times should be fine)
but it came up by eslint using the require-atomic-updates rule.

An alternative would be to use a map with all currently loading
files and each entry contains a promise that is resolved as soon as
the module is fully loaded. That way it could be awaited and the async
file read would still be fine. I have no strong opinion about either
solution @nodejs/modules-active-members.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3503
CI: https://ci.nodejs.org/job/node-test-pull-request/23792/
CI: https://ci.nodejs.org/job/node-test-pull-request/23804/
CI: https://ci.nodejs.org/job/node-test-pull-request/23821/
CI: https://ci.nodejs.org/job/node-test-pull-request/23834/ ✅ (only a known Windows flake "test.async-hooks/test-statwatcher" failed)

[addaleax]

An alternative would be to use a map with all currently loading
files and each entry contains a promise that is resolved as soon as
the module is fully loaded. That way it could be awaited and the async
file read would still be fine.

Yes, I think that’s a better solution – it’s great that we now have a module system that can read asynchronously from the disk.

[guybedford]

I'm not sure I understand why such a map is needed though? The loader registry exactly is that map as far as I'm concerned. Yes other parallel JSON loading mechanisms don't get to share it, but that doesn't seem like a problem to me?

[BridgeAR]

@guybedford it should impact things like (without testing - I do not know the esm code well and I just had a very brief look at this):

importfoofrom'./foo.json'importfoofrom'./foo.json'

Here both imports would start reading the file from disk because the second import call is happening while the first is still loading. Instead, it should resolve to the same file read.

[guybedford]

@BridgeAR these would resolve to the same module record, where the translator is only applied once per module record, so it wouldn't impact this case - there would only be one read.

[BridgeAR]

@guybedford yes, I see. moduleMap catches this early on. That's something that the eslint rule is not able to know about. Thanks for pointing that out.

It would AFAIK still be a race condition for the following case though (both files are in the same directory and are loaded directly after each other):

// file1.jsrequire('./foo.json')// file2.mjsimportfoofrom'./foo.json'

I guess it's unlikely that this happens but it's at least a possibility. We could add a ignore statement in case we want to activate the eslint rule instead of adding the extra check. Shall I close the PR or do you think this should be addressed?

[BridgeAR]

@nodejs/modules-active-members PTAL at #27674 (comment)

[guybedford]

@BridgeAR note that require() will inject its loads immediately into the ESM loader to ensure well-defined snapshotting, So even that example you provide is not race condition and will only do one JSON load.

That said, the converse is not currently true - import('./file.json'), require('./file.json') will result in a double load.

While I believe this is rare enough not to merit the need to make JSON loading sync (two stats for a very rare edge case is not a concern!), the fact that there is a different instance is a problem though, and this could possibly be resolved by having the require('./json') case "beat" the in-progress ESM load to ensure this.

I don't think the above should affect the discussion around whether JSON module parsing should be sync or async in Node.js, as that discussion should be based on performance arguments predominantly.

[BridgeAR]

I just updated the code with your suggestion @guybedford PTAL.

[guybedford]

Seems the best approach to me.

[Trott]

I imagine there's no reliable way to test this, is there?

[BridgeAR]

Landed in 9939762 🎉

[BridgeAR]

@Trott sorry, I just saw your comment after landing this. It is probably not easy to write a reliable test but it should be possible. Especially by using import.meta.url. I'll open a follow-up PR.

[BridgeAR]

@guybedford by our former discussion I would have expected the following to fail reliably before this patch but it always passes:

// Flags: --experimental-modulesimport'../common/index.mjs';importassertfrom'assert';import{createRequire}from'module';importjson1from'../fixtures/es-modules/json-cache/test.json';constrequire=createRequire(import.meta.url);constjson2=require('../fixtures/es-modules/json-cache/test.json');assert.strictEqual(json1,json2);

Could you have a look at it / suggest an alternate test?

---

Update: I can't get it to fail with dynamic import on Node.js v12.4.0 either.

[guybedford]

@BridgeAR it seems the ESM loader was injecting into the CJS loader cache for JSON specifically already, while this fix was particularly about the race condition of a CJS require while the ESM load was "in progress" (eg the while JSON is still being fetched through an async read).