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

Using execArgv with worker_threads.Worker breaks 'NODE_PRESERVE_SYMLINKS' envvar usage in worker. #30627

Closed
arciisine opened this issue Nov 24, 2019 · 6 comments
Labels
worker Issues and PRs related to Worker support.

Comments

@arciisine
Copy link

  • Version: 12.13.0
  • Platform: Linux

I have a child_process.fork that I am converting to use the worker_threads module. Currently I need to conditionally preserve symlinks, and this information was being passed through environment variables to the forked process. Switching over to worker_threads, this seemed to no longer work by default.

When creating the Worker instance, I am passing the NODE_PRESERVE_SYMLINKS flag in the env object. This fails to to work when execArgv is used as well. By removing execArgv, everything works as expected.

Looking at the docs for Worker's execArgv

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

I can pass the --preserve-symlinks flag as a CLI option, and I've verified that is working. Though this is less than ideal as I need to enforce this conditionally.

Ultimately I'm trying to see if there is something overriding the env usage, or if the env vars, regarding process creation are potentially being ignored.

@addaleax addaleax added the worker Issues and PRs related to Worker support. label Nov 25, 2019
@HarshithaKP
Copy link
Member

@arciisine, I was able to recreate it with flag --throw_deprecation.

const { Worker, isMainThread, parentPort, workerData } = require('worker_threads');
const a = true
if (isMainThread) {
   new Worker(__filename, { execArgv:[a]});
} 
else {
  console.log('Inside Worker!');
  const k = new Buffer(10)
}

I debugged in worker threads and based on my observation, the property is set at lib/internal/bootstrap/pre_execution.js but later it is unset, do not know for whatever reason.

@HarshithaKP
Copy link
Member

  • just to clarify, the above code does not throw exception as expected, when export NODE_OPTIONS=--throw-deprecation is ON.
  • drilling down a bit, process.throwDeprecation field is undefined in worker is the root cause of this. when execArgv is absent, this field is true as expected.
  • I have evidence that process.throwDeprecation is good in
    addReadOnlyProcessAlias('throwDeprecation', '--throw-deprecation');
    but later it is unset.
  • debugged a bit in JS (worker constructor) and C++ (Worker::New and Worker::Worker) but they don’t take part in decision making for this field value
  • I see the process object creation and population is treated differently for main and worker threads (lib/internal/bootstrap/switches...) but not able to follow the bootstrap sequence completely. @addaleax / @joyeecheung - any guidance?

@HarshithaKP
Copy link
Member

Also, is there a way for me to print debug data in the bootstrap phase? console.log / process.stdout.write etc. do not work, because of the phase in which we are, where these objects are not fully bootstrapped?

@addaleax
Copy link
Member

@HarshithaKP The reason that this is happening is that when execArgv is passed, it is assumed that that is the entire execArgv for the worker. The issue is that NODE_OPTIONS and other environment variables aren’t being taken into account when setting that up.

Also, is there a way for me to print debug data in the bootstrap phase? console.log / process.stdout.write etc. do not work, because of the phase in which we are, where these objects are not fully bootstrapped?

For debugging Node.js core, process._rawDebug() is a good choice.

@HarshithaKP
Copy link
Member

@addaleax , thanks for the info on process._rawDebug, that is really useful.

The reason that this is happening is that when execArgv is passed, it is assumed that that is the entire execArgv for the worker.

I believe this is a bug, do you agree?

The issue is that NODE_OPTIONS and other environment variables aren’t being taken into account when setting that up.

I am trying to find out that location, but failed. I looked at these locations:

  • bootstrap sequences
  • parser
  • worker constructor
  • GetOptions()

Do you know where this decision is taken? my current debugging is halted with this finding:

const result = options.get(option);

It returns true for main thread and undefined for worker.

@addaleax
Copy link
Member

The reason that this is happening is that when execArgv is passed, it is assumed that that is the entire execArgv for the worker.

I believe this is a bug, do you agree?

Yes.

The issue is that NODE_OPTIONS and other environment variables aren’t being taken into account when setting that up.

I am trying to find out that location, but failed. I looked at these locations:

The Worker::New() function (in src/node_worker.cc) should do this when putting together per_isolate_opts, imo.

codebytere pushed a commit that referenced this issue Feb 17, 2020
PR-URL: #31711
Fixes: #30627
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
codebytere pushed a commit that referenced this issue Mar 15, 2020
PR-URL: #31711
Fixes: #30627
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
codebytere pushed a commit that referenced this issue Mar 17, 2020
PR-URL: #31711
Fixes: #30627
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
codebytere pushed a commit that referenced this issue Mar 30, 2020
PR-URL: #31711
Fixes: #30627
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants