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.

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

feature request: add calling thread ID to loader resolve() context #49472

Closed
cjihrig opened this issue Sep 3, 2023 · 6 comments
Closed

feature request: add calling thread ID to loader resolve() context #49472

cjihrig opened this issue Sep 3, 2023 · 6 comments
Labels
feature request Issues that request new features to be added to Node.js. loaders Issues and PRs related to ES module loaders

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Sep 3, 2023

What is the problem this feature will solve?

Some loaders, such as a mocking loader, should be aware of the thread initiating the operation. For example, it is common for ESM mocking to pass a list of export names to the loader thread, and later resolve those exports on an application thread. If a module is mocked from one application thread, and another thread tries to import the same module, it will be unaware of the export values. The export values themselves cannot be passed between threads because some values, such as function, do not serialize.

What is the feature you are proposing to solve the problem?

The resolve() loader hook already has a context object, which includes the parentURL. I am requesting that the thread ID also be included. If the thread ID were in the context, the resolve() hook could identify the proper mock (or real) implementation to use, depending on the thread.

What alternatives have you considered?

No response

@cjihrig cjihrig added the feature request Issues that request new features to be added to Node.js. label Sep 3, 2023
@github-project-automation github-project-automation bot moved this to Pending Triage in Node.js feature requests Sep 3, 2023
@cjihrig cjihrig added the loaders Issues and PRs related to ES module loaders label Sep 3, 2023
@MrJithil
Copy link
Member

Is this something we can start working? Or should we wait for enough opinions?

@bnoordhuis
Copy link
Member

#30061 (comment) may be relevant.

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 27, 2023

FWIW, I was referring to using worker.threadId here.

I would avoid starting any work until you get some input from the loaders team in case they push back for some reason.

@targos
Copy link
Member

targos commented Oct 2, 2023

I feel like there's a misunderstanding here (or I don't really understand the use case). If your application has one main thread and two worker threads, then there are three separate loader threads. With this feature, each loader would receive a different thread ID, but always the same one.

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 2, 2023

If your application has one main thread and two worker threads, then there are three separate loader threads.

Maybe there is a misunderstanding then. I was under the impression that once the loaders were moved off thread, there was a single loader thread for the entire application. If that is not the current state, I thought it was at least the goal state based on #47747 (comment).

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 13, 2023

Another data point from Slack:

when hooks are registered, node spawns a thread to run them in. when user code creates a thread, node spawns another thread for running hooks; there should just be one hooks thread that handles however many application threads there are

If we will eventually end up with a single hook thread, then it seems like it would be very useful to have the calling threadId for use cases like mocking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. loaders Issues and PRs related to ES module loaders
Projects
None yet
Development

No branches or pull requests

4 participants