Squashed and rebased to pull in the new hooks (format, source). It's not broken right now (builds & URL resolve in hook works) but the new hooks haven't been fully ported. Hopefully I'll make some progress on that tonight.

My main early feedback on this would be that if we do this, we should allow environment attenuation through an attenuation hook, since currently loaders are able to do attenuation themselves since they are in the same context.

For example, instrumenting globals, etc.

One API for this might be something like -

export function environmentInit () {
  return `
    Object.customProperty = function () { console.log('custom') };
  `;
}

where the loader basically returns a string to evaluate in the target environment before any modules are loaded.

If we don't do something like this loaders that want this would need to have eg an import that is always added for every single module to get the same feature, which would be an unfortunate workaround to force I think.

That feels like it overlaps with --require for doing environment setup. What would be the use case here? Would something along the lines getPreloadModuleURLs(): string[] work as well for this?

P.S.: I'm leaning towards "prepare the execution context" (--require) and "provide hooks into resource resolution and fetching" (--loader) being two separate concerns. It does feel awkward if there's an indirect --require through the loader hooks as well. But that's just a -0, no strong opinion on my side.

Allowing loaders to specify their own modules to preload is a viable alternative, yes.

Would this extend to other aspects as well? E.g. should a loader be able to set a policy manifest or influence other restrictions traditionally set up using existing CLI flags?

E.g. should a loader be able to set a policy manifest or influence other restrictions traditionally set up using existing CLI flags?

I do think loaders are a good place to direct further security mechanisms. Better policy integration would be nice to see too.

Alright, basic getFormat hook is working again. There's now a "remote object" API for making the RPCs which makes it a bit easier to switch local (default) implementations in and out imo.

As is I was moving to the remote object pattern, I also made some small adjustments to the protocol. It now matches the v8 inspector naming ({ id, method, params } => { id, error, result }) which at least to me made it easier. It was already very similar and this way we have documentation built-in.

I think there are a lot of missing hooks right now we can discuss in other PRs that could even block this PR. This PR however seems complex enough already without adding more hooks to it. Does that seem fine to split up that work @guybedford ?

I think there are a lot of missing hooks right now we can discuss in other PRs that could even block this PR. This PR however seems complex enough already without adding more hooks to it.

To clarify: The hooks I mentioned are hooks that already had landed on master (format & source). There's tests for them that are now broken post-rebase because those hooks need to be wired up to work.

I see now that this was about the additional hooks for env setup. Sorry, shouldn't comment before waking up. :)

There's two failing tests left that both are regressions in error output:

make -j72 && ./node --experimental-loader ./test/fixtures/es-module-loaders/syntax-error.mjs test/message/esm_loader_syntax_error.mjs

make -j72 && ./node --experimental-loader i-dont-exist test/message/esm_loader_not_found.mjs

The error messages are reported properly but the stack is much more verbose and at the same time doesn't contain the proper source quote. Example:

- file://*/test/fixtures/es-module-loaders/syntax-error.mjs:2
- await async () => 0;
- ^^^^^
- 
+ events.js:298
+      throw er; // Unhandled 'error' event
+       ^
  SyntaxError: Unexpected reserved word

Added a call to decorate the errors which doesn't fix that the stacks are super verbose but at least the helpful source snippet is back.

I feel that dynamicInstantiate removal should happen in a separate PR that could potentially be merged faster. I suggest this on the expectation this PR will get merged but will take some time. Removing dynamicInstantiate sooner will minimize the number of loaders which ever use that feature.

I also think dynamicInstantiate removal deserves it's own entry in the changelogs so I assume this means separate commit?

@coreyfarrell a separate PR would be doable, but it likely should get a docs deprecation as we don't have a full replacement for some behavior of attenuating globals in the same way.

@bmeck I'm not sure what you mean by attenuating globals? I think docs deprecation of dynamicInstantiate would be better than nothing but if we know for a fact that it's going to be removed and not replaced then shouldn't we remove ASAP?

I had looked into a docs deprecation and afaict that already happened. At least the master markdown has a prominent warning: https://github.com/nodejs/node/blob/master/doc/api/esm.md#dynamicinstantiate-hook. The only adjustment may be to change "may disappear" to "will disappear".

@bmeck Another way of reading your comment: We should add a new API for attenuating globals asap to unblock removing dynamicInstantiate?

FWIW my personal take on this is: There's already --require. If people want to experiment with global attenuation and loaders, they can always set both the --experimental-loader and the --require flag. I don't think that adding a new "preload code" hook has to block removing dynamicInstantiate.

@jkrems I'm somewhat in agreement as long as we can agree that using both is a stopgap that needs to be filled eventually

The only adjustment may be to change "may disappear" to "will disappear".

I was thinking of the docs deprecation as entirely removing this section from the docs, to make dynamicInstantiate undocumented until it was removed from the code.

@GeoffreyBooth I think changing the situation for when no loaders are present does make a difference, yes. If it’s not enabled by default, I am okay with a noticeable performance impact for when it is enabled.

Also, just noticing this now, but this seems to be missing documentation entirely. At the very least, it seems to make specifying the loader option for a given Worker thread impossible, so that should be reflected in documentation in some way.

Currently in this PR we do not spin up a thread for the default loader if no user land loader is provided per https://github.com/nodejs/node/pull/31229/files#diff-6dda3ef5f860111452d324058e12a738R37

Currently in this PR we do not spin up a thread for the default loader if no user land loader is provided per #31229 (files)

Great! What about the use case of APM loaders running in production? That's the only use case I can think of where we might want a loader running in production, but it's a major use case. What are the performance implications there?

Or another way of looking at that, I guess, is what are the performance implications of such a loader under this PR as compared to the performance hit such a loader already entails in CommonJS?

Instead of having each loader on its own thread would it be worth considering having all loaders on a single thread? This way the performance concerns don't have to scale as badly with the number of loaders in the chain.

@guybedford the number of loaders and workloads vary performance, spinning up N number of threads all starting in parallel isn't a linear operation and CPU bound operations on each thread would be linear instead of parallel, the overall payloads passed between threads isn't incredibly high but is a good place to optimize. I'm unclear and actually doubtful that having all the loaders in the same thread is a good idea. It causes a head of line blocking point.

@bmeck it sounds to me like you're convincing yourself that for the standard use case of a simple resolver hook and some APM / mocking, each loader on its own thread and the associated context costs won't be worse with multiple threads.

The idea of many CPU cycles associated with loaders is terrifying. If people run complex transforms in loaders, it is not our job to work out how best to distribute that work across the CPUs, that is their job to work out through their own threading models.

@guybedford they can work out their own threading models, but spinning up a worker after the loader starts up is more costly than spinning up all the threads at the same start time. If they want to do everything in a single thread they can always just import the other loaders like you see with things like https://github.com/targos/multiloader does. Doing the inverse isn't the same since you have to wait on each step to spin up a new loader in a linear rather than parallel manner.

@bmeck this feel like big assumptions to me about the usage, when firstly we know nothing about the usage and secondly we know nothing about what would optimize any given usage.

Regardless of these arguments though, having a thread per loader in a multiple loader scenario will definitely affect the performance of the simple scenarios of low-compute hooks. Penalizing the base case based on heavy assumptions about the performance gains on other cases is quite a leap and I would be against such an assumption.

@guybedford the base case is unclear to some extent like you stated in the first part of your comment. The usage of eagerly allocating early rather than requiring extra startup time prior to spinning up another worker seems to allow your single threaded compute and allow for customized management by using import instead of a CLI flag to specify they want to keep a loader in the same thread.

@bmeck multiple loaders indicate a compositional model that is native to Node.js - I don't think expecting users to use a userland loader to achieve this same composition without incurring the cost of multiple threads is a solution here.

I could understand if the arguments were based on security or isolation here, but arguing that say 4 loaders (eg an http loader, mocking loader, coverage loader and resolver enhancing loader) requires 4 threads by default as a performance improvement seems rather odd.

My main concern is that getSource in one loader will serialize and send the source through all of the loaders incurring latency and serialization / deserialization costs entirely unnecessarily, when we should be able to handle this memory by reference.

Agreed, and yes the concerns do not affect this PR in its current form.

Note that this PR is not trying to allow for multiple loaders.

It seems to me that if we had to choose between chained loaders and loaders in a worker thread, we would prefer the former. Does anyone disagree? So maybe we should merge #33812 first after all, and this will need to be rebased/refactored to account for the addition of loader chaining, assuming we can figure out a way to use workers in a chained loaders setup.
cc @bmeck @devsnek

@GeoffreyBooth @bmeck Fwiw, I would like to be able to have an informed opinion about that, but I don’t really understand why we would bring Workers into this in the first place (and believe me, I am a big fan of Workers).

The PR description references “older design docs and discussions”, do you have a link to any of that?

I don’t really understand why we would bring Workers into this in the first place

@addaleax Please see nodejs/modules#351 (comment), I think @bmeck does a good job summarizing the reasons why there. See also the top of that thread for links to older design docs, this is probably the primary thread with most of the loaders discussion so far. There are a few other issues in https://github.com/nodejs/modules that discuss specific aspects of loaders, and #30986 has a lot of more recent discussion.

@GeoffreyBooth Yeah, I see 👍

In that case one thing that I’d really like to see from this PR is to not enforce the fact that all Worker threads in a hierarchy share a single loader thread. It should keep being possible, and probably even the default, to start Worker threads in a way that they don’t share the parent thread’s loader thread, but rather start their own one, imo.

@addaleax

can you clarify why the default wouldn't be to share the same loader thread? It is much more difficult to setup the shared state / that mimics how things like ServiceWorkers share handling requests from multiple sources.

also, one of the uses of loaders/audit hooks/etc. that we have talked about in various WG is to allow instrumentation of core entirely. I think it should be possible to prevent user code from escaping the constraints applied by those hooks. Does that match the constraints you have about allowing an escape hatch if a Worker needs to be spawned with a different loader?

@addaleax

can you clarify why the default wouldn't be to share the same loader thread?

Because Worker threads are, fundamentally, fully independent Node.js instances, except that they can communicate with each other. That invariant is broken here.

It is much more difficult to setup the shared state / that mimics how things like ServiceWorkers share handling requests from multiple sources.

I feel like this sentence if missing an “if we decide to do X”? Is this referring to Worker threads not sharing the same loader?

To be clear, the most important thing to me is that this is optional. Being able to spawn workers with an independent loader mechanism is a must for me. That being the default is the most sensible choice in my eyes, but also one that can be debated for sure.

also, one of the uses of loaders/audit hooks/etc. that we have talked about in various WG is to allow instrumentation of core entirely. I think it should be possible to prevent user code from escaping the constraints applied by those hooks. Does that match the constraints you have about allowing an escape hatch if a Worker needs to be spawned with a different loader?

I don’t really know what you are referring to here, but I’m also not sure how Workers differ from child processes in this respect?

Because Worker threads are, fundamentally, fully independent Node.js instances, except that they can communicate with each other. That invariant is broken here.

Can you explain where this invariant is documented. To my knowledge this isn't true given things like policies which went out of their way to intertwine the 2 isolates. Workers are unlike child processes as they are strictly unable to be independent due to having shared address space, environment variables, OS permissions, cannot have differentiated sandboxing. I think a design invariant might need to be ironed out and documented but I don't agree that they are completely separated nor that they could ever be.

I feel like this sentence if missing an “if we decide to do X”? Is this referring to Worker threads not sharing the same loader?

Yes, it would be difficult to share the same loader and/or part of the same loader chain if they are expected to create entirely new instances. It is also unclear why mandating a new instance is directly useful to me unless we want loaders to be stateful on a per context level and not on a group of contexts level. Given the progress of the Realms and Compartments APIs that seems a bit worrisome to myself at least since I would expect some capability of instrumenting those to affect loaders which would once again make them multi-context aware.

Keeping default isolation also goes along with the difficulties you see in things like in https://github.com/nodejs/tooling/issues which often cover issues of needing to share state with APIs so that supplemental instances such as child processes and workers do not go out of sync with modifications of the spawning instance.

Overall it feels there may be a need to write up a technical direction document on how to approach this kind of design space, but I am in no position or authority to do so and would not even know where to start except maybe asking the TSC to do so.

I don’t really know what you are referring to here, but I’m also not sure how Workers differ from child processes in this respect?

Workers and child processes are inherently different in the fact that workers cannot have differences such as address space, OS permissions, and sandboxing. Child processes are a much more discrete separation of the instances that do not need to deal with the multi-tenancy concerns that a worker must.

@bmeck Let me be a bit clearer: As Node.js instances, Worker threads are independent in the same way that processes are. Of course Worker threads are not fully independent as OS-level threads, but that’s a strawman argument – child processes are not fully independent either because they still share a file system, networking stack, etc.

In both cases, it makes sense to inherit the defaults of whatever parent instance spawned them, but if there can be flexibility, there should be.

that’s a strawman argument – child processes are not fully independent either because they still share a file system, networking stack, etc.

I do not believe so; I think you are carrying my argument to an extreme that any sharing is to be treated the same as undeniable sharing. Things like the effective user id do affect things like the filesystem and how it can be accessed, you can create a child process with a different user id for example. The limits of sharing are around at what the boundary sharing is being modeled, to me workers have a distinctly different boundary from processes which are distinct from containerized/sandboxed process groups and different from remote processes. I do not think we should treat all the boundaries as unable to be distinguished here as they do affect the deniable capabilities within each different type.

In both cases, it makes sense to inherit the defaults of whatever parent instance spawned them, but if there can be flexibility, there should be.

I don't believe that is entirely true but agree that flexibility is desirable; particularly, if you configure a specific setting with the mind that you are affecting the parent instance in a potentially constraining way such as a loader/permissions mode/policy configuration there can be some flexibility to escape this but it should be controlled and deniable. This falls in the same category to my viewpoint as not wanting a context which is denied fs access to simply spawn another context and by default get fs access (for example never allowing the fs module to be resolved).

I would note that, deniability doesn't necessarily need to be easy to do from my standpoint; however, it should be realistically feasible to do so without extreme effort.

I think you are carrying my argument to an extreme that any sharing is to be treated the same as undeniable sharing.

I’m not doing that, I’m just saying that you’re listing essentially things that Worker threads share that are essentially arbitrary and aren’t inherently relevant to the discussion about this particular PR.

I would note that, deniability doesn't necessarily need to be easy to do from my standpoint; however, it should be realistically feasible to do so without extreme effort.

Yes, I think we’re on the same page here. At the same time, I’m not convinced that loaders are a good choice for approaching deniability.

aren’t inherently relevant to the discussion about this particular PR.

I do think this is relevant since the discussion above was about not sharing by default and I see that as a strict downside as a default. So, I wanted to understand why it should be a default and this topic of what the potential intents/boundaries of workers came out of the statement about an invariant that I don't think is currently agreed upon; so I stated why I don't think that invariant is applicable as a whole.

Yes, I think we’re on the same page here. At the same time, I’m not convinced that loaders are a good choice for approaching deniability.

My main point for why loaders should have deniability is that if loaders need to be able to apply the same affects as policies so that policies can do things like instrument the results of loaders with integrity that leads towards a model where policies are just configuration of the default loader in my view.

Even if we are not talking about deniability via resolution, deniability by doing things like freezing parts of core is possible in loaders which allows emulation of --frozen-intrinsics while also keeping polyfilling as possible via getGlobalPreloadCode.

Perhaps discussing if loaders should be able to fully integrate with security features and/or have the same abilities as a policy feature for Node.js is pertinent here. I think if they are aiming towards having the same overall capabilities then they should take a stance of deniable escape hatches for sharing rather than no-sharing by default because that is how I would expect security minded (not necessarily enforcing) mechanisms to function by default.

I think from a deniability POV, it's about how easy it is to deny? E.g. a loader could intercept calls to the worker thread module and ensure that the program doesn't try to override the loader (or instrument a custom loader). But it does add another thing the loader would have to think of.

I would consider the ability to generally create child contexts with different loader hooks important. It would be unfortunate if the only way to do it would be spawning new node processes.

@jkrems I think we agree that we need the ability to spawn different worker contexts, but the question is about what defaults make sense for the intents of loaders and if there are other constraints. E.G. you cannot share a loader instance with a child process so that is out of scope, but for workers do they have the intent of acting as if they were child processes and if so what are the boundaries that differentiate them from processes.

E.G. you cannot share a loader instance with a child process so that is out of scope

EDIT: Ah, misread. Where does the idea of sharing loader instances with child processes come from? That feels like another topic unless I'm missing something.

Can you clarify this? It seems like the following should be possible, assuming the current context's loaders are in a thread:

// inherit hooks from current thread, default
new Worker('./x.mjs', { moduleLoaderHooks: true })

// disable hooks from current thread
new Worker('./x.mjs', { moduleLoaderHooks: false })

// provide custom hooks for the new thread, creates a new loader hook thread
new Worker('./x.mjs', { moduleLoaderHooks: ['ts-loader'] })

I think that the name worker_threads already implies the big difference: They are code that runs in the same address space and with the same process-level permissions. Whereas subprocesses get process-level isolation / are in a different security boundary. My impression was that this discussion was about what level of isolation that's possible and it sounded like there's already full agreement on the same default:

In both cases, it makes sense to inherit the defaults of whatever parent instance spawned them, but if there can be flexibility, there should be.

That sounded like @addaleax agrees with inheriting hooks from the parent. Which seemed like your preference as well. So the only thing I added was that it's still possible to deny this API, even if it's configurable. So I'm not sure where there's still disagreement..?

I think we just need to figure out the level of sharing that we achieve by default. We basically have 2 options and escape hatches would vary depending on which the default is:

• Application threads get their own Loader thread by default, allow some flag to share the existing loader with spawned child.
• Application threads share a Loader thread between each application thread by default, allow some flag to recreate the current loader with spawned child.

In both cases we also need to figure out an escape hatch given these comments to allow creating a new (unrelated?) loader in a Worker. We are not currently in the comments seeking to prevent a loader from manipulating/swapping the default of that escape hatch.

With #33812 this becomes more complex if each step of the chain is given its own thread vs not. I do think both these PRs are touching on a lack of consensus on a model for how we should be creating boundaries that would be helpful to iron out.