The future of globalPreload

[GeoffreyBooth]

In working on nodejs/node#44710, moving loaders off-thread, I’ve run into various issues related to the globalPreload hook. It has an undocumented setImportMetaCallback function available alongside the getBuiltin and port globals that are provided for use in the bespoke sloppy script environment for use by globalPreload code; both setImportMetaCallback and port are problematic for moving loaders off thread, because import.meta.resolve needs to be defined in the main thread as far as I can tell, and is the port supposed to provide a link into the loaders thread or into the module loader that lives on the main thread? All of these issues might be solvable, but I’m starting to wonder if they’re worth the effort.

Starting with just setImportMetaCallback and port, these were added in nodejs/node#39240, which aimed to prove that mocking was possible with loaders. As far as I can tell, neither of these features have been used anywhere else or built on at all; and mocking already has solutions with broad adoption, like https://github.com/testdouble/testdouble.js/ and https://github.com/testdouble/quibble. Here’s the globalPreload use in Quibble, one of the very few uses of globalPreload that I’ve been able to find in the wild:

function loadQuibble () {
  global.__quibble = { stubModuleGeneration: 1 }
}

export function globalPreload () {
  return `(${loadQuibble.toString()})()`
}

It doesn’t use setImportMetaCallback and port, which leads me to believe that either the new features aren’t necessary for mocking or they don’t enable any improvements over existing solutions. Whereas conversely, they add a lot of complexity and a significant maintenance burden. I think that we should remove them, at least until after nodejs/node#44710 lands; and any PR to re-add them needs to justify why they’re needed and better than existing solutions.

More broadly, I’m wondering why we need globalPreload at all. It’s a weird API: sloppy mode scripts, neither ESM nor CommonJS, that can’t import or require dependencies other than Node builtins. The only examples or usage that I can find set a few trivial global-scope variables like counters. Couldn’t the same be achieved by prepending some code to the source returned by the load hook?

Looking at the original PR that created globalPreload, then called getGlobalPreloadCode, the justifications I see given are:

This work is meant to unblock progress on moving hook execution out of the main thread (and global scope). It creates an official place where hooks may interact with the global scope, hopefully allowing us to remove the need for random global side effects elsewhere. . . .

One fundamental design decision implied here is: there are multiple people who want to move loader hooks to run on a separate thread.

The use cases are instrumenting core modules and setting up any kind of global state required by code generated by other hooks. E.g. if transformSource assumes the existence on certain global runtime functions. Another use case is global attenuation / locking down the runtime before user code runs.

Right now loader hooks may just rely on the fact that they happen to run in the same global scope as all other code but when moving to threads, that is no longer true.

The security concerns are already much better handled by a few other in-progress APIs under https://nodejs.org/api/permissions.html. The “define things in global scope” like polyfills or helpers can be done via --import, which is a much better way to do so because --import allows real ESM code that can actually import dependencies. I would think that loaders could also prepend whatever they wanted to the source returned by load for the application entry point (the only resolved module whose parentURL is undefined). Or put another way, I don’t know what globalPreload provides for us that can’t be achieved in better ways via other APIs.

Of course, I’m inviting everyone to now go ahead and prove me wrong, so feel free. 😄 I think if we do need to keep globalPreload for some reason, in the near term I think we have to at least revert nodejs/node#39240 at least temporarily to get the off-thread PR finished and move the overall Loaders API to stable; but we would mark globalPreload specifically as still experimental, as we continue to investigate its use cases and what extra features it needs like communication channels.

cc @nodejs/loaders @JakobJingleheimer @jkrems @bmeck @giltayar

[giltayar]

I currently don't use port because the loader works "on thread", and so I can communicate via global variables.

But I already changed what needs to change to support "off thread" work, and you can see the globalPreload code which uses port here: https://github.com/testdouble/quibble/blob/support-off-thread-loaders/lib/quibble.mjs#L128

So, yes: globalPreload with port is necessary exactly for when loaders become "off thread".

[GeoffreyBooth]

@giltayar Would you mind reducing https://github.com/testdouble/quibble/blob/support-off-thread-loaders/lib/quibble.mjs#L128 to a minimal unit test of the port feature? That has as little else as possible; just a unit test of port. I’d like a test in the Node codebase for port in isolation, basically the inverse of https://github.com/nodejs/node/blob/main/test/fixtures/es-module-loaders/mock-loader.mjs which is a test of a mocking reference implementation.

