child_process: use signal.reason in child process abort by debadree25 · Pull Request #47817 · nodejs/node

debadree25 · 2023-05-02T11:22:01Z

aduh95 · 2023-05-02T12:16:26Z

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 · 2023-05-02T12:33:28Z

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 · 2023-05-02T13:05:40Z

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 · 2023-05-02T14:06:01Z

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 · 2023-05-02T14:56:23Z

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

Thanks!

nodejs-github-bot · 2023-05-02T17:25:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51583/

nodejs-github-bot · 2023-05-03T10:40:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51599/

nodejs-github-bot · 2023-05-03T17:53:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51611/

nodejs-github-bot · 2023-05-04T04:48:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51618/

nodejs-github-bot · 2023-05-06T14:17:58Z

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 · 2023-05-06T16:46:29Z

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

Thanks!

RaisinTen

LGTM

test/parallel/test-child-process-exec-abortcontroller-promisified.js

benjamingr · 2023-05-07T06:54:41Z

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

benjamingr · 2023-05-07T06:55:53Z

lib/child_process.js

    function onAbortListener() {
-      abortChildProcess(child, killSignal);
+      abortChildProcess(child, killSignal, options.signal.reason);
    }


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

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

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.

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

debadree25 · 2023-05-07T12:05:43Z

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

Co-authored-by: Darshan Sen <raisinten@gmail.com>

lib/child_process.js

RaisinTen

still lgtm

nodejs-github-bot · 2023-05-08T06:50:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51696/

nodejs-github-bot · 2023-05-08T08:17:05Z

Landed in 2dc4290

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>

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>

debadree25 added 2 commits May 2, 2023 16:48

child_process: use signal.reason in child process abort

f2e3974

Fixes: nodejs#47814

fixup! add another test

29954c9

debadree25 requested a review from benjamingr May 2, 2023 11:22

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 added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 2, 2023

fixup! use cause

6fc91a2

benjamingr approved these changes May 2, 2023

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 2, 2023

github-actions bot mentioned this pull request May 4, 2023

CI Reliability 2023-05-04 nodejs/reliability#555

Open

28 tasks

This was referenced May 5, 2023

CI Reliability 2023-05-05 nodejs/reliability#556

Open

CI Reliability 2023-05-06 nodejs/reliability#557

Open

debadree25 requested a review from aduh95 May 6, 2023 07:59

aduh95 approved these changes May 6, 2023

View reviewed changes

debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2023

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

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

github-actions bot mentioned this pull request May 7, 2023

CI Reliability 2023-05-07 nodejs/reliability#558

Open

27 tasks

RaisinTen approved these changes May 7, 2023

View reviewed changes

test/parallel/test-child-process-exec-abortcontroller-promisified.js Outdated Show resolved Hide resolved

benjamingr reviewed May 7, 2023

View reviewed changes

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

fixup! update test

e26e572

Co-authored-by: Darshan Sen <raisinten@gmail.com>

fixup! update default exception

ece8b5e

aduh95 reviewed May 7, 2023

View reviewed changes

lib/child_process.js Show resolved Hide resolved

github-actions bot mentioned this pull request May 8, 2023

CI Reliability 2023-05-08 nodejs/reliability#559

Open

26 tasks

RaisinTen approved these changes May 8, 2023

View reviewed changes

debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2023

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 merged commit 2dc4290 into nodejs:main May 8, 2023

github-actions bot mentioned this pull request May 9, 2023

CI Reliability 2023-05-09 nodejs/reliability#560

Open

29 tasks

targos mentioned this pull request May 15, 2023

v20.2.0 release proposal #48020

Merged

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

Uh oh!

Conversation

debadree25 commented May 2, 2023

Uh oh!

aduh95 commented May 2, 2023

Uh oh!

debadree25 commented May 2, 2023

Uh oh!

debadree25 commented May 2, 2023

Uh oh!

benjamingr commented May 2, 2023

Uh oh!

debadree25 commented May 2, 2023

Uh oh!

nodejs-github-bot commented May 2, 2023

Uh oh!

nodejs-github-bot commented May 3, 2023

Uh oh!

nodejs-github-bot commented May 3, 2023

Uh oh!

nodejs-github-bot commented May 4, 2023

Uh oh!

nodejs-github-bot commented May 6, 2023

Uh oh!

debadree25 commented May 6, 2023

Uh oh!

RaisinTen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benjamingr commented May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamingr May 7, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 May 7, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 May 7, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 May 7, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 commented May 7, 2023

Uh oh!

Uh oh!

RaisinTen left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 8, 2023

Uh oh!

nodejs-github-bot commented May 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benjamingr commented May 7, 2023 •

edited

Loading