Open issues with require(esm)

[hybrist]

This issue is meant to track concerns raised in the past about require(esm). I'm hoping to preempt having these discussions in January after we (or rather @weswigham) already invested more work into providing an implementation of require(esm).

Previous discussions:

• Is require with ESM planned? nodejs/modules#308
• Conditional Exports section of 2019-10-23
• Unflagging in 12.x
• Proposal for single-mode packages with optional fallbacks for older versions of node
• Issue asking for a way to turn async code into sync code

From the 2019-10-23 meeting notes:

Wes’ points are valid. Except we should be decisive about require(ESM). Wes did good work. Now that work has stalled, we can’t say it will appear in 6 months. But it’s not spec compliant with ESM. Promises are specced to suspend main thread. Flattening promises would be non-spec compliant. So problem is lack of work and spec adherence. Top-level await makes this observable.

— @guybedford (emphasis mine)

From #308:

Even if require(esm) was provided today it becomes hazardous to use over time as async-to-eval modules get introduced. For example a JS module using top-level await or a WebAssembly module. This can happen in dependencies you don't control, so it's not easy to defend against this hazard.

For this reason I'm also skeptical that this feature could be provided in a safe way that guaranteed future compatibility.

— @robpalme

[hybrist]

Example 1: Transitive Dependency Uses TLA

// This code works perfectly fine - until a transitive dependency of `some.mjs` uses TLA.
// Then suddenly it breaks and it's afaict impossible to fix since there's nothing to wait for.
// myThing suddenly is undefined even though some.mjs didn't change.
const { myThing } = require('./some.mjs');
myThing();

[hybrist]

Example 2: Temporary Event Loop and Resources

The syncify in the existing prototype requires the introduction of a separate event loop.

See: weswigham/ecmascript-modules@3e6a768

If any part of the loading pipeline, including custom loaders, creates resources attached to the event loop, they can prevent the event loop from terminating. Reversely, deref'd resources may appear to "hang" with timeouts. Also, if a long-running handle is created, it may still reference the old event loop (e.g. opening a file archive and keeping it open for future loads).

[coreyfarrell]

Loader hooks are async so I expect code which performed require(esm) would break when run under tooling which injects loader hooks. This would be problematic for tooling as users would blame the tool for an issue that would be outside the control of the tool.

[bmeck]

@coreyfarrell no, loader hooks work fine with sync code. see https://docs.google.com/presentation/d/16XSS7P2RMUtpBA8iolaMV9MpLP_Yot8lAml1trbl38I/edit?usp=sharing for details on how that is achievable.

[hybrist]

@bmeck I think that comes with an important qualifier though, right? "Hopefully upcoming off-thread loader hooks work fine with sync code". Because the current dynamic instantiate hook isn't necessarily safe in this context afaict.

[bmeck]

@jkrems correct, the current loader hooks are not expected to survive in their state and I'd be unlikely to endorse on thread/shared heap for them currently.

[weswigham]

@robpalme 's concern about async-to-execute stuff entering via your dependencies isn't unique to require(esm) - TLA is observable in modules in the same way. It's no worse than (or doesn't need to be any worse than) TLA in that regard, however you feel about that.

[weswigham]

The syncify in the existing prototype requires the introduction of a separate event loop.

Yah, so does child_process.execSync. Your point? It's done elsewhere in the node core code. The "hazard" is that if a cross-loop event dependency was taken, there'd potentially be a deadlock. This is not unique to require(esm) - any async pattern with a form of "locks" can deadlock (in this case, which loop is actively executing essentially holds a lock on the execution). There's obviously at least two ways to avoid this, since the problems of async execution models are well known - preemtive rescheduling (restarting the parent loop even if the child still has content after awhile to try to guarantee liveness), or simply mindful implementation that doesn't contend on the lock (eg, so the algorthms syncified only use internal event deps) which is a kind of cooperative multitasking. This is implemented in core precisely so that that invariant can be upheld (as, in core, you'd need to do something silly like in the resolver, like wait on a promise that was created prior to starting resolution, which can be easily avoided, so long as you know to), so a deadlock does not occur, which is much nicer than the "preemptive" case (and much easier to explain the result of).

[weswigham]

But it’s not spec compliant with ESM. Promises are specced to suspend main thread. Flattening promises would be non-spec compliant. So problem is lack of work and spec adherence. Top-level await makes this observable.

I think the reverse. Top-level await's spec chicanery very explicitly makes "flattening" promises a thing. All the execution APIs are supposed to be async to handle TLA; but TLA is also specced, very explicitly, to skip all those extra event loop turns and promise resolutions in cases where the module doesn't actually use await, in order to maintain comparability with the current execution expectation that "if a module graph only contains sync code, its whole execution happens sync". This is a flattening of a promise, a .unwrapSync call, if you will. Given this is observable in the world with TLA, I don't see it being observable in require(esm) being an issue.

[weswigham]

Long story short, you point out something you find distasteful with require(esm), and I can probably point out how TLA in modules ends up requiring the runtime do the same thing. If anything, I feel vindicated by the current TLA implementation. If we could get a createFunction v8 API that supported setting the Async flag (which doesn't seem hard to make from the internals I looked at, but because I'm not familiar with v8, I'm unsure how it should get done), I totally think we could even support TLA in cjs with the same caveats it has in esm (sync code is immediately unwrapped, otherwise it's waited on and can deadlock on contended resources, just like TLA). (Though it begs the question of if you can have a sloppy mode Async-flagged function, since cjs modules are sloppy mode by default.)

[guybedford]

The specific spec invariant with require of TLA is that the event loop is being forked, with the main event loop effectively stalled while the ESM event loop runs separately. When spawning a child process this is being done using the process boundaries. But within the same memory and realm this violates a number of spec timing properties of the language all at once. JavaScript is not designed to support two event loops running, where which event loop code is loaded through, when, and when they will be locked / running is non determinate.

…

On Tue, Dec 10, 2019 at 14:37 Wesley Wigham ***@***.***> wrote: Long story short, you point out something you find distasteful with require(esm), and I can probably point out how TLA in modules ends up requiring the runtime do the same thing. If anything, I feel vindicated by the current TLA implementation. If we could get a createFunction v8 API that supported setting the Async flag (which doesn't seem hard to make, but because I'm not familiar with v8, I'm unsure how it should get done), I totally think we could even support TLA in cjs with the same caveats it has in esm (sync code is immediately unwrapped, otherwise it's waited on and can deadlock on contended resources, just like TLA). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#454?email_source=notifications&email_token=AAESFSW2KBAFN6PHBR6STATQX7VV3A5CNFSM4JZBQMV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGQSQVA#issuecomment-564209748>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESFSXSTI3KTOAJVCSEFDTQX7VV3ANCNFSM4JZBQMVQ> .

[hybrist]

TLA is observable in modules in the same way. It's no worse than (or doesn't need to be any worse than) TLA in that regard, however you feel about that.

Are you talking about a previous draft of TLA? I'm fairly certain that the ESM equivalent of Example 1 is perfectly safe and doesn't break if a transitive dependency uses TLA.

import { myThing } from './some.mjs';
// We definitely know that myThing is initialized, even if some transitive dep of some.mjs
// is using top level await.
myThing();

Can you be more specific where TLA in a transitive dep is observable in the same way in ESM?

[weswigham]

@jkrems I'm talking about how If I have

// @filename: root.js
import {thing} from "./setUpThing.js";
import "some-library";
import {consume} from "./consumeThing.js";
consume(thing);
// @filename: setUpThing.js
export let thing = true;
function removeThing() {
    thing = undefined;
}
Promise.resolve().then(removeThing); // schedule `thing` to be immediately removed because reasons - must be used synchronously
// @filename: consumeThing.js
export function consume(thing) {
   console.log(thing ? "all sync deps" : "async dep somewhere");
}

the log in consume can detect if some-library contains, somewhere in it's dependency tree, TLA, as if it did, the event loop turned between the first import and the third import, thereby executing the removeThing function. The current TLA allows interleaved execution once await is in play (and, as a result, allows you to deadlock yourself if you await on something that can never execute - this is known).

With respect to

const { myThing } = require('./some.mjs');
myThing();

given how TLA works in ESM, I think it's now reasonable to wait on any promises on some.mjs's execution if they can't be immediately unwrapped - actually, if I'm to replicate how the current TLA works in esm, I don't think I even need a secondary event loop anymore (as that was moreso to prevent any interleaved execution, which the current TLA proposal actually allows once TLA is in use) - I can just unwrap immediately, if possible, and, if not, spin the main event loop until the promise we're waiting on resolves or rejects (then finally yield back to the caller of require with the value or throw). Nothing worse should happen with require(esm) than what's possible with TLA.

[hybrist]

Okay, I'd prefer if we focus on the relatively clear usability example that doesn't require any special code (other than "some dependency uses TLA in any way whatsoever") to break. I don't think it's fair to put your example on the same level here because it requires very specific coding patterns.

So, looking at your explanation: It sounds like what you're actually suggesting as the solution is that during the execution of require, we would allow arbitrary async code execution by running the event loop until a certain condition is met (e.g. promise for the load job is settled). Which means that code like this is no longer safe:

const x = fs.createReadStream('./x');
require('./foo'); // x may start emitting error events
x.on('error', console.error);

That's definitely more of an edge case compared to "Example 1" since it requires that require is running interspersed with other application code. But I'm not sure I would support a node version that removes the old guarantee that there's no ticks during require.

[weswigham]

Which means that code like this is no longer safe:

TLA does the same for esm. That's the tradeoff TLA makes:

const x = fs.createReadStream('./x');
await import('./foo'); // x may start emitting error events
x.on('error', console.error);