Repository navigation
Loaders that use childProcess.fork lead to endless recursion of processes #47615
Description
Activity
Perhaps the solution is simply not to carry over loaders onto child processes that are forked within loaders.
Oh also, if such a solution were to be used, for chained loaders this only needs to be the last loader in the current chain that is removed. i.e. If we have
node --loader ./loader1.mjs --loader ./loader2.mjs ./some-file.mjsthen ifloader2.mjsuseschildProcess.fork, the forked process can have--loader ./loader1.mjsjust fine (as long as this continues inductively).This seems to me firmly a case of "just don't do that."
Reacted by Richard Simko and Tomáš HübelbauerThis seems to me firmly a case of "just don't do that."
Third party libraries, like
typescript, are out of my control. This change has actively broken the ability to importtypescriptwithin a loader at all (because TS uses child processes to split up work).Then you should work with the typescript people to resolve that. I don't think it's node's job to protect against fork bombs.
Reacted by Richard SimkoRunning your
test.mjsthrough esbuild-kit/tsx (run ts files, similar to ts-node ) straight up without forking a child process is crashing.
doingnpx tsx test.mjsand waiting will crash with a warning about custom ESM loaders. Similar to this issue.I tried adding
process.on('warning', e => console.warn(e.stack));to the program, and this (small part of the whole) error ->
FATAL ERROR: MarkCompactCollector: young object promotion failed Allocation failed - JavaScript heap out of memory
is created.This is like the error I came to the issues to find. This is only
v20.0.0. I was using a similar readline prompt, and creating a listener on stdin. I thought that creating the listener was going infinite and creating the memory leak, but my problem might be the same as this as tsx uses esbuild-kit/esm-loader to transform typescript to ESM.Given that I can do
node test.mjswithout crashing, it points to loaders.Reacted by Folke LemaitreI don't think it's node's job to protect against fork bombs.
Well in this case I don't think the behaviour even makes sense, the threaded implementation of loaders still has
execArgvset within the worker to include the loader:// loader.mjs // ["--loader", "./loader.mjs"] console.log(process.execArgv);
This means forked processes receive this by default in their
execArgv.But this doesn't make sense, the loader thread isn't loaded using
loader.mjs(otherwise any loader would be an immediate fork bomb), it's loaded using the next loader in the chain.That's changing the subject / moving the goal posts, isn't it? Your original bug report is about
child_process.fork()and I'm of the opinion it isn't a bug.Reacted by Richard SimkoThat's changing the subject / moving the goal posts, isn't it?
Well the problem comes from the fact that
childProcess.forkdefaults to usingprocess.execArgv.Ultimately I don't really care about what solutions are applied, but the fact that existing libraries are broken within a loader compared to previously is to me the problem here.
The reason I think changing
process.execArgvis a particularly good solution though is because from the perspective of the loader thread having["--loader", "./loader.mjs"]is simply wrong, the loader thread does not have such a loader applied. From it's point of view no loaders are applied yetprocess.execArgvcontains one.@nodejs/loaders
I can confirm it's not a duplicate of #47566, #47615 (comment) seems to be correct. I agree that not supporting this use case is tempting, but not very manageable indeed.
FWIW, as a workaround, you might be interested in sucrase as an alternative for transpile TS to JS that doesn't use forks.
I imagine that the problem would also happen if the loaders are set by
NODE_OPTIONS, not necessarily the CLI itself?Reacted by Antoine du HamelSo the obvious solution seems to be to eliminate loader info from
process.execArgv(and likewiseNODE_OPTIONS) passed into loaders, but surely that has side effects, I would assume? Like loaders would have no way of discovering what other loaders might have been registered? Which maybe isn’t such a big deal, as we could provide a specific API for that if there’s a use case for such.@GeoffreyBooth there is no problem to communicate with another loader in complex setup - nextResolve / nextLoad can be used as bi-directional channel (as an example, esmock client detects loader via this way, not by argv / NODE_OPTIONS parsing).
Like loaders would have no way of discovering what other loaders might have been registered?
Just to be clear the suggestion I am making here is that the active loader is removed from the list.
So if we have the command
node --loader ./loader1.mjs --loader ./loader2.mjs file.mjsthen theprocess.execArgvfor each would be as follows:file.mjs->["--loader", "./loader1.mjs", "--loader", "./loader2.mjs"]loader2.mjs->["--loader", "./loader1.mjs"]loader1.mjs->[]
Essentially from the perspective of each loader thread, their
process.execArgvis set such that they know the next loaders in the chain.Put another way, from the perspective of each thread here the list of loaders is simply the loaders the apply to the current thread.
Just to be clear the suggestion I am making here is that the active loader is removed from the list.
I think that assumes that the
forkcall would be made at the top-level of the loader, but that's not necessarily the case, and there are no real way of detecting which loader made theforkcall AFAIK.I think that assumes that the
forkcall would be made at the top-level of the loader, but that's not necessarily the case, and there are no real way of detecting which loader made theforkcall AFAIK.Are all loaders in one thread? How does this work if
loader2.mjsneeds resolution fromloader1.mjsand so forth?Loaders are loaded serially in the loader thread, so when
loader2.mjsis loading,loader1.mjsis already loaded and will intercept all theresolveandloadrequests. Note that ifloader2.mjscallsimport()outside of its top-level, it will go through its ownresolveandloadhooks, but that's not as bad as a fork bomb.This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 240 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.
Version
v20.0.0
Platform
Linux 5.19.0-38-generic #39~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Fri Mar 17 21:16:15 UTC 2 x86_64 x86_64 x86_64 GNU/LinuxSubsystem
loaders
What steps will reproduce the bug?
Use
childProcess.forkwithin a loader, for example in the following loader:And run
node --loader ./loader.mjs ./test.mjs.How often does it reproduce? Is there a required condition?
This happens consistently.
What is the expected behavior? Why is that the expected behavior?
The short answer is I'm not sure what should be done here. But it seems problematic that libraries (such as
typescript) that useschildProcess.forkcannot be used within a loader without endless recursion of processes.Perhaps the solution is simply not to carry over loaders onto child processes that are forked within loaders.
What do you see instead?
Loaders get continually created spamming the console with:
while also rapidly growing memory usage from the endless chain of processes.
Additional information
I noticed this because
ts-noderapidly memory leaks in Node 20. I experimented it a bit and found that TypeScript is callingchildProcess.forkwhich causes this error. i.e. A minimal loader that exhibits this behaviour would just be:Using this loader with
node --loader ./loader.mjs ./test.mjswill rapidly consume all memory on the machine until the OS kills the process (if not terminated sooner).