worker: Add experimental SynchronousWorker implementation

[jasnell]

Adds SynchronousWorker from the standalone synchronous-worker module directly into Node.js (as Experimental).

The synchronous-worker module was originally written by Anna Henningsen and is incorporated here at Matteo Collina's request and Anna's permission.

This is near verbatim to the original module with necessary adaptations to match Node.js style.

Signed-off-by: James M Snell [email protected]

/cc @addaleax @mcollina

[nodejs-github-bot]

Review requested:

• @nodejs/tsc

[mcollina]

LGTM

There is a tough bug in the original code.. but having it in core will allow us to actually fix them.

[RaisinTen]

There is a tough bug in the original code.. but having it in core will allow us to actually fix them.

@mcollina are you referring to addaleax/synchronous-worker#8?

[mcollina]

Yes indeed.

[legendecas]

Is there an explanation for this module/feature to be maintained in the core rather than a userland module? Is it possible for us to incorporate the patches mentioned in addaleax/synchronous-worker#8 into the core?

[GeoffreyBooth]

I would like to land this feature, but before we add it I think it needs a few things:

• It needs to work with built-in fetch. The primary example of the API uses fetch (albeit node-fetch because internal doesn’t work for some reason) and I can see lots of users trying to use this with our fetch and thinking that something is broken (which it is).
• We should update the docs to replace node-fetch with our fetch.
• We need ESM versions of the examples in the docs.
• The feature itself needs to work in ESM, and have tests proving that it does. (I’m assuming that it does already, but without tests I’m unsure.) We don’t need a copy of the full 265-line test file ported into ESM, but at least core functionality should be in both module systems since this seems to be a feature that might be module system-dependent. We might want to consider making the primary test file in ESM and have a CommonJS version include just core/basic functionality and functionality that might differ in CommonJS.

Along the lines of the above, I can see a huge number of people using this feature as a way to synchronously import ESM packages into CommonJS; like the folks frustrated that the latest, ESM-only version of node-fetch doesn’t work in CommonJS without dynamic import(). We should consider what this would mean: would such an outcome be bad? (I feel like it would, but why, exactly?) Should we mention it in the documentation, either with a recommendation or with a warning (like to discourage its use)? If we encourage the pattern, we should probably have tests around it.

I understand some folks might respond to this to-do list with “but it’s experimental! Just land it and we’ll iterate!” but I feel like there could be some big unintended consequences to enabling the syncification of async code, and we need to be a bit wary before putting such an ability into core. Again I’m 100% on board with doing so, and I can be persuaded that some of my suggestions above are unnecessary, but I think we should be careful with this one.

[jasnell]

I definitely agree that those are things that should happen before it comes out of experimental but disagree that they should block this landing initially being the experimental flag. We've allowed multiple experimental features to land while still being incomplete. I wanted to keep this initial pr focused first on getting the basic functionality added and spend subsequent PRs iterating. The fetch issue, for instance, is one that I would rather work on in a separate PR as a distinct piece of work.

I understand some folks might respond to this to-do list with “but it’s experimental! Just land it and we’ll iterate!” but I feel like there could be some big unintended consequences to enabling the syncification of async code, and we need to be a bit wary before putting such an ability into core.

That's a fundamentally different concern. Let's not use an unrelated set of TODO items as an excuse to block when the issue is the concern over adding deasync functionality in general. If that's where the concern lies, then let's discuss that specifically

[legendecas]

@jasnell @mcollina My concern is specifically about the constructor options invalidating the core pattern SynchronousWorker provides.

When sharedEventLoop is true, both runLoop and runLoopUntilPromiseResolved become unavailable. IIUC, the basic requirement of using SynchronousWorker is to run asynchronous tasks in a synchronous pattern. I would find creating new "node" environments and running the codes asynchronously a separate issue from exposing the ability of SynchronousWorker provides, as its name suggests.

I would love to land this feature to explore the ability to run asynchronous tasks synchronously, but I would also like to distinguish those distinct use cases. To be clear, I'm totally onboarded with those needs. For creating disposable node environments, I would find it the goal of the environments and realms refactoring to expose a full set of Node.js built-in modules in the vm.Context (probably vm.NodeContext for compatibility, nvm, not settled yet).

[Qard]

Is there a reason to make this a worker_threads thing when it's not threading rather than adding a way to run a vm.Module synchronously? Seems to me like building this sort of behaviour into the vm module makes it a bit more flexible. 🤔

[GeoffreyBooth]

Let’s not use an unrelated set of TODO items as an excuse to block when the issue is the concern over adding deasync functionality in general.

I’m not concerned about adding deasync functionality. As I wrote, I want to ship this feature.

Most of my list were simply questions, that haven’t yet been answered. I’ll repeat them here:

• Does the feature work when run from ESM user code?
• How is this feature used in ESM? I see createRequire as part of the API: does that work in ESM, and if so, how? Is import.meta.url supported?
• Can this be used to run an ESM file? Is there an import equivalent to createRequire, or does the function returned by createRequire support ESM code?
• Can this feature be used to synchronously import an ESM library like node-fetch into a CommonJS context?
• If so, is that something we want to discourage? Why or why not?

This feature might be able to solve a pressing user desire: being able to import ESM-only packages like node-fetch synchronously into CommonJS contexts. If this feature can do that, and I’m assuming that it can because that’s what your example seems to show, then it might become immediately very popular. That would create its own issues as then we’d feel pressure to never remove it, even if we can’t solve other pieces like how to get it working with full equivalency in ESM (if that’s even an issue). I don’t want a repeat of async_hooks where we have something that works in CommonJS but maybe kinda doesn’t really work in ESM but we can’t remove the API or figure out how to fix it in ESM. At this point, we shouldn’t be releasing features that don’t work (or don’t work fully equivalently) in ESM. That’s not fair to the modules team, who then feel the pressure to fix features like async_hooks or vm or continue to suffer user complaints that they can’t migrate to ESM until such-and-such feature works in ESM. And it’s not fair to our users to release a broken feature; even for experimental features, we should have minimum standards of quality. We don’t release experimental features without tests; ESM support should be part of our standard.

So let’s at least answer all the above questions. If we can’t fix all those issues in one PR, which I understand, perhaps we could require a build flag for the node binary to include or enable this feature. Then development could continue in subsequent PRs without this also being available publicly (at least not easily). This also already exists as a standalone repo; perhaps some of the shortcomings and bugs can be addressed there before this is proposed to be merged into core.

[mcollina]

Does the feature work when run from ESM user code?

From my tests, it works.

How is this feature used in ESM? I see createRequire as part of the API: does that work in ESM, and if so, how? Is import.meta.url supported?

It requires a tiny patch to support ESM. See mcollina/worker@5d38649.
@jasnell could you pull that in?

Can this be used to run an ESM file? Is there an import equivalent to createRequire, or does the function returned by createRequire support ESM code?

Not at this time. As you can see in the commit above, you can use a commonjs file to dynamically import. It's definitely within the realm of the possibilities to add that functionality.

Can this feature be used to synchronously import an ESM library like node-fetch into a CommonJS context?

I do not have a good answer for this right now. I think it could be done with some limitations. The loaded module needs to have separate loops to remove the async nature, however for something like node-fetch to work it needs to run on the main event loop. In other terms, it's unlikely to work right now for complex use cases (like node-fetch). Long term, I believe this feature could enable us to have some custom synchronous import with a few notable caveats.

We don’t release experimental features without tests; ESM support should be part of our standard.

My primary use case of this feature is to have hot module reloading in a pure ESM context. Barred bugs, it's incredible DX.

So let’s at least answer all the above questions. If we can’t fix all those issues in one PR, which I understand, perhaps we could require a build flag for the node binary to include or enable this feature. Then development could continue in subsequent PRs without this also being available publicly (at least not easily). This also already exists as a standalone repo; perhaps some of the shortcomings and bugs can be addressed there before this is proposed to be merged into core.

I'm not sure it's possible. Every time I tried to fix any of the mentioned shortcomings I stumbled upon Node core code that I will have to change to make that possible.

[jasnell]

@GeoffreyBooth:

Most of my list were simply questions, that haven’t yet been answered. I’ll repeat them here:

• Does the feature work when run from ESM user code?
• How is this feature used in ESM? I see createRequire as part of the API: does that work in ESM, and if so, how? Is import.meta.url supported?
• Can this be used to run an ESM file? Is there an import equivalent to createRequire, or does the function returned by createRequire support ESM code?
• Can this feature be used to synchronously import an ESM library like node-fetch into a CommonJS context?
• If so, is that something we want to discourage? Why or why not?

There is zero intent to exclude ESM support from this feature, but I propose that we defer answering these questions to a subsequent PR. This initial PR is not intended to address all of the bugs that may exist in this feature currently.

Does the feature work when run from ESM user code?

Yes.

How is this feature used in ESM? I see createRequire as part of the API: does that work in ESM, and if so, how? Is import.meta.url supported?

I'll need you to be more specific with the first part of this question. createRequire() does work in ESM. I do not yet know if import.meta.url is supported but there is no intention to not support it. I propose we deal with that question in a separate follow on PR.

Can this be used to run an ESM file? Is there an import equivalent to createRequire, or does the function returned by createRequire support ESM code?

As Matteo indicates, not currently. It creates a require() function with the same expected semantics as require(). We can continue iterating on the mechanism to allow better support for ESM but, again, I'd rather do that in a separate PR as it is likely to be non-trivial on it's own.

Can this feature be used to synchronously import an ESM library like node-fetch into a CommonJS context?

Yes, but currently only indirectly and with limitations. For instance, in the following example from the tests, we use createRequire() to acquire a CJS that exports an async function that uses dynamic import.

const w = new SynchronousWorker();
const req = w.createRequire(__filename);
const fixture = fixtures.path('es-modules', 'message.js');
const message = w.runLoopUntilPromiseResolved(req(fixture)());
strictEqual(message, 'A message');

This could likely be improved, yes, but the intent of this PR is not to make API improvements. My intent is for that to happen in subsequent PRs.

If so, is that something we want to discourage? Why or why not?

This PR does not seek to encourage or discourage anything for any reason.

[jasnell]

@legendecas:

When sharedEventLoop is true, both runLoop and runLoopUntilPromiseResolved become unavailable. IIUC, the basic requirement of using SynchronousWorker is to run asynchronous tasks in a synchronous pattern. I would find creating new "node" environments and running the codes asynchronously a separate issue from exposing the ability of SynchronousWorker provides, as its name suggests.

I would love to land this feature to explore the ability to run asynchronous tasks synchronously, but I would also like to distinguish those distinct use cases. To be clear, I'm totally onboarded with those needs. For creating disposable node environments, I would find it the goal of the environments and realms refactoring to expose a full set of Node.js built-in modules in the vm.Context (probably vm.NodeContext for compatibility, nvm, not settled yet).

Unfortunately I don't know how to translate this into actionable changes. I understand what your concerns are but I do not know what concrete changes you are wanting to see made. Is it just renaming the class? Moving it off of worker_threads? Removing or refactoring the config options? Having multiple classes for different purposes?

[GeoffreyBooth]

There is zero intent to exclude ESM support from this feature, but I propose that we defer answering these questions to a subsequent PR. This initial PR is not intended to address all of the bugs that may exist in this feature currently.

I’m just going to respond to this by itself for now, since there are a lot of questions zooming around here right now. In short, I don’t think it’s okay to land a feature, even as experimental, that defers full ESM support until later. Not only may later never come, but as soon as a feature is accessible to users they’ll start opening bug reports against it, and it’s the entire team’s responsibility to address those. We incur a maintenance burden as soon as the public has access to a feature.

I sympathize with the desire to split up the work into several PRs. I can think of a few ways to enable that while still leaving this feature inaccessible from the public:

• We could land this but mark it as don’t-backport for all release lines. It would stay on main but never get released. This might cause backport issues for other PRs that edit any of the same files.
• We could gate this feature behind a build flag, where the feature is inaccessible in the node binaries we build for release but local core developers could build the node binary with it and continue developing on it.
• We could follow the pattern of https://github.com/nodejs/ecmascript-modules, where we create a fork of the node repo and this feature is developed over there over the course of many PRs, and then one big PR is opened against this repo when the feature is ready.

[jasnell]

We could land this but mark it as don’t-backport for all release lines. It would stay on main but never get released. This might cause backport issues for other PRs that edit any of the same files.

I'm perfectly fine with don't land labels for the time being.

I work with @mcollina on determining a path forward here.

[GeoffreyBooth]

Can this be used to run an ESM file? Is there an import equivalent to createRequire, or does the function returned by createRequire support ESM code?

Not at this time. As you can see in the commit above, you can use a commonjs file to dynamically import. It’s definitely within the realm of the possibilities to add that functionality.

Could we change createRequire to createImport and then it can accept either a CommonJS or an ESM file? And under the hood if it’s given an ESM file it could create a CommonJS wrapper like you described, of a dynamic import of the ESM file; or just handle the ESM file directly if that’s not too hard. (This could happen in a follow-up, but it feels like something that should be addressed now because this feels like a fundamental API question and this is the PR that creates the API.)

How is this feature used in ESM? I see createRequire as part of the API: does that work in ESM, and if so, how? Is import.meta.url supported?

I’ll need you to be more specific with the first part of this question. createRequire() does work in ESM. I do not yet know if import.meta.url is supported but there is no intention to not support it.

The existing module.createRequire accepts either a file path or a URL; the example in the docs shows createRequire(import.meta.url). I think we should support file URLs as input to this createRequire and have a test around it. (I don’t think this necessarily needs to be part of this initial PR, but it feels to me like it should be. It shouldn’t be too hard to detect a URL and call urlToFilePath as needed, probably just copying whatever the other createRequire does in its implementation.)

Can this feature be used to synchronously import an ESM library like node-fetch into a CommonJS context?

I do not have a good answer for this right now. I think it could be done with some limitations.

The examples in the docs of this PR show it working with node-fetch. Are you saying that those examples don’t actually work, or that they do but not every ESM-only package would necessarily work?

My primary use case of this feature is to have hot module reloading in a pure ESM context. Barred bugs, it’s incredible DX.

If this is already working, can we get some tests added for this use case? And some tests for the basic functionality running in an ESM context, since I think you’re saying that that’s also already working. (This feels like something that could and should be part of this initial PR.)

---

How do you feel about this list of requirements?

For merging in at all, even with the dont-land flags:

• Add emitExperimentalWarning somewhere so that it prints the warning.
• Add a test for the ESM hot module reload that’s already working.
• Add a test for basic functionality of the feature in an ESM context. This could be a copy of the existing tests just converted to ESM, without the ones where we we’re unconcerned about ESM/CommonJS compatibility.
• Add ESM examples to the docs, for the ESM functionality that’s already working.
• Add text to the docs explaining known issues (ESM bugs/limitations, internal fetch not working, etc.).

For removing the dont-land flags and getting released as experimental:

• Resolve questions about naming, like should this live as part of node:vm etc.
• Get built-in fetch working, and update the examples to replace node-fetch with our fetch.
• Resolve all the questions about how to use this not just in ESM user code that calls this feature, but ESM code passed into the feature such as having it load ESM files to run. Like should we change createRequire to createImport, or have both, etc. ESM code should be supported first-class, not only within a CommonJS wrapper (that the user needs to see, at least).
• If this feature enables synchronously importing ESM libraries like node-fetch, we need a section in the docs explaining the downsides of doing so (assuming there are some), with recommendations. I don’t want people solving the “I can’t import node-fetch into my app” problem by copy-pasting the same snippet from StackOverflow into thousands of apps, if that snippet has a big issue like locking the main thread during sync await or something like that. We should get out ahead of this by covering both the use case and our recommendations in our docs, with our own snippet if we think it can be done safely, as that will help prevent wrong information from spreading too widely before we can try to start correcting it.

