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

trace-atomics-wait doesn't propagate to workers when env is set #53374

Closed
orinatic opened this issue Jun 6, 2024 · 7 comments
Closed

trace-atomics-wait doesn't propagate to workers when env is set #53374

orinatic opened this issue Jun 6, 2024 · 7 comments
Labels
worker Issues and PRs related to Worker support.

Comments

@orinatic
Copy link

orinatic commented Jun 6, 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

trace-atomics-wait

What steps will reproduce the bug?

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

If you edit tests/parallel/test-trace-atomics-wait.js in this repo with

-`, { eval: true, workerData: i32arr });
+`, { eval: true, workerData: i32arr, env: process.env });

and then run

node ./test/parallel/test-trace-atomics-wait.js

the 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

This may be a dup of #52825, but given that it's a different flag, the fix might be slightly different, so I figured I should bring it to your attention

@RedYetiDev
Copy link
Member

CC @theanarkh, I believe #53029 fixed this, right?

@VoltrexKeyva VoltrexKeyva added the worker Issues and PRs related to Worker support. label Jun 6, 2024
@theanarkh
Copy link
Contributor

Yes. But this flag --trace-atomics-wait has been removed.

@orinatic
Copy link
Author

orinatic commented Jun 7, 2024

Oh, also: it looks like the same thing is true of --trace-exit. I assume that's probably covered by the fix as well though?

@theanarkh
Copy link
Contributor

Yes. The fix ensures that execArgv is passed to the worker(it will be released in Node.js v22.3.0).

You can try the following code with main branch(node --trace-exit demo.js)

const { isMainThread, Worker } = require('worker_threads')
if (isMainThread) {
    new Worker(__filename, { env: process.env })
} else {
    throw new Error('a')
}

it will print

(node:38048, thread:1) WARNING: Exited the environment with code 1
(node:38048) WARNING: Exited the environment with code 1

Before v22.3.0. It will print

(node:38048) WARNING: Exited the environment with code 1

@orinatic
Copy link
Author

orinatic commented Jun 7, 2024

Awesome, thank you!

@RedYetiDev
Copy link
Member

Great! v22.3.0 will release soon (#53379)

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

No branches or pull requests

5 participants
@orinatic @theanarkh @RedYetiDev @VoltrexKeyva and others