[giltayar]

@GeoffreyBooth Would love to (assuming it's still relevant), but it'll take me a week or two because I gotta finish writing this talk that I'm supposed to present in two weeks.

[giltayar]

In other words, and if I was not clear: if you move loaders off thread, and don't give quibble/testdouble some kind of channel to communicate with the main thread, then it cannot be made to work "off thread".

[arcanis]

There's some precedent for having a single message channel that all consumers share and filter as needed (that's how iframes and workers behave, through postmessage)

[GeoffreyBooth]

--import would work (assuming it should run per thread, and I think it should), but:

1. I think it is not good DX to now require two options to make this kind of thing work, and.
2. You still need to find some way to communicate via a port.

Just so we don’t skip over the question, though, does globalPreload provide anything that --import or --require don’t other than the communication channel? Regardless of whether we think whatever non-globalPreload option is better DX, what functionality would be lost if we removed the globalPreload hook entirely and instead provided some other way of establishing a communication channel? I’m not saying that --import plus who-knows-what should be the way that that’s done; it’s just a hypothetical to tease out what value we’re getting out of globalPreload. Once I know what functionality we need to preserve, then we can discuss what API is the best way to achieve it, both from a DX perspective and a maintainability perspective (which might very well be globalPreload, but we need to sketch out all our options to be sure).

[giltayar]

@GeoffreyBooth all I need is a bidirectional communication channel to each user thread (or, as @arcanis said), a shared one for all user threads. I'm guessing I could differentiate based on thread id.

[GeoffreyBooth]

@GeoffreyBooth all I need is a bidirectional communication channel to each user thread (or, as @arcanis said), a shared one for all user threads. I’m guessing I could differentiate based on thread id.

That’s fine. What I’m getting at is that I don’t know if the current design is the most sensible one. Like even if we want sloppy mode scripts injected into each thread, maybe that’s not also the best place to define the communications channel. They don’t seem necessarily connected.

Another API we’ve been discussing is import { registerLoader } from 'module', to allow registering loaders from application code. Such loaders would only apply to modules imported after the loaders are registered, so it would be used something like await registerLoader('my-loader'); await import('application-entry-point.js'); but this would allow you to put any other application-side initialization code you wanted there, such as to define globals or register communications channels and so on. Again I’m not sure if this is better than any of the alternatives, but just an option for discussion.

[giltayar]

That’s fine. What I’m getting at is that I don’t know if the current design is the most sensible one.

I agree. Having to send over some code that is eval-ed in userland doesn't seem like it's the best API. One could imagine an additional function in the loader (getUserLandModule and please ignore the really bad name) that accepts a port and returns a module specifier for that loader that is loaded in userland (as opposed to the loader thread), and which contains an exported function that it contains a port. E.g.

// loader.mjs

function getUserLandModule(portThatCommunicatesWithTheUserLandModuleReturned) {
  // save the port somewhere

  return './user-land-part-of-loader.mjs';
}

// user-land-part-of-loader.mjs

function init(portThatCommunicatesWithLoader) {
 // save the port somewhere
}

[bmeck]

To an extent yes userland loaders could use this, but things like multi-thread coordination and head of line blocking would be pretty rough. There is a test for this exact kind of mocking in core already at https://github.com/nodejs/node/blob/main/test/fixtures/es-module-loaders/mock-loader.mjs that relies on the message channel. It was undocumented when implemented as it isn’t something that gives any benefits unless loaders are moved to their own thread.

[GeoffreyBooth]

Yes, this is the test I’m struggling to get to pass. I think I need to refactor it to remove setImportMetaCallback and the parts that depend on import.meta, which are pretty central to the test so I might have to remove it completely and it gets re-added in a follow-up PR.

This appears to be the only test in the codebase that tests globalPreload port, but it simultaneously tests a lot of other things. What would really help is a much simpler test that tests port by itself in isolation. cc @giltayar

[bmeck]

Sloppy mode was as @ljharb stated. Polyfills. It is a hook to modify the runtime not for module loading

…

