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

AbortController/AbortSignal Triggering 'Error' Event in Child Process #46036

Closed
pwmichaelharris opened this issue Dec 31, 2022 · 3 comments · Fixed by #46072
Closed

AbortController/AbortSignal Triggering 'Error' Event in Child Process #46036

pwmichaelharris opened this issue Dec 31, 2022 · 3 comments · Fixed by #46072
Labels
child_process Issues and PRs related to the child_process subsystem. good first issue Issues that are suitable for first-time contributors.

Comments

@pwmichaelharris
Copy link

pwmichaelharris commented Dec 31, 2022

Version

v18.9.0

Platform

Linux Mint 20.1

Subsystem

child_process

What steps will reproduce the bug?

  1. Create a child process with an AbortSignal attached.
  2. Abort the child process via AbortSignal before the child process exit event
  3. This will trigger the child process's error event.

The following code will generate the error for spawn, execFile, and exec

const { spawn, exec, execFile } = require("child_process");

const tests = [
    { commandName: "spawn", command: spawn },
    { commandName: "execFile", command: execFile },
    { commandName: "exec", command: exec }
];

(async () => {
  for ( let { commandName, command } of tests ) {
    const abortController = new AbortController();
    
    await new Promise( resolve => {
      //  I used 'ls' but the command does not seem to effect the result
      const child = command( "ls", { signal: abortController.signal } )
        .on( "error", ( err ) =>
            console.log({ event: `${commandName}.error`, killed: child.killed, err }) )

        .on( "exit", ( code, signal ) => {
            console.log({ event: `${commandName}.exit`, code, signal, killed: child.killed });
        } )

        .on( "close", ( code, signal ) => {
            console.log({ event: `${commandName}.close`, code, signal, killed: child.killed });
            resolve();
        } )

        .on( "spawn", () => {
            console.log({ event: `${commandName}.spawn`, killed: child.killed });
//          child.kill( "SIGTERM" );  //  child will NOT cause an 'error' event
            abortController.abort();  //  child will cause an 'error' event
        } );
    } );
  }
})();

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

This occurs every time, a child process is aborted via AbortSignal and it has not performed it's exit event.

What is the expected behavior?

{ event: 'spawn.spawn', killed: false }
{ event: 'spawn.exit', code: null, signal: 'SIGTERM', killed: true }
{ event: 'spawn.close', code: null, signal: 'SIGTERM', killed: true }
...
Repeated for execFile and exec

What do you see instead?

{ event: 'spawn.spawn', killed: false }
{
  event: 'spawn.error',
  killed: true,
  err: AbortError: The operation was aborted
      at abortChildProcess (node:child_process:706:27)
      at AbortSignal.onAbortListener (node:child_process:776:7)
      at [nodejs.internal.kHybridDispatch] (node:internal/event_target:731:20)
      at AbortSignal.dispatchEvent (node:internal/event_target:673:26)
      at abortSignal (node:internal/abort_controller:292:10)
      at AbortController.abort (node:internal/abort_controller:322:5)
      at ChildProcess.<anonymous> (/index.js:30:29)
      at ChildProcess.emit (node:events:513:28)
      at onSpawnNT (node:internal/child_process:481:8)
      at process.processTicksAndRejections (node:internal/process/task_queues:81:21) {
    code: 'ABORT_ERR'
  }
}
{ event: 'spawn.exit', code: 0, signal: null, killed: true }
{ event: 'spawn.close', code: 0, signal: null, killed: true }
...
Repeated for execFile and exec

Additional information

According to the documentation a child process error event occurs only on:

  • The process could not be spawned, or
  • The process could not be killed, or
  • Sending a message to the child process failed.

https://nodejs.org/api/child_process.html#event-error

None of these conditions are true when the process is aborted via AbortSignal. If the child process is terminated via a child.kill( "SIGTERM" ) it does not trigger the error event.

I am assuming that AbortSignal.abort() is suppose to act like child.kill( "SIGTERM" ) and not that the documentation is out-of-date.

@aduh95
Copy link
Contributor

aduh95 commented Jan 1, 2023

I think we should fix the documentation. PR welcome :)

/cc @nodejs/child_process

@aduh95 aduh95 added child_process Issues and PRs related to the child_process subsystem. good first issue Issues that are suitable for first-time contributors. labels Jan 1, 2023
@pwmichaelharris
Copy link
Author

I think we should fix the documentation. PR welcome :)

In regards to how AbortSignal is handled in Node.js's core modules, I agree; however, child_process already deals with abortions and they do not trigger error events. Instead when a child process is aborted, the parent process is alerted in the exit or close event via the signal argument. I think that if AbortSignal triggering an error (like it does in the rest of the core modules) will lead to more problems than if just alerts in exit/close's signal argument (in line with child_process's modus operandi). At least for callback version; the promisify versions, you could make a good argument that AbortSignal is explicit suppose to trigger a rejection.

I think AbortSignal triggering an 'error' event will lead to either:

  • People having to duplicate handlers for aborting. One in the error handler if the signal is from Node.js's AbortSignal and another copy in the exit or close handler if the signal is from the local system.
  • People (lazily) squashing all error events, because it's causing their program to crash and the handler is already in exit and close event.

@debadree25
Copy link
Member

I am assuming that AbortSignal.abort() is suppose to act like child.kill( "SIGTERM" ) and not that the documentation is out-of-date.

The doc does mention AbortController would behave the same way but with the caveat of passing the AbortError

If the signal option is enabled, calling .abort() on the corresponding AbortController is similar to calling .kill() on the child process except the error passed to the callback will be an AbortError:
https://nodejs.org/api/child_process.html#child_processexeccommand-options-callback

But nonetheless have put PR updating that specific line

nodejs-github-bot pushed a commit that referenced this issue Jan 7, 2023
Fixes: #46036
PR-URL: #46072
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this issue Jan 17, 2023
Fixes: nodejs#46036
PR-URL: nodejs#46072
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
RafaelGSS pushed a commit that referenced this issue Jan 20, 2023
Fixes: #46036
PR-URL: #46072
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
juanarbol pushed a commit that referenced this issue Jan 26, 2023
Fixes: #46036
PR-URL: #46072
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
juanarbol pushed a commit that referenced this issue Jan 31, 2023
Fixes: #46036
PR-URL: #46072
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants