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

Programmatically exiting deno does not kill child processes running #8772

Closed
andykais opened this issue Dec 15, 2020 · 5 comments
Closed

Programmatically exiting deno does not kill child processes running #8772

andykais opened this issue Dec 15, 2020 · 5 comments
Labels
working as designed this is working as intended

Comments

@andykais
Copy link

I have discovered that calling Deno.exit() will stop the deno process, but will not stop any subprocesses running inside it. This is different than the behavior when ctrl-c'ing a program.

small repro:

const proc = Deno.run({ cmd: ['ping', 'google.com', '-c', '4'] })
proc.status().then(() => {
  console.log('ping command completed')
})
setTimeout(() => {
  console.log('programmatically exiting')
  Deno.exit(0)
}, 1000)

output from the program:

 andrew  ~  deno run --allow-run tiny-repro.ts
Check file:///home/andrew/tiny-repro.ts
PING google.com(lga34s18-in-x0e.1e100.net (2607:f8b0:4006:818::200e)) 56 data bytes
64 bytes from lga34s18-in-x0e.1e100.net (2607:f8b0:4006:818::200e): icmp_seq=1 ttl=115 time=47.3 ms
programmatically exiting
 andrew  ~  64 bytes from lga34s18-in-x0e.1e100.net (2607:f8b0:4006:818::200e): icmp_seq=2 ttl=115 time=47.4 ms
64 bytes from lga34s18-in-x0e.1e100.net (2607:f8b0:4006:818::200e): icmp_seq=3 ttl=115 time=45.3 ms
64 bytes from lga34s18-in-x0e.1e100.net (2607:f8b0:4006:818::200e): icmp_seq=4 ttl=115 time=48.4 ms

vs when I cancel early with ctrl+c, the subprocess also receives a stop signal:

 ✘ andrew  ~  deno run --allow-run tiny-repro.ts 
PING google.com(lga34s18-in-x0e.1e100.net (2607:f8b0:4006:818::200e)) 56 data bytes
64 bytes from lga34s18-in-x0e.1e100.net (2607:f8b0:4006:818::200e): icmp_seq=1 ttl=115 time=58.5 ms
^C
--- google.com ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 58.545/58.545/58.545/0.000 ms

Is this expected behavior?

@magurotuna
Copy link
Member

That looks to be a bit related to #3603 in the sense that they both have something to do with cleaning up on Deno.exit.

Deno.exit() internally calls std::process::exit in Rust, but destructors that clean up child processes won't run unless we call std::process::exit at a proper place.

From doc(https://doc.rust-lang.org/std/process/fn.exit.html):

Note that because this function never returns, and that it terminates the process, no destructors on the current stack or any other thread's stack will be run. If a clean shutdown is needed it is recommended to only call this function at a known point where there are no more destructors left to run.

So ideally std::process::exit should be called at a place almost near to the entry point (main function) but I think it should be kind of hard considering the current infrastructure of ops.

@kt3k
Copy link
Member

kt3k commented Jan 9, 2021

I'm not sure what is the expected behavior here, but I found node.js works in a similar way.

If you run the following and press ctrl-c, the subprocess doesn't leak,

// node.js
require("child_process").exec("sleep 1000");

but the follwoing leaks sleep 1000 on each execution:

require("child_process").exec("sleep 1000");
process.exit(0);

@kt3k
Copy link
Member

kt3k commented Jan 9, 2021

https://stackoverflow.com/questions/60193762/who-send-sigint-to-foreground-process-when-press-ctrlc-tty-driver-or-shell

It seems that when pressing ctrl-c the tty driver program sends sigint to the process group. That's what OS does and not the responsibility of single process (deno cli in this case). So I think we shouldn't do anything special for this situation.

@stale
Copy link

stale bot commented Mar 11, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 11, 2021
@kitsonk kitsonk added the working as designed this is working as intended label Mar 11, 2021
@stale stale bot removed the stale label Mar 11, 2021
@kitsonk
Copy link
Contributor

kitsonk commented Mar 11, 2021

As mentioned by @kt3k, this behaviour aligns to Node.js and further research indicating that this isn't something that Deno should do specifically.

@kitsonk kitsonk closed this as completed Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
working as designed this is working as intended
Projects
None yet
Development

No branches or pull requests

4 participants