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: support Symbol.dispose #48551

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Jun 25, 2023

No description provided.

@MoLow MoLow requested a review from benjamingr June 25, 2023 22:17
@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 Jun 25, 2023
@MoLow MoLow changed the title child_process: support Symbol.asyncDestroy child_process: support Symbol.asyncDispose Jun 26, 2023
@MoLow MoLow requested a review from ronag June 26, 2023 05:23
@@ -516,6 +520,15 @@ ChildProcess.prototype.kill = function(sig) {
return false;
};

ChildProcess.prototype[SymbolAsyncDispose] = function() {
return new Promise((resolve) => {
this.on('exit', () => resolve(null));
Copy link
Member

Choose a reason for hiding this comment

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

What if already exited?

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the user remove all exit event handlers?

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

ronag commented Jun 26, 2023

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

ChildProcess.prototype[SymbolAsyncDispose] = async function() {
  if (!this.closed) {
    await EE.once(this, 'close');
  }
};

@MoLow
Copy link
Member Author

MoLow commented Jul 2, 2023

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
Copy link
Member Author

MoLow commented Jul 2, 2023

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

ChildProcess.prototype[SymbolAsyncDispose] = async function() {
  if (!this.closed) {
    await EE.once(this, 'close');
  }
};

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

@MoLow MoLow requested a review from ronag July 2, 2023 07:55
lib/internal/child_process.js Outdated Show resolved Hide resolved
lib/internal/child_process.js Outdated Show resolved Hide resolved
lib/internal/child_process.js Outdated Show resolved Hide resolved
lib/internal/child_process.js Outdated Show resolved Hide resolved
lib/internal/child_process.js Outdated Show resolved Hide resolved
lib/internal/child_process.js Outdated Show resolved Hide resolved
const ret = this.kill(this[kKillSignal]);
assert(ret, 'unexpected kill failure');
await EventEmitter.once(this, 'close');
this.emit('error', new AbortError());
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@ronag ronag Jul 2, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Let's ask for @ronbuckton 's advice:

The question here is:

await using cp = childProcess1;

// someOtherCode

doSomethingWith(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
Copy link
Member

ronag commented Jul 2, 2023

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

@MoLow
Copy link
Member Author

MoLow commented Jul 2, 2023

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
Copy link
Member

ronag commented Jul 2, 2023

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

@ronag
Copy link
Member

ronag commented Jul 2, 2023

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
Copy link
Member

ronag commented Jul 3, 2023

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
Copy link
Member

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
Copy link
Member

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 Outdated Show resolved Hide resolved
@MoLow MoLow changed the title child_process: support Symbol.asyncDispose child_process: support Symbol.dispose Jul 4, 2023
@MoLow MoLow requested a review from ronag July 4, 2023 19:43
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow 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 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 nodejs-github-bot merged commit 773fde2 into nodejs:main Jul 5, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 773fde2

juanarbol pushed a commit that referenced this pull request Jul 13, 2023
PR-URL: #48551
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@juanarbol juanarbol mentioned this pull request Jul 13, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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
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
PR-URL: #48551
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 11, 2023
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
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
PR-URL: #48551
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@MoLow MoLow deleted the child-process-destry branch May 24, 2024 09:01
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. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants