doc: clarify options for single customization hooks thread

[dygabo]

triggered by comment on #47747.
This PR adds clarification on single customization chain thread that were introduced with #52706

@nodejs/loaders

[JakobJingleheimer]

Thanks for this!

[GeoffreyBooth]

To me this reads as “the loader won’t apply to worker threads,” but what I assume you mean is “you don’t need to pass this argument when spawning a worker thread, as it will inherit from the main thread.” I’m not sure we need this addition at all; the other addition in module.md might be enough.

[dygabo]

indeed that was intended. What do you think about: "This option
has no effect on worker threads. Worker threads inherit the customization hooks chain configured on main thread."

I can also remove this addition. The idea for adding it here as well was to have all relevant entry points in the documentation covered. To make it clear so that the user expectation is correct.

[dygabo]

I updated the commit with the last proposal. Let me know what you think

[dygabo]

@GeoffreyBooth friendly ping, trying to move this one forward. Is the current version suitable for landing?

[GeoffreyBooth]

The more I read this, the more that I think it’s unnecessary. Now that we’ve fixed it so that registered hooks apply to all threads, I don’t think we need to call out that it does work as expected. Before the recent fix it would’ve been good to document this unexpected behavior, but we’ve fixed it.

[GeoffreyBooth]

“only has an effect on the main thread” sounds like the hooks don’t apply to worker threads. I just wouldn’t add anything here. I think the default assumption is that customization hooks apply everywhere, and thanks to the recent PR, now they do.

[dygabo]

ok, removed the change in cli.md now.

[dygabo]

The more I read this, the more that I think it’s unnecessary. Now that we’ve fixed it so that registered hooks apply to all threads, I don’t think we need to call out that it does work as expected. Before the recent fix it would’ve been good to document this unexpected behavior, but we’ve fixed it.

do you mean by that the whole PR is not needed? Because I think it sets expectentions for the user correct.ly. One might expect they create a new Worker and pass in execArgv an --import file.mjs that registers a customization hook and that this has effect on the Worker thread. It is nowhere documented otherwise without this patch. Same for --experimental-loader in fact. And even if it is how we currently implemented it and how we expect it to behave, I think we should document it for the users to avoid misunderstandings. wdyt? Is the behaviour of the customization hoooks explicitly documented anywhereelse?

[GeoffreyBooth]

One might expect they create a new Worker and pass in execArgv an --import file.mjs that registers a customization hook and that this has effect on the Worker thread.

There are probably lots of runtime flags that don’t apply to new Worker. I think it would be better to update our docs more generally to say either something nonspecific like “not all runtime flags apply to worker threads” or to clarify the full list of which ones do or don’t. It doesn’t make sense to single out these in particular, as --import is a very weird one to want to use (you’re spawning a particular file, so why would you want to have another file run beforehand?) and --experimental-loader is de facto deprecated and might get removed.

Alternatively, rather than documenting things, we could update the runtime behavior to help users avoid footguns. So if --import and/or --experimental-loader have no effect in new Worker, then we should throw an error when they’re passed. That would be more effective than documentation, and we wouldn’t even need to document this. It’s arguably a bug that Node just silently ignores these flags currently, or we could land the error as a semver-major change.

[dygabo]

then we should throw an error when they’re passed. That would be more effective than documentation...

we tried that as part of #52706 and we decided to take that up in an own follow-up as it is with current implementation of workers and other implementation details not easy either way (error or success of these operations). As long as this is not solved this behaviour imo should be documented. execArgv does have a list of options that will not work on Workers documented. This update states only that --import and --loader fall into the category of options that affect the process and not the thread. I don't want to argue too much about it as it's a minor thing but imo one that could save some debugging time for people running in this issue with different expectations until we have a runtime solution for it.

EDIT link to the relevant part of t he discussion on #52706

[Flarna]

Is it --import which has no effect or is it the module.register() call?

[dygabo]

strictly from code point of view it is the module.register() but in terms of expectations with worker execArgv (that this refers to) it is the combination of the two that does not have effect.

[Flarna]

In that case I find it a bit misplaced here as I would assume only very few users of workers actually use customization hooks. One might argue that setting process.title in an --import script does also not work so text here could get long over time.

I would prefer to improve the documentation at module.register (already done here) and in the Customization Hooks section. The Customization Hooks section has no example for workers.
Also the sample Communication with module customization hooks does not mention how to setup this from a worker (if it is possible at all).
In the Hooks section there I read The hooks thread may be terminated by the main thread at any time - which doesn't seem correct.

Also the behavior in case loader thread hangs or dies is not described.

Usually one would expect that workers are isolated but via customization hooks all of them are bound to the loader thread and via this one to main thread.

But not sure if it is your intention to get that far in this PR.

[GeoffreyBooth]

I think let’s just cut the addition to worker_threads.md and focus on module.md?

[GeoffreyBooth]

Suggested change

	resolution and loading behavior. Registering modules that export [hooks][] only
	has an effect on the main thread. All worker threads inherit the customization
	hooks chain of the main thread. See [Customization hooks][].
	resolution and loading behavior. These hooks are run in a new thread
	spawned for this purpose. After registration, the hooks will apply to any
	modules imported on the main thread or on any worker threads created
	by application code. See [Customization hooks][].

I know that for the main thread, obviously the hooks don’t apply to any modules already imported before the hooks are registered, and then they do apply to modules imported after registration; is it the same for worker threads? So if I have code that creates a new worker thread, then registers hooks, and then code within the worker thread does import(), will that last import() be customized?

And what happens when you call register from a worker thread? If it just silently does nothing, we should probably change that to have it throw an error. I think it should either work as intended (adding new hooks to the customization chains) from wherever it’s called, or it should error when called from places where it doesn’t do its job.

[GeoffreyBooth]

I think let’s just cut the addition to worker_threads.md and focus on module.md?

[mcollina]

I'm actually not on board with this change. I think the loaders should be working within worker threads themselves. This change was not noted in #52706 and should be fixed, not documented. Note that this is actually hanging the process, as shown in #53182.

I'll also open a revert PR to discuss.

[dygabo]

hi @mcollina ,
fwiw, this was discussed on the original PR and it was decided to implement a solution in a followup. The loaders work on the worker threads. The configuration of the loader hooks (or customization hooks) is done once per process for the main thread but the hooks are active for all user threads that get spawned after the call to module.register. This PR tries to clarify this for the users running in this issue until it gets properly solved.

[mcollina]

I think we should actually revert that change, or at least revert it in v22 until that follow-up PR is done. #53183.

[GeoffreyBooth]

I’m actually not on board with this change. I think the loaders should be working within worker threads themselves.

As in, you think that users should need to define the hooks that should apply to worker threads, rather than all threads inheriting the hooks registered for the overall process? While per-thread hooks may be useful in certain scenarios, consider the default use case of an application developer writing their app in TypeScript, where that app uses worker threads. They should be able to launch their app via node --import tsx app.ts and spawn workers via new Worker("./worker.ts"); they shouldn’t need to write new Worker("./worker.ts", { execArgv: [ "--import", "tsx" ] }). Needing to do the latter means that the user can’t write portable code; it’s explicitly tied to Node command-line options. Even if someday they want to change their transpilation library, they would need to know to update it not just in the command that launches their app but also in all the places in their code where they call new Worker.

[mcollina]

I think that #52706 breaks (at least) Angular and Ava, and the PR skipped a few tests to pass CI. I'm not on board with that implementation. I think "reusing" a worker should be supported, but that PR was not ready to land.