Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

doc: clarify options for single customization hooks thread #53006

Closed
wants to merge 2 commits into from
Closed

doc: clarify options for single customization hooks thread #53006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2496644
doc: clarify options for single customization hooks thread
dygabo May 16, 2024
4de0ba9
remove doc update
dygabo May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion doc/api/module.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,9 @@ changes:
`initialize` hook.

Register a module that exports [hooks][] that customize Node.js module
resolution and loading behavior. See [Customization hooks][].
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][].
Comment on lines +116 to +118
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, removed the change in cli.md now.

Comment on lines +116 to +118
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


### `module.syncBuiltinESMExports()`

Expand Down
5 changes: 4 additions & 1 deletion doc/api/worker_threads.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1008,7 +1008,10 @@ changes:
* `execArgv` {string\[]} List of node CLI options passed to the worker.
V8 options (such as `--max-old-space-size`) and options that affect the
process (such as `--title`) are not supported. If set, this is provided
as [`process.execArgv`][] inside the worker. By default, options are
as [`process.execArgv`][] inside the worker. `--experimental-loader`
and `--import` with `module.register()` calls to contribute to the hooks
Copy link
Member

@Flarna Flarna May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@dygabo dygabo May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Flarna Flarna May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

chain do not have an effect on the worker threads. These options affect
only the configuration of the main thread. By default, options are
inherited from the parent thread.
* `stdin` {boolean} If this is set to `true`, then `worker.stdin`
provides a writable stream whose contents appear as `process.stdin`
Expand Down