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

AbortSignal doesn't abort child_process.spawn #37273

Closed
Linkgoron opened this issue Feb 8, 2021 · 1 comment
Closed

AbortSignal doesn't abort child_process.spawn #37273

Linkgoron opened this issue Feb 8, 2021 · 1 comment
Labels
child_process Issues and PRs related to the child_process subsystem.

Comments

@Linkgoron
Copy link
Member

  • Version: v15.5.0, v15.8.0 (works fine on v15.6, v15.7)
  • Platform: macOS 10.15.7
  • Subsystem: child_process

What steps will reproduce the bug?

Create a child process using spawn, and abort the controller. The signal doesn't abort child process.

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

This occurs every time, while running the following file:

const { spawn } = require('child_process');

const ac = new AbortController();
const { signal } = ac;
const cp = spawn(process.execPath, ['./infinity-demo.js'], { signal });
const stillRunningTimeout = setTimeout(() => { console.log('still running!'); cp.kill('SIGTERM'); }, 5000);
cp.on('exit', () => { clearTimeout(stillRunningTimeout); console.log('exited') });
cp.on('error', (e) => console.log('AN ERROR HAS HAPPENED', e.name));
setTimeout(()=>ac.abort(),5);

This is infinity-demo.js:

setInterval(()=>{},1000);

What is the expected behaviour?

The cp should die correctly, with the program printing:

AN ERROR HAS HAPPENED AbortError
exited

What do you see instead?

The cp still running (even though it emits an error) with the program printing:

AN ERROR HAS HAPPENED AbortError
still running!
exited

Additional information

This works correctly on 15.6, 15.7 but fails on 15.5 and 15.8. I believe that the tests missed this because they run on short-lived tasks (echo etc.) that die anyway, and in addition the cp still emits the error.

I'd be happy to provide a PR for this.

@benjamingr
Copy link
Member

Good find thanks - looking forward to the PR

@PoojaDurgad PoojaDurgad added the child_process Issues and PRs related to the child_process subsystem. label Feb 9, 2021
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 11, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: nodejs#37273
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 11, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: nodejs#37273
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 13, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: nodejs#37273
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 20, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: nodejs#37273
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 20, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: nodejs#37273
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 20, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: nodejs#37273
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 23, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: nodejs#37273
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 25, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: nodejs#37273
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 25, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: nodejs#37273
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 25, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: nodejs#37273
targos pushed a commit that referenced this issue Feb 28, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: #37273

PR-URL: #37325
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit to targos/node that referenced this issue Aug 8, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: nodejs#37273

PR-URL: nodejs#37325
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit that referenced this issue Sep 1, 2021
Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: #37273

PR-URL: #37325
Reviewed-By: Benjamin Gruenbaum <[email protected]>
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

Successfully merging a pull request may close this issue.

3 participants