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

NODE_V8_COVERAGE inconsistent when used with workers #46378

Open
AriPerkkio opened this issue Jan 27, 2023 · 1 comment
Open

NODE_V8_COVERAGE inconsistent when used with workers #46378

AriPerkkio opened this issue Jan 27, 2023 · 1 comment
Labels
inspector Issues and PRs related to the V8 inspector protocol worker Issues and PRs related to Worker support.

Comments

@AriPerkkio
Copy link

AriPerkkio commented Jan 27, 2023

Version

v19.5.0

Platform

Darwin Aris-MacBook-Air.local 22.2.0 Darwin Kernel Version 22.2.0: Fri Nov 11 02:06:26 PST 2022; root:xnu-8792.61.2~4/RELEASE_ARM64_T8112 arm64

Subsystem

No response

What steps will reproduce the bug?

/Users/x/x/repro
├── index.mjs
└── worker.js
// worker.js
console.log("worker::NODE_V8_COVERAGE", process.env.NODE_V8_COVERAGE);
// index.mjs
import { Worker } from "worker_threads";
import { existsSync } from "fs";
import { resolve } from "path";

const NODE_V8_COVERAGE = resolve("./coverage");

const worker = new Worker("./worker.js", { env: { NODE_V8_COVERAGE } });

await new Promise((resolve, reject) => {
  worker.on("exit", resolve);
  worker.on("error", reject);
});

// Check if V8 coverage report was written to file system
console.log("existsSync(NODE_V8_COVERAGE)", existsSync(NODE_V8_COVERAGE));
$ rm -rf ./coverage # Just to be sure setup is clean
$ node index.mjs 

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

Always.

What is the expected behavior?

Worker should use options.env.NODE_V8_COVERAGE and output the coverage report there.

What do you see instead?

No coverage report is written. However worker logs process.env.NODE_V8_COVERAGE correctly.

$ rm -rf ./coverage # Just to be sure setup is clean
$ node index.mjs 

> worker::NODE_V8_COVERAGE /Users/x/y/repro/coverage
> existsSync(NODE_V8_COVERAGE) false

Worker only outputs coverage report if main thread has process.env.NODE_V8_COVERAGE set. It does not matter what value main thread has. Worker will use the one it was given in options. This is demonstrated below.

If the NODE_V8_COVERAGE is present on main thread, worker will report coverage. The worker.js is seen in ./coverage/<x>.json report.

- const worker = new Worker("./worker.js", { env: { NODE_V8_COVERAGE } });
+ process.env.NODE_V8_COVERAGE = NODE_V8_COVERAGE;
+ const worker = new Worker("./worker.js");
$ rm -rf ./coverage # Just to be sure setup is clean
$ node index.mjs 

worker::NODE_V8_COVERAGE /Users/x/y/repro/coverage
existsSync(NODE_V8_COVERAGE) true

To make things even more inconsistent, let's try having different values for NODE_V8_COVERAGE in main thread and worker.

const NODE_V8_COVERAGE = resolve("./coverage");
...
+ process.env.NODE_V8_COVERAGE = resolve("./coverage-main-thread"); // This won't be used at all, but there has to be some value in main thread's env for workers to work
const worker = new Worker("./worker.js", { env: { NODE_V8_COVERAGE } });
$ rm -rf ./coverage # Just to be sure setup is clean
$ node index.mjs 

> worker::NODE_V8_COVERAGE /Users/x/y/repro/coverage
> existsSync(NODE_V8_COVERAGE) true

$ ls
coverage    index.mjs    worker.js

The main thread's process.env.NODE_V8_COVERAGE is ignored by worker - there is no ./coverage-main-thread directory on file system.
However if main thread's process.env.NODE_V8_COVERAGE is not present, worker won't output coverage at all.

Additional information

It does not matter if worker.js runs takeCoverage():

// worker.js
console.log("worker::NODE_V8_COVERAGE", process.env.NODE_V8_COVERAGE);
+ require("v8").takeCoverage();

To summarize:

  1. Worker does output V8 coverage if main thread has process.env.NODE_V8_COVERAGE set ✅
  2. Worker does not output V8 coverage if main thread has no process.env.NODE_V8_COVERAGE set, and worker is given options.env.NODE_V8_COVERAGE
  3. Worker outputs coverage to options.env.NODE_V8_COVERAGE and not to main thread's process.env.NODE_V8_COVERAGE. If main thread's process.env.NODE_V8_COVERAGE is removed, no report is generated. So why is main threads environment variable required to be present? 😕
@bnoordhuis bnoordhuis added inspector Issues and PRs related to the V8 inspector protocol worker Issues and PRs related to Worker support. labels Jan 27, 2023
@bnoordhuis
Copy link
Member

@nodejs/inspector @bcoe my hunch is that it's caused by this block of code:

node/src/node.cc

Lines 186 to 195 in 0a46107

inspector_agent_->Start(inspector_path,
options_->debug_options(),
inspector_host_port(),
is_main);
if (options_->debug_options().inspector_enabled &&
!inspector_agent_->IsListening()) {
return;
}
profiler::StartProfilers(this);

I.e., returns before calling StartProfilers().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol worker Issues and PRs related to Worker support.
Projects
None yet
Development

No branches or pull requests

2 participants