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

Calling unref() on a child process instance created by Node.js' spawn API kills the sub process #21446

Open
ymeine opened this issue Dec 3, 2023 · 2 comments
Assignees
Labels
needs investigation requires further investigation before determining if it is an issue or not node API polyfill Related to various "node:*" modules APIs node compat

Comments

@ymeine
Copy link

ymeine commented Dec 3, 2023

Version: Deno 1.38.0
Platform: Microsoft Windows NT 10.0.22621.0 x64


Scenario

A parent program:

import process from 'node:process';
import {spawn} from 'node:child_process';

const timer = `Running program`;
console.time(timer);
process.on('exit', () => console.timeEnd(timer));

const procNode = spawn('node', ['test.js']);
setTimeout(() => procNode.unref(), 1000);

that spawns a Node.js sub process implemented by test.js:

import {writeFile} from 'node:fs/promises';
import {setTimeout} from 'node:timers/promises';

console.log('node running');
await setTimeout(2000);
await writeFile('from-node.txt', 'Hello from Node.js');

Bug

We can see Deno fails making the sub process go through because the file from-node.txt is never created.

If you're wondering about the different logs and timings, as well as why I spawn a child Node.js process: it's an example I adapted from this Node.js issue

They help inspecting the behavior. Thanks to that, I can see in System Informer that the sub process is created, but that it gets terminated at the same time as the parent due to the unref() call in the middle.

This code works with Node.js v20.9.0.

Expectations

As part of the Node.js compatibility work, this should eventually be fixed.

In the meantime, update the compatibility list page which tells only this:

The ipc and overlapped stdio options are missing. Passing file descriptors by an integer value is missing.

Context

@bartlomieju
Copy link
Member

@ymeine thanks for the report. I just tried your reproduction in both Deno (v1.42.3) and Node (v.21.5.0) on macOS and in both cases the file is not written. I think the behavior here is correct - unless you wanted to spawn the subprocess as "detached" one, meaning it should continue running after the parent exits. Please provide more information.

@bartlomieju bartlomieju added needs info needs further information to be properly triaged and removed bug Something isn't working correctly labels Apr 14, 2024
@ymeine
Copy link
Author

ymeine commented Apr 14, 2024

Hi,

I've tried again on my side:

  • Node.js: 21.7.3, 21.5.0, 20.9.0
  • Deno: 1.42.3
  • OS: Windows

I've adapted the reproduction code to make it a bit clearer:

main.js

import process from 'node:process';
import {spawn} from 'node:child_process';

const isDeno = globalThis.Deno != null;
if (isDeno) console.log(`Running Deno ${Deno.version.deno}`);
else console.log(`Running Node.js ${process.version}`);

const timer = 'Current time';
console.time(timer);

process.on('exit', () => console.timeEnd(timer)); // should print something around 1 second, not 2 seconds

const child = spawn(
    'node', ['child.js', isDeno ? 'deno' : 'node'],
    {stdio: 'ignore'}, // try commenting that line to see the difference between Node.js and Deno here
);
setTimeout(() => {
    console.timeLog(timer); // should print something around 1 second
    child.unref();
}, 1000);

child.js

import {writeFile} from 'node:fs/promises';
import {setTimeout} from 'node:timers/promises';

const runtime = process.argv[2];
console.log(`${runtime} running`);
await setTimeout(2000);
await writeFile(`from-${runtime}.txt`, `Hello from ${runtime}`);

Now I am a bit confused here because the result is not how I remembered, but honestly I was opening too many detailed bug reports in various projects at that time, my brain fatigue was real.

So here is what happens:

  • without the {stdio: 'ignore'} option (as in my original bug report):
    • Deno exits after 1 second, not waiting for the child process to complete, and kills that process while exiting: the file is not created
    • Node.js exits after 2 seconds, still waiting for the child process to complete despite the unref call: the file is created
  • with the {stdio: 'ignore'} option: both Deno and Node.js behave the same, exiting and killing the child process after 1 second

Given the more accurate testing, and given the result you gave for the original reproduction on macOS, it looks like there are discrepancies:

  • Node.js on macOS and on Windows does not behave the same: without the {stdio: 'ignore'} option, one still waits for the child process and the other does not
  • if we imagine the Windows version is correct, then there's still a difference between Node.js and Deno, where the latter does not require {stdio: 'ignore'} to be set for unref to behave as expected

At that stage I don't know what to think, since with the discrepancy for Node.js itself between OSs, it's not possible to tell if the Windows behavior is correct or not. If it is, there's a compatibility issue between Deno and Node.js; if it isn't, then there's just a Node.js bug and no Deno bugs.

@bartlomieju bartlomieju added needs investigation requires further investigation before determining if it is an issue or not and removed needs info needs further information to be properly triaged labels Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs investigation requires further investigation before determining if it is an issue or not node API polyfill Related to various "node:*" modules APIs node compat
Projects
None yet
Development

No branches or pull requests

3 participants