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

cpu-prof doesn't propagate to workers when env is set #52825

Closed
orinatic opened this issue May 3, 2024 · 5 comments · Fixed by #52827
Closed

cpu-prof doesn't propagate to workers when env is set #52825

orinatic opened this issue May 3, 2024 · 5 comments · Fixed by #52827
Labels
confirmed-bug Issues with confirmed bugs. lib / src Issues and PRs related to general changes in the lib or src directory. worker Issues and PRs related to Worker support.

Comments

@orinatic
Copy link

orinatic commented May 3, 2024

Version

20.8.1

Platform

Linux ushanka-housing 6.5.0-1020-oem #21-Ubuntu SMP PREEMPT_DYNAMIC Wed Apr 3 14:54:32 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

cpu-prof

What steps will reproduce the bug?

You can reproduce this by modifying one of the existent node tests.

If you edit tests/fixtures/workload/fibonacci-worker.js in this repo with

-new Worker(path.join(__dirname, 'fibonacci.js'));
+new Worker(path.join(__dirname, 'fibonacci.js'), {env: process.env});

and then run

node ./test/sequential/test-cpu-prof-dir-worker.js

the new test will fail

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

This should reproduce 100% of the time -- you don't need to do anything special

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

According to https://nodejs.org/docs/latest-v20.x/api/worker_threads.html#new-workerfilename-options, the default value for a worker thread's env is process.env. Thus, not passing an environment and passing process.env as the environment should always produce the same behavior.

What do you see instead?

When running in its original form, the test produces two profile files, as intended. When passing {env: process.env} into the worker invocation, however, it only produces one profile file.

Additional information

If I print the environment in the worker thread, it appears to be identical in both cases. It doesn't seem like the problem is that part of the environment is getting lost, but rather there's some hidden variable that isn't getting propagated

@RedYetiDev RedYetiDev added worker Issues and PRs related to Worker support. confirmed-bug Issues with confirmed bugs. labels May 3, 2024
@RedYetiDev
Copy link
Member

I've been able to reproduce.

const worker_threads = require('worker_threads');

if (worker_threads.isMainThread) {
    new worker_threads.Worker(__filename, { env: process.env });
} else {
    console.log('Hello, world!');
}

node --cpu-prof index.js

@RedYetiDev
Copy link
Member

RedYetiDev commented May 4, 2024

The issue occurs when the env is set to any value. I'll look into it.

@RedYetiDev RedYetiDev added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 4, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented May 4, 2024

I've determined the issue is not on the JS side of things but on the C++ side instead. I'm no CPP expert, so I haven't looked into it much, but I'd assume it is has something to do with the node_worker.cc file: https://github.com/nodejs/node/blob/71a1fa3043d495dfaa2105d07cd090c51bcd8eed/src/node_worker.cc

(I determined this by fiddling around with the /lib/internal/worker.js file, and seeing that no matter the value set, the issue has to do with the WorkerImpl, implemented in CPP)

@RedYetiDev
Copy link
Member

CC @nodejs/workers

@theanarkh
Copy link
Contributor

theanarkh commented May 4, 2024

I think the relevant code is here . I debug into the C++ code and found the cpu_prof in env->options_ is false.

aduh95 pushed a commit that referenced this issue May 12, 2024
PR-URL: #52827
Fixes: #52825
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue May 12, 2024
PR-URL: #52827
Fixes: #52825
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#52827
Fixes: nodejs#52825
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#52827
Fixes: nodejs#52825
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Sep 21, 2024
PR-URL: #52827
Fixes: #52825
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. lib / src Issues and PRs related to general changes in the lib or src directory. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants