--conditions xxx cannot pass into worker_thread

[himself65]

Version

v20.10.0

Platform

Darwin Alexs-MacBook-Pro.local 23.1.0 Darwin Kernel Version 23.1.0: Mon Oct 9 21:28:45 PDT 2023; root:xnu-10002.41.9~6/RELEASE_ARM64_T6020 arm64

Subsystem

module

What steps will reproduce the bug?

// main.js
import { Worker } from 'node:worker_threads'

new Worker(new URL('worker.js', import.meta.url), {
  execArgv: ['--conditions', 'react-server']
})

// worker.js
import { register } from 'node:module'

register('./register.js', import.meta.url)

console.log('1')

import('./test.js')

// register.js
export async function resolve (specifier, context, nextResolve) {
  // Take an `import` or `require` specifier and resolve it to a URL.
  console.log('context', context.conditions)
  return nextResolve(specifier, context, nextResolve)
}

export async function load (url, context, nextLoad) {
  return nextLoad(url, context, nextLoad)
}

// test.js
console.log('3')

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

1
context [ 'node', 'import', 'node-addons', 'react-server' ]
3

What do you see instead?

1
context [ 'node', 'import', 'node-addons' ]
3

Additional information

Might cause RSC bundle issue dai-shi/waku#178

[juanarbol]

I think I can work on this one <3

Thanks for the report

[himself65]

I think I can work on this one <3

Thanks for the report

Appreciate it

[juanarbol]

For whatever reason, Node.js is creating three workers, the first one is receiving and setting the context, as expected, but the next two are not, I'm still working on it

[juanarbol]

Oh yes, the loader now lives on a worker thread; I'll see how I could send the argv to those workers.

[GeoffreyBooth]

@nodejs/loaders

[himself65]

I believe that the bug is more complex than I thought

[himself65]

https://github.com/himself65/node-js-worker-thread-hook

[himself65]

Please try this demo

node ./main.js

the output is

this is pkg-a/react-server.cjs
this is pkg-b/index.cjs

But I think it should be

this is pkg-a/react-server.cjs
this is pkg-b/react-server.cjs

[GeoffreyBooth]

This behavior is expected. You can pass argv (or anything else) into the hooks thread via register and initialize. See #50916 (review)

[targos]

@GeoffreyBooth execArgv control how the Node.js instance is created. You cannot configure everything at runtime after that.

[himself65]

himself65/node-js-worker-thread-hook

This is weird thing. In this case, CJS module will have conditions but ESM module won't have

[juanarbol]

I don't think the ESM worker needs more argv than --conditions I could continue with #50916 by sending the --conditions argv if provided (and modify the factual test, as requested); what do you think about that @nodejs/loaders and participants?

[GeoffreyBooth]

So I think I misunderstood the original request. I guess the desire is that context.conditions in the hook input contains whatever the conditions were passed when Node was initialized?

It so then I think finishing #50752 would resolve this, as new Worker in user code wouldn’t be spawning a new hooks thread and therefore the hooks thread would have the same conditions as the main thread received. If anyone on this issue can tackle finishing that PR, I could really use the help.

[guybedford]

I wonder if the bug here is actually just that --conditions is appearing in process.argv instead of process.execArgv (which is passed through to worker threads automatically by default).

If so, the fix might be as simple as including the --conditions in Node.js's process.execArgv population.

That would fit my intuition, as the expectation is that --conditions is an environment-style flag.

[juanarbol]

I wonder if the bug here is actually just that --conditions is appearing in process.argv instead of process.execArgv (which is passed through to worker threads automatically by default).

No. The problem is that we are creating the ES worker w/out the parent's execArgv I debugged the whole thing in the C++ layer, and everything is working as expected. See https://github.com/nodejs/node/blob/main/lib/internal/modules/esm/hooks.js#L508 and https://github.com/nodejs/node/blob/main/lib/internal/worker.js#L465

[guybedford]

Ahh, ok so the loader is getting the top-level conditions, but not the explicit conditions passed to execArgv for the individual worker being created.

@GeoffreyBooth brings up a good point then as to whether the design we want here is one where all workers share the same global resolution context, or whether we want to enable new Worker to have the ability to spawn a sub-resolution context.

This also extends to for example import maps support - would we expect the import map of the parent to be shared with the worker, or would we expect to be able to override that etc.

These are important questions to get right. My personal preference would be to not treat worker spawning as a virtualization boundary, but to instead have transparent worker spawning in the same resolution context, while enabling virtualization through other virtualization-specific APIs that explicitly control the entire loader context.

@GeoffreyBooth are these topics on the loaders agenda? I'd definitely be interested to join to discuss them further.

[GeoffreyBooth]

are these topics on the loaders agenda? I'd definitely be interested to join to discuss them further.

They're not in the agenda but they could be if you want. The current behavior of a new hooks thread for each new application worker thread was accidental, and is considered a bug. I don't know if anyone is advocating for that behavior, and if not then I think we're already in alignment where we treat the hooks thread as derived from the initial environment/main thread like #50752 aims to achieve.

[juanarbol]

Just asking, should I close #50916 as long as will be superseded by #50752?

[GeoffreyBooth]

Just asking, should I close #50916 as long as will be superseded by #50752?

I think so. #50916 assumes that “one hooks thread per worker thread” is correct and tries to align their environments, but that’s an incorrect assumption in the first place (that we need to fix).

[daniel-nagy]

For what it is worth, I just ran into this issue while experimenting with React server components.