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

child_process.exec is not abortable in Linux #37518

Closed
MadaraUchiha opened this issue Feb 25, 2021 · 8 comments
Closed

child_process.exec is not abortable in Linux #37518

MadaraUchiha opened this issue Feb 25, 2021 · 8 comments
Labels
child_process Issues and PRs related to the child_process subsystem.

Comments

@MadaraUchiha
Copy link
Contributor

MadaraUchiha commented Feb 25, 2021

  • Version: 15.8.0 (but also 12.21.0, and 16.0.0-pre)
  • Platform: Linux madara-laptop-linux 5.4.0-65-generic The README is oriented towards Node #73-Ubuntu SMP Mon Jan 18 17:25:17 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: child_process

What steps will reproduce the bug?

  1. Create a script that will run forever eg. keep-alive.js: setInterval(() => {}, 10_000);
  2. Create a script that will exec that file and abort it within a few seconds:
// keep-alive-test.js
const { exec } = require('child_process');

const ac = new AbortController();
const { signal } = ac;
const child = child_process.exec(`${process.execPath} ./keep-alive.js`, { signal ), console.log);

setTimeout(() => ac.abort(), 10_000);
  1. Run node keep-alive-test.js &
  2. Examine running processes with ps awwx | grep alive | grep -v grep | cat
  3. You will note 3 running processes, node keep-alive-test.js, sh -c node keep-alive.js and node keep-alive.js
  4. After 10 seconds, the abort is invoked.
  5. Examine running processes again with ps awwx | grep alive | grep -v grep | cat
  6. You will note 2 running processes, node keep-alive-test.js and node keep-alive.js (sh was terminated).
  7. Both processes will live forever until explicitly terminated by the user (fg -> CTRL+C, or killall node etc)

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

Every time.

What is the expected behavior?

Expected behavior for ac.abort() was to abort the child process.

What do you see instead?

Child process does not get aborted, only the shell does.

Additional information

Internally, the abort handler for all child_process methods use child.kill() to terminate the underlying process. However, it is documented that this does not work "as expected" in Linux.

This makes ac.abort() quite useless on execed children, since none of them would actually get aborted at all.

This will probably happen if one uses child_process.spawn() to similarly spawn a shell to invoke a process, though I have not tested.

@MadaraUchiha
Copy link
Contributor Author

cc @Linkgoron @benjamingr

@RaisinTen
Copy link
Contributor

RaisinTen commented Feb 26, 2021

@MadaraUchiha Is this the same problem as reported in #37273?
exec calls execFile which calls spawn internally with the signal here:

node/lib/child_process.js

Lines 254 to 263 in 5c3bc21

const child = spawn(file, args, {
cwd: options.cwd,
env: options.env,
gid: options.gid,
shell: options.shell,
signal: options.signal,
uid: options.uid,
windowsHide: !!options.windowsHide,
windowsVerbatimArguments: !!options.windowsVerbatimArguments
});

and that's being fixed in #37325.

@benjamingr
Copy link
Member

Fixed in #37273 - thanks for the report :)

@benjamingr
Copy link
Member

Apparently not 😅

@benjamingr benjamingr reopened this Feb 27, 2021
@MadaraUchiha
Copy link
Contributor Author

MadaraUchiha commented Feb 28, 2021

and that's being fixed in #37325.

@RaisinTen I don't see how. ac.abort() still calls the underlying child.kill(), and child.kill() does not work as expected in Linux, when child is sh.

In fact, the fact that @Linkgoron uses sleep instead of the alive-forever fixture for the exec test indicates that they specifically want to use something that's long enough to not immediately terminate and affect the test, but short-lived enough as to not leak resources in the test.

@richardlau
Copy link
Member

FTR I don't think this is limited to Linux. I've been able to reproduce this on our SmartOS CI instances while investigating nodejs/build#3154.

nodejs-github-bot pushed a commit that referenced this issue Jan 22, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau added a commit to richardlau/node-1 that referenced this issue Jan 22, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: nodejs#46276
Fixes: nodejs/build#3154
Refs: nodejs#37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau added a commit that referenced this issue Jan 22, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ruyadorno pushed a commit that referenced this issue Feb 1, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 3, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 5, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: #46276
Fixes: nodejs/build#3154
Refs: #37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
oraluben pushed a commit to oraluben/node that referenced this issue Mar 14, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: nodejs/node#46276
Fixes: nodejs/build#3154
Refs: nodejs/node#37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
oraluben pushed a commit to oraluben/node that referenced this issue Mar 17, 2023
Extend the Linux logic to all POSIX platforms in
test-child-process-exec-abortcontroller-promisified.

PR-URL: nodejs/node#46276
Fixes: nodejs/build#3154
Refs: nodejs/node#37518
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BlackGlory
Copy link

BlackGlory commented Jun 1, 2023

As I recall exec is just an advanced wrapper for spawn, so once you use shell: true, the problem exists.

My current solution is to kill the whole process group via process.kill(-pid)(you also need detached: true):

signal?.addEventListener('abort', () => {
  if (childProcess.pid !== undefined) {
    process.kill(-childProcess.pid)
  }
})

@bnoordhuis
Copy link
Member

I think this issue is a case of wrong expectations. Maybe the documentation can be clarified but I don't consider it a node bug, it's just how the UNIX process model works. Either avoid shell: true or use @BlackGlory's solution.

I'm going to close this but if someone wants to send a documentation pull request, please do.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants