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: use signal.reason in child process abort #47817

Merged
merged 5 commits into from
May 8, 2023

Conversation

debadree25
Copy link
Member

Fixes: #47814

@debadree25 debadree25 requested a review from benjamingr May 2, 2023 11:22
@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels May 2, 2023
@debadree25 debadree25 added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 2, 2023
@aduh95
Copy link
Contributor

aduh95 commented May 2, 2023

Shouldn't we keep rejecting with an AbortError and add the reason on the cause property of said error? Can you add tests when the AbortController is aborted with a non-Error value?

@debadree25
Copy link
Member Author

Oh ok my bad it seems new AbortError(undefined, { cause: signal?.reason }) is the proper way, updating and adding the relevant test @aduh95

Thanks!

@debadree25
Copy link
Member Author

Ok so the original issue mentions wanting the behavior to be like fetch, fetch seems to just pass the error without wrapping on AbortError so which way would be correct 🧐

@benjamingr
Copy link
Member

We don't want to throw signal.reason since that would "explode" the API surface and would mean anything can be thrown so +1 for new AbortSignal("...", { cause: signal.reason })

@debadree25
Copy link
Member Author

Done updated to use cause, and included tests where the passed value is not a error, PTAL :-)

Thanks!

@debadree25 debadree25 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 2, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 2, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@debadree25 debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 6, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/47817
✔  Done loading data for nodejs/node/pull/47817
----------------------------------- PR info ------------------------------------
Title      child_process: use signal.reason in child process abort (#47817)
Author     Debadree Chatterjee  (@debadree25)
Branch     debadree25:ft/child-process-abort -> nodejs:main
Labels     child_process, semver-major, author ready, needs-ci, commit-queue-squash
Commits    3
 - child_process: use signal.reason in child process abort
 - fixup! add another test
 - fixup! use cause
Committers 1
 - Debadree Chatterjee 
PR-URL: https://github.com/nodejs/node/pull/47817
Fixes: https://github.com/nodejs/node/issues/47814
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/47817
Fixes: https://github.com/nodejs/node/issues/47814
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 02 May 2023 11:22:01 GMT
   ✔  Approvals: 2
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/47817#pullrequestreview-1409488231
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/47817#pullrequestreview-1415776719
   ✘  semver-major requires at least 2 TSC approvals
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-05-04T04:48:59Z: https://ci.nodejs.org/job/node-test-pull-request/51618/
- Querying data for job/node-test-pull-request/51618/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/4902048348

@debadree25
Copy link
Member Author

Ah requires another TSC approval could someone from @nodejs/tsc take a look!

Thanks!

@debadree25 debadree25 removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label May 6, 2023
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@benjamingr
Copy link
Member

benjamingr commented May 7, 2023

As this just adds .reason as .cause and doesn't replace the AbortError this also doesn't need to land as semver-major as it doesn't change error codes but rather just adds another property on the exception

@@ -787,7 +787,7 @@ function spawn(file, args, options) {
}

function onAbortListener() {
abortChildProcess(child, killSignal);
abortChildProcess(child, killSignal, options.signal.reason);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: accessing options.signal.reason can potentially throw though I don't think we guard against that anywhere else to be honest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, why does it throw tho? I have never seen it like that, is there any way to guard?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be a getter that throws. To guard it against that, we would need a try/catch:

let reason;
try { ({ reason } = options.signal); } catch {}

abortChildProcess(child, killSignal, reason);

If we don’t guard against that anywhere else, it’s probably OK to ignore for this PR, but we should do it in a follow up PR IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm i check no where else it guarded maybe we could do a larger refactor in a followup

@debadree25
Copy link
Member Author

As this just adds .reason as .cause and doesn't replace the AbortError this also doesn't need to land as semver-major as it doesn't change error codes but rather just adds another property on the exception

Oh ok so removing the semver-major then @benjamingr

@debadree25 debadree25 removed the semver-major PRs that contain breaking changes and should be released in the next major version. label May 7, 2023
debadree25 and others added 2 commits May 7, 2023 22:02
Co-authored-by: Darshan Sen <raisinten@gmail.com>
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still lgtm

@nodejs-github-bot
Copy link
Collaborator

@debadree25 debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2023
@nodejs-github-bot nodejs-github-bot merged commit 2dc4290 into nodejs:main May 8, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2dc4290

targos pushed a commit that referenced this pull request 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 pull request 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 pull request 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
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

child_process.exec(...) (and fork, spawn) do not abort with expected signal.reason
5 participants