child_process: support `Symbol.dispose`#48551

MoLow · 2023-06-25T22:17:18Z

No description provided.

ronag · 2023-06-26T05:39:06Z

lib/internal/child_process.js


+ChildProcess.prototype[SymbolAsyncDispose]=function(){
+returnnewPromise((resolve)=>{
+this.on('exit',()=>resolve(null));


What if already exited?

What if the user remove all exit event handlers?

ronag

Isn't this changing a little too much? I think this PR could be more minimal.

lib/internal/child_process.js

ronag

I'm unsure whether it's sufficient to wait for 'exit' or 'close' is the more correct event to wait for? Not sure exactly what the difference is between these two.

ronag · 2023-06-26T06:51:24Z

Let's add a closed property to child process and use the following logic:

ChildProcess.prototype[SymbolAsyncDispose]=asyncfunction(){if(!this.closed){awaitEE.once(this,'close');}};

MoLow · 2023-07-02T07:19:40Z

I'm unsure whether it's sufficient to wait for 'exit' or 'close' is the more correct event to wait for? Not sure exactly what the difference is between these two.

according to the code, exit is emitted first when child process exits, then close is fired once all the streams are drained. so we should probably use close

MoLow · 2023-07-02T07:30:47Z

Let's add a closed property to child process and use the following logic:
ChildProcess.prototype[SymbolAsyncDispose]=asyncfunction(){if(!this.closed){awaitEE.once(this,'close');}};

we cannot use EE.once since it will always reject when calling abortChildProcess.

lib/internal/child_process.js

ronag · 2023-07-02T13:14:17Z

lib/internal/child_process.js

+constret=this.kill(this[kKillSignal]);
+assert(ret,'unexpected kill failure');
+awaitEventEmitter.once(this,'close');
+this.emit('error',newAbortError());


Why this? Not saying I disagree. Just not sure whether I do agree. @benjamingr any thoughts?

So other consumers of the child process know it was aborted. this is the same behavior we've implemented on Readable[Symbol.asyncDispose]

There is a race here though. The process might still exit successfully.

I am Not sure I follow. can you provide an example?

The process can succeed before it receives the kill. In which case you are emitting a bad error.

An abortion is just a hint. The process may still choose to fully complete.

I think this quickly becomes complicated. I'd prefer not to emit any error here.

it can be a surprising behavior for a child process to exit without a kill signal and a code, and with incomplete stdout/stderr

Let's ask for @ronbuckton 's advice:
The question here is:
awaitusingcp=childProcess1;// someOtherCodedoSomethingWith(childProcess1);
What should the behavior be from doSomethingWith's side? For example if someone is reading stdout while another consumer is disposing it should it error?

ronag · 2023-07-02T15:19:06Z

Any signal other than SIGKILL may actually be ignored by the process. Which would lead to a deadlock in our case.

MoLow · 2023-07-02T15:55:34Z

Any signal other than SIGKILL may actually be ignored by the process. Which would lead to a deadlock in our case.

a pending promise is not a deadlock

ronag · 2023-07-02T16:19:28Z

It is if you wait for it, expecting it to resolve and it never does

ronag · 2023-07-02T18:58:45Z

I'd like to have some more opinions on this. I feel there is a risk of this being an anti-pattern that we encourage.

@benjamingr @jasnell

ronag · 2023-07-03T09:13:24Z

Maybe we can get around the deadlock issue by always having a timeout which eventually calls sigkill and makes sure the promise will eventually resolve.

benjamingr · 2023-07-03T17:46:22Z

An alternative is to implement dispose and not asyncDispose, send the signal and wait for nothing. If someone wants to wait for the process to terminate they can do so by listening to the event themselves.

benjamingr · 2023-07-04T11:45:19Z

An alternative is to implement dispose and not asyncDispose, send the signal and wait for nothing. If someone wants to wait for the process to terminate they can do so by listening to the event themselves.

I think this is the only reasonable option given we don't know how to wait for the process to terminate "for sure"

doc/api/child_process.md

nodejs-github-bot · 2023-07-05T03:55:34Z

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

nodejs-github-bot · 2023-07-05T08:28:45Z

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

nodejs-github-bot · 2023-07-05T10:21:16Z

Landed in 773fde2

PR-URL: #48551 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: nodejs#48551 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #48551 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

MoLow requested a review from benjamingr June 25, 2023 22:17

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 Jun 25, 2023

MoLow changed the title ~~child_process: support Symbol.asyncDestroy~~child_process: support Symbol.asyncDisposeJun 26, 2023

MoLow force-pushed the child-process-destry branch from eb3ccaa to 50f076dCompare June 26, 2023 05:21

MoLow requested a review from ronag June 26, 2023 05:23

ronag requested changes Jun 26, 2023
View reviewed changes

aduh95 reviewed Jun 26, 2023
View reviewed changes

lib/internal/child_process.js Outdated Show resolvedHide resolved

ronag reviewed Jun 26, 2023
View reviewed changes

lib/internal/child_process.js Outdated Show resolvedHide resolved

ronag reviewed Jun 26, 2023
View reviewed changes

lib/internal/child_process.js Outdated Show resolvedHide resolved

ronag reviewed Jun 26, 2023
View reviewed changes

lib/internal/child_process.js Outdated Show resolvedHide resolved

ronag reviewed Jun 26, 2023
View reviewed changes

MoLow force-pushed the child-process-destry branch from de376ce to 034c37aCompare July 2, 2023 07:55

MoLow requested a review from ronag July 2, 2023 07:55

ronag requested changes Jul 2, 2023
View reviewed changes

lib/internal/child_process.js Outdated Show resolvedHide resolved
lib/internal/child_process.js Outdated Show resolvedHide resolved
lib/internal/child_process.js Outdated Show resolvedHide resolved
lib/internal/child_process.js Outdated Show resolvedHide resolved

ronag requested changes Jul 2, 2023
View reviewed changes

lib/internal/child_process.js Outdated Show resolvedHide resolved
lib/internal/child_process.js Outdated Show resolvedHide resolved

ronag reviewed Jul 2, 2023
View reviewed changes

ronag reviewed Jul 4, 2023
View reviewed changes

doc/api/child_process.md Outdated Show resolvedHide resolved

child_process: support Symbol.dispose
5d2af02

MoLow force-pushed the child-process-destry branch from 0b342e8 to 5d2af02Compare July 4, 2023 19:43

MoLow changed the title ~~child_process: support Symbol.asyncDispose~~child_process: support Symbol.disposeJul 4, 2023

MoLow requested a review from ronag July 4, 2023 19:43

ronag approved these changes Jul 5, 2023
View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2023

benjamingr approved these changes Jul 5, 2023
View reviewed changes

MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 5, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 5, 2023

nodejs-github-bot merged commit 773fde2 into nodejs:mainJul 5, 2023

juanarbol pushed a commit that referenced this pull request Jul 13, 2023
child_process: support Symbol.dispose
4e08160
PR-URL: #48551 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

juanarbol mentioned this pull request Jul 13, 2023
V20.5.0 proposal #48761
Merged

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
child_process: support Symbol.dispose
e03039e
PR-URL: nodejs#48551 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
child_process: support Symbol.dispose
80d09cb
PR-URL: nodejs#48551 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
child_process: support Symbol.dispose
f70b63e
PR-URL: #48551 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

ruyadorno mentioned this pull request Sep 11, 2023
v18.18.0 proposal #49220
Merged

ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
child_process: support Symbol.dispose
535de5a
PR-URL: #48551 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
child_process: support Symbol.dispose
611db8d
PR-URL: #48551 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

MoLow deleted the child-process-destry branch May 24, 2024 09:01

skyclouds2001 mentioned this pull request Apr 5, 2025
javascript.builtins.Symbol - Symbol.{dispose, asyncDispose} available in node18.18.0, deno1.37.0 and Safari 18.3 mdn/browser-compat-data#26216
Closed

Uh oh!

child_process: support Symbol.dispose#48551

child_process: support Symbol.dispose#48551

Uh oh!

Conversation

MoLow commented Jun 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Jun 26, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoLow commented Jul 2, 2023

Uh oh!

MoLow commented Jul 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronagJul 2, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronag commented Jul 2, 2023

Uh oh!

MoLow commented Jul 2, 2023

Uh oh!

ronag commented Jul 2, 2023

Uh oh!

ronag commented Jul 2, 2023

Uh oh!

ronag commented Jul 3, 2023

Uh oh!

benjamingr commented Jul 3, 2023

Uh oh!

benjamingr commented Jul 4, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 5, 2023

Uh oh!

nodejs-github-bot commented Jul 5, 2023

Uh oh!

nodejs-github-bot commented Jul 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

child_process: support `Symbol.dispose`#48551

child_process: support `Symbol.dispose`#48551

ronag commented Jun 26, 2023•
edited
Loading

ronagJul 2, 2023•
edited
Loading