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(...) (and fork, spawn) do not abort with expected signal.reason #47814

Closed
MadLittleMods opened this issue May 2, 2023 · 2 comments · Fixed by #47817
Closed
Labels
abortcontroller Issues and PRs related to the AbortController API child_process Issues and PRs related to the child_process subsystem.

Comments

@MadLittleMods
Copy link
Contributor

MadLittleMods commented May 2, 2023

Version

v18.16.0

Platform

Microsoft Windows NT 10.0.19044.0 x64

Subsystem

No response

What steps will reproduce the bug?

  1. Use Node.js v18.16.0
  2. Run this script in the REPL or save as a script and run with Node.js
    (async () => {
        const { spawn } = require('child_process');
    
        const abortController = new AbortController();
        setTimeout(() => {
            abortController.abort(new Error('My custom error'));
        }, 0);
    
        try {
            await new Promise((resolve, reject) => {
                const child = spawn('echo', ["foo"], { signal: abortController.signal });
                child.on('error', (err) => {
                    reject(err);
                });
                child.on('close', () => {
                    resolve();
                });
            });
        } catch(err) {
            console.log('err', err);
            // err AbortError: The operation was aborted
            //     at abortChildProcess (node:child_process:720:27)
            //     at EventTarget.onAbortListener (node:child_process:790:7)
            //     at [nodejs.internal.kHybridDispatch] (node:internal/event_target:737:20)
            //     at EventTarget.dispatchEvent (node:internal/event_target:679:26)
            //     at abortSignal (node:internal/abort_controller:314:10)
            //     at AbortController.abort (node:internal/abort_controller:344:5)
            //     at Timeout._onTimeout (REPL22:6:25)
            //     at listOnTimeout (node:internal/timers:569:17)
            //     at process.processTimers (node:internal/timers:512:7) {
            //   code: 'ABORT_ERR'
        }
    })();
  3. Notice AbortError: The operation was aborted instead of our custom signal.reason -> Error: My custom error

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

Consistently reproducible (always)

What is the expected behavior? Why is that the expected behavior?

I expect this to behave like the fetch API where the error thrown is the signal.reason that we aborted with. Example:

(async () => {
    const abortController = new AbortController();
    setTimeout(() => {
        abortController.abort(new Error('My custom error'));
    }, 0);

    try {
        const res = await fetch('https://nodejs.org/', { signal: abortController.signal });
    } catch(err) {
        console.log('err', err);
        // err Error: My custom error
        //     at Object.fetch (node:internal/deps/undici/undici:11457:11)
        //     at async REPL17:8:21
    }
})();

What do you see instead?

It always throws: AbortError: The operation was aborted

Additional information

Related to #43874

debadree25 added a commit to debadree25/node that referenced this issue May 2, 2023
@debadree25 debadree25 added child_process Issues and PRs related to the child_process subsystem. abortcontroller Issues and PRs related to the AbortController API labels May 2, 2023
@benjamingr
Copy link
Member

I expect this to behave like the fetch API where the error thrown is the signal.reason that we aborted with.

That's a bad API since signals get passed into things the code around it can't predict what exceptions may be thrown. The current behavior is intentional and what fetch does is really unfortunate.

@MadLittleMods
Copy link
Contributor Author

AbortErrors shouldn't be overridable to reject with other stuff even if that's the .reason and should expose it as .cause instead. It means every user would need to refactor their error handling otherwise.

@benjamingr Your idea from #43874 to put the signal.reason as the Error: cause sounds decent. Whatever consistent thing would be nice.

It seems like things are converging on throwing signal.reason given fetch and #43874 🤷

nodejs-github-bot pushed a commit that referenced this issue May 8, 2023
Fixes: #47814
PR-URL: #47817
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this issue May 12, 2023
Fixes: #47814
PR-URL: #47817
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this issue Jul 6, 2023
Fixes: #47814
PR-URL: #47817
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this issue Jul 6, 2023
Fixes: nodejs#47814
PR-URL: nodejs#47817
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abortcontroller Issues and PRs related to the AbortController API child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@MadLittleMods @benjamingr @debadree25 and others