On Wed, Dec 28, 2022, 7:26 PM Geoffrey Booth ***@***.***> wrote: Access to a bidirectional port to communicate with the worker thread. There’s one more thing: globalPreload needs to run *for every worker thread spawned by the user*. Because the loader thread needs to communicate with all of them. Let’s step back for a second, though. When globalPreload was introduced in nodejs/node#32068 <nodejs/node#32068> in March 2020, it didn’t have a communications channel. The port helper was added in nodejs/node#39240 <nodejs/node#39240> in July 2021. So is it fair to say that the *original* globalPreload didn’t do anything that --import can do instead? (Leaving aside the sloppy mode consideration, since we can’t seem to pin down why that was desired; I also feel like it should be doable within --import, either through vm like @jkrems <https://github.com/jkrems> suggests or maybe by ESM code importing a CommonJS file.) Because if --import can do everything that the injected sloppy mode script can do, it begs the question whether an injected script is the best way to provide a hook to a communications channel. Maybe we could leave --import for the “inject code before user application code” responsibility and create a new API specifically to provide a communications channel between the loaders thread and the main thread. What would be the downside of doing so? As for any of this running for each worker thread spawned by the user, that’s a whole other ball of complexity that I’m not sure we’ve ever tested. Arguably we might want a way to run --import for each worker thread, for example to add instrumentation or polyfills to each worker thread, beyond just the use case of loader thread communication. — Reply to this email directly, view it on GitHub <#124 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJI26WWUWW6MABRCOVI3WPTSD7ANCNFSM6AAAAAATLAAVJU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bmeck]

ServiceWorker is closer to loaders than that. They do not do broadcast. 1 to 1 communication is the norm there. Multi response is an issue

…

On Thu, Dec 29, 2022, 7:25 AM Maël Nison ***@***.***> wrote: There's some precedent for having a single message channel that all consumers share and filter as needed (that's how iframes and workers behave, through postmessage) — Reply to this email directly, view it on GitHub <#124 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJIZCD2RYA34YUU75JA3WPWGKZANCNFSM6AAAAAATLAAVJU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bmeck]

Sloppy mode and guarantee of execution on first tick.

…

On Thu, Dec 29, 2022, 4:03 PM Geoffrey Booth ***@***.***> wrote: --import would work (assuming it should run per thread, and I think it should), but: 1. I think it is not good DX to now require *two* options to make this kind of thing work, and. 2. You still need to find some way to communicate via a port. Just so we don’t skip over the question, though, *does* globalPreload provide anything that --import doesn’t other than the communication channel? Regardless of whether we think whatever non-globalPreload option is better UX, what would be lost if we removed the globalPreload hook entirely and instead provided some other way of establishing a communication channel? I’m not saying that --import plus who-knows-what *should* be the way that that’s done; it’s just a hypothetical to tease out what value we’re getting out of globalPreload. — Reply to this email directly, view it on GitHub <#124 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJI3J4XYPTDSTB2GCIYTWPYDD5ANCNFSM6AAAAAATLAAVJU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[GeoffreyBooth]

I think --require provides both of those?

[bmeck]

Yep but requires disk unlike import due to using paths

…

On Thu, Dec 29, 2022, 4:58 PM Geoffrey Booth ***@***.***> wrote: I think --require provides both of those? — Reply to this email directly, view it on GitHub <#124 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJIYVHXRVF3JPEH4MNO3WPYJQ5ANCNFSM6AAAAAATLAAVJU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Flarna]

We could add a possibility to register loaders programmatically in a --import or --require script to avoid the need that users have to provide two command line arguments to have the globalPreload part and the loader part in place.

[GeoffreyBooth]

Yes, this is what I was referring to when I mentioned import { registerLoader } from 'module' somewhere. The potential issue with such an API is dependencies that might call it, which is an issue we already have with require.extensions and so maybe it isn’t a concern.

[GeoffreyBooth]

Once we finish shaking out the bugs from moving loaders off the main thread, I think the resolve and load hooks will probably be ready to be marked stable (with the exception of context.importAssertions within each hook). For globalPreload, though, I would like to do a redesign probably along the lines of #124 (reply in thread), where the “sloppy script” aspect goes away and all that the API does is provide a communication port between application code and a particular loader, similar to https://nodejs.org/api/worker_threads.html#class-messagechannel. What do people think of this?