[jasnell]

-1 to changing createRequire(...) but I can look into adding an additional createImport(...) that implements import semantics. I don't think it's a good idea at all to try combining those semantics or trying to make it too intelligent. require() and import() are different things, let's keep them different things.

I'm also -1 on adding an experimental warning unless we also get rid of the experimental flag. We should have one or the other, there's no reason to have both.

Add a test for the ESM hot module reload that’s already working.

If @mcollina wants to provide such a test. I disagree that it should be a necessary requirement for this to land. It's a use of this API, yes, but it's not the only use of it, and we can demonstrate the api works without it.

I also still definitely disagree that this initial PR must have full ESM capability demonstrated up front to land but, ok, I'll explore that if it helps unjam it.

[GeoffreyBooth]

-1 to changing createRequire(...) but I can look into adding an additional createImport(...) that implements import semantics. I don’t think it’s a good idea at all to try combining those semantics or trying to make it too intelligent. require() and import() are different things, let’s keep them different things.

I’m fine with whatever solution that treats both module systems as first-class. This can also come later if it’s too big a change for this initial PR.

I’m also -1 on adding an experimental warning unless we also get rid of the experimental flag. We should have one or the other, there’s no reason to have both.

We have lots of experimental features with both a flag and a warning: --experimental-network-imports, --experimental-vm-modules, --experimental-wasm-modules, etc. We had --experimental-ecmascript-modules flag and warning both for years. Dropping the warning is a preliminary step before dropping the flag. We’ve generally had both the warning and the flag when the API has significant risk of change, which I think is the case here.

If @mcollina wants to provide such a test. I disagree that it should be a necessary requirement for this to land. It’s a use of this API, yes, but it’s not the only use of it, and we can demonstrate the api works without it.

If there isn’t a test, unrelated changes may break this ability. For example, once #44710 lands will SynchronousWorker-enabled hot module reload still work?

[jasnell]

After talking it over with @mcollina ... we've decided to go a different direction.

[mcollina]

This feature is mostly self-contained, expand the capability of Node.js and if successful will solve real problems people have today. It's not a major feature of Node.js in any way, but rather something minor.

The original module powers the CLI of MongoDB, and I've used it daily too. Given the production usage, it's ready enough to land with an experimental warning and no experimental flag.

James and myself talked briefly about this after I demoed my hot-reloading-esm solution. He volunteered to open a PR to bring Anna's work in as-is. Finishing this will likely require a collaborative effort. What this PR is about is an ask to iterate on this in core and incrementally get there. I see a lot of LGTMs and only a significant request for changes which will require quite a significant amount of additional work to get complete.

I cannot ask @jasnell to put much more time into brining this over the finishing line, and at the same time I do not want to call a @nodejs/tsc vote for something that is not ready yet.

We are likely going to iterate on it and propose a different solution for the same problem.

[GeoffreyBooth]

I chatted with @mcollina about this. The question of how much compatibility with ESM is reasonable to require of experimental features is one that I’m unaware of being discussed before, so I opened #45084 to try to get an answer on that. I’ll defer to whatever consensus that discussion arrives at with regard to that.

I think my list in #45018 (comment) for what this PR needs in order to land is reasonable. This feature has no tests for functionality that supposedly already works in ESM; we don’t usually merge in PRs with incomplete tests, even for experimental features. This feature has no documentation of how to use it in ESM; I’m unaware of other features that are undocumented in ESM. For a feature with so many known issues, I think it’s reasonable to have a docs section spelling out those issues and other caveats so that users avoid frustration and don’t open lots of issues complaining about them. These are all very standard requests for PRs to be able to land.

With regard to the second list, of what needs to be fixed in order for this to be released, I’m more flexible and I anticipate the list changing based on the state of this PR when it lands. If the feature is so broken that it doesn’t solve any user problems, it has little likelihood of becoming popular and therefore it doesn’t matter if it’s released or not. If it does solve the user problem of synchronously importing ESM libraries in CommonJS, then I think we need a higher-than-usual bar for releasing this, even as experimental. Especially since the original author is no longer involved, and @jasnell has limited time to work on this, this feels to me like a feature at unusually high risk of being merged in in an incomplete state and abandoned, or left without ESM parity. I want to avoid those outcomes; if we can find a way to do so that doesn’t involve using blocks, that would be my preference, because I do want to ship this feature. I feel like I’ve been direct about my concerns and I’m happy to work with this feature’s champions to get all issues addressed and to get this released.

[addaleax]

The original module powers the CLI of MongoDB.

Let me be clear that this is not true at all.

I very much worry about busy-waiting promise resolution being a massive footgun.

This is also not what this is. There is no busy loop here.

There is a tough bug in the original code.. but having it in core will allow us to actually fix them.

Fixing that bug properly requires significant changes to libuv's event loop management code. I'd be super glad to see that happen but it's going to be a bunch of work for somebody to pick up.

The reason why I need this module is because it's the most straightforward way to have hot-module-reloading for ESM.

It's probably a decent way to achieve this, but it's also a terrible sign if the vm.Module API doesn't enable the same use case using its existing API.

[liuxingbaoyu]

I would like to make a personal point of view from an ordinary user's point of view. 😃

I'm really looking forward to this as it solves a problem that has been bugging me all the time (at least for me).
Async is so terribly contagious that I've been leaning towards sync/cjs all the time.
This PR makes it possible to cooperate synchronously and asynchronously.

In addition to some of the real world examples already mentioned, here are a few more.

jestjs/jest#13495 (comment)
prettier v3 is migrating to async and jest doesn't want to introduce async for this.
Although worker_threads can be used, it is more cumbersome and consumes more resources.

prettier/prettier#13158 (comment)
Prettier also ran into some annoying issues when migrating to async.
Prettier is a CPU-intensive program, and most operations do not need to be asynchronous.
But since some dependent libraries are using async, the whole prettier has to use async as well.
This brought a terrible performance loss of up to 20%-30% at the beginning, and after some work, the performance loss decreased to an acceptable level of 0%-10%.

There are also some scenarios in babel, but those are less important, currently babel is using worker_threads.
https://github.com/babel/babel/blob/main/babel-worker.cjs#L6-L9
The only thing worth mentioning here, which is really annoying, I think it sucks to start a worker thread just for the color of the terminal output. (No need to do this in babel since the actual call is async, but I think I would run into this problem if I wanted to use chalk in a purely synchronous call)

So from my personal point of view, although this may make some things available in cjs but not available in esm, it's a lot less negative than the current use of esm in cjs is far behind using cjs in esm. This PR just closes the gap, not even much, as it will take countless years before it becomes stable and widely available in the real world.

So unless we want esm to completely replace cjs, the advantages of this PR outweigh the disadvantages.
In addition, it seems that esm is not stable enough, even if it is hoped that esm will completely replace cjs, it is a little early.
#25424
#43083
jestjs/jest#11956

[legendecas]

I chatted with @mcollina about the HMR requirements and the works on vm/ShadowRealm. As the scenario brought up by Matteo doesn't require the synthetic Node.js environment to be able to synchronously drain the asynchronous events, it would be more decent and intuitive to improve the existing APIs like builtin module vm and ShadowRealm.

The ability to synchronously import an es module in CJS is still worth to be explored. But it requires more meticulous thought to help people not shot themselves so easily on the wait-for-events-from-another-loop lock problem.

[GeoffreyBooth]

it would be more decent and intuitive to improve the existing APIs like builtin module vm and ShadowRealm.

Possibly related, https://nodejs.org/api/vm.html#class-vmsourcetextmodule has been experimental for quite a while. If I remember correctly it has various issues that needed addressing before it could become stable; and its status is a major blocker for tools like Jest.

[legendecas]

Possibly related, https://nodejs.org/api/vm.html#class-vmsourcetextmodule has been experimental for quite a while. If I remember correctly it has various issues that needed addressing before it could become stable; and its status is a major blocker for tools like Jest.

yeah, let's track them in #37648, #43899, and #42528 :)