src: fix kill signal 0 on Windows

[StefanStojanovic]

The previous changes to this file missed the special case of process.kill(0), thus breaking it on Windows. This PR fixes that.

Refs: #55514
Refs: #42923
Fixes: #57669

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (657f818) to head (cd81beb).
Report is 23 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #57695   +/-   ##
=======================================
  Coverage   90.23%   90.24%           
=======================================
  Files         630      630           
  Lines      185017   185017           
  Branches    36207    36216    +9     
=======================================
+ Hits       166948   166966   +18     
- Misses      11020    11025    +5     
+ Partials     7049     7026   -23     

Files with missing lines	Coverage Δ
src/process_wrap.cc	66.66% <ø> (ø)

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[targos]

Is it possible to add a test?

[StefanStojanovic]

Is it possible to add a test?

Good idea. I've added it. Hopefully, the 1 second I gave it will be enough for the CI machines.

[lpinca]

Just a suggestion (that can be ignored) to not rely on timers and not wait a second for the test to end.

Suggested change

	const checkProcess = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 1000)']);
	checkProcess.on('exit', (code, signal) => {
	assert.strictEqual(code, 0);
	assert.strictEqual(signal, null);
	});
	checkProcess.kill(0);
	const checkProcess = spawn(process.execPath, [
	'-e',
	'setInterval(() => {}, 1000)'
	]);
	checkProcess.on('exit', (code, signal) => {
	assert.strictEqual(code, null);
	assert.strictEqual(signal, 'SIGTERM');
	});
	checkProcess.kill(0); // 'SIGKILL' is not sent to the child process.
	checkProcess.kill('SIGTERM');

[StefanStojanovic]

I like changing timeout to interval and then killing it manually to have complete control. However, the new asserts change the nature of the test itself. We want to test checkProcess.kill(0); and see if the code and signal will be as expected. This change checks if sending 0 will kill the process, but not if it'll return the expected values. Correct?

[lpinca]

Yes, correct, the assumption is that checkProcess.kill(0) is a no-op because the received signal is 'SIGTERM'. If it wasn't the received signal would be 'SIGKILL'. I think this is sufficient to assert that it doesn't change the signal. However, as you said, there is no check on the exit code.

[lpinca]

Another option can be something like this:

const code = `const interval = setInterval(() => {}, 1000);
process.stdin.on('data', () => { clearInterval(interval); });
process.stdout.write('x');
`;

const checkProcess = spawn(process.execPath, ['-e', code]);

checkProcess.on('exit', (code, signal) => {
  assert.strictEqual(code, 0);
  assert.strictEqual(signal, null);
});

checkProcess.stdout.on('data', common.mustCall((chunk) => {
  assert.strictEqual(chunk.toString(), 'x');
  checkProcess.kill(0);
  checkProcess.stdin.write('x');
  checkProcess.stdin.end();
}));

What do you think?

[StefanStojanovic]

I like that one. It's more elegant IMHO. You can submit it as a new suggestion if you want.

[lpinca]

Feel free to apply it yourself.

[StefanStojanovic]

Done, please reapprove when you get a chance. Thanks.

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/65926/

[github-actions]

Failed to start CI

   ⚠  Commits were pushed since the last approving review:
   ⚠  - src: fix kill signal 0 on Windows
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/14239782978

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/65996/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66005/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66011/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66017/

[nodejs-github-bot]

Landed in 32e5e81