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

Add Promise-based versions of child_process #38823

Closed
thernstig opened this issue May 27, 2021 · 33 comments
Closed

Add Promise-based versions of child_process #38823

thernstig opened this issue May 27, 2021 · 33 comments
Labels
child_process Issues and PRs related to the child_process subsystem. feature request Issues that request new features to be added to Node.js. promises Issues and PRs related to ECMAScript promises.

Comments

@thernstig
Copy link
Contributor

thernstig commented May 27, 2021

Is your feature request related to a problem? Please describe.
It would be nice if child_process functionality, such as exec, had Promised based versions by default. Currently it does not, see https://nodejs.org/api/child_process.html

Describe the solution you'd like
A new child_process/promises import path that can be used similar to how fs promises are used:

// Using ESM Module syntax:
import { exec } from 'child_process/promises';

try {
  const { stdout } = await exec(
    'sysctl -n net.ipv4.ip_local_port_range'
  );
  console.log('successfully executed the child process command');
} catch (error) {
  console.error('there was an error:', error.message);
}

Describe alternatives you've considered
I am already using const exec = util.promisify(process.exec); but that is not as nice. And this new proposal follows along what is happening in other Node.js APIs.

@Ayase-252 Ayase-252 added child_process Issues and PRs related to the child_process subsystem. promises Issues and PRs related to ECMAScript promises. feature request Issues that request new features to be added to Node.js. labels May 27, 2021
@Trott
Copy link
Member

Trott commented May 27, 2021

@nodejs/child_process

@thernstig thernstig changed the title Add Promise-based versions of Child process (child_process) Add Promise-based versions of child process (child_process) May 28, 2021
@thernstig thernstig changed the title Add Promise-based versions of child process (child_process) Add Promise-based versions of child_process May 28, 2021
@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 24, 2022
@thernstig
Copy link
Contributor Author

Unstale since it has 6 upvotes and makes great sense.

@bnoordhuis
Copy link
Member

See discussion in #41964, #41964 (comment) in particular. Thanks for the suggestion but closing as a wontfix.

@thernstig
Copy link
Contributor Author

@bnoordhuis please re-open this issue. There are parts that can be given async versions directly, as indicate by Node.js documentation.

Please see https://nodejs.org/api/child_process.html#child_processexeccommand-options-callback:

If this method is invoked as its util.promisify()ed version, it returns a Promise for an Object with stdout and stderr properties.

Ergo the feature request is about not having to do e.g. const exec = util.promisify(require('child_process').exec); like is given as an example in your own documentation.

@bnoordhuis
Copy link
Member

Can you explain how your feature request is materially different from the one in #41964?

I'll grant you that child_process methods can be promisified. The discussion in the other issue was about whether they should (conclusion: no.)

@thernstig
Copy link
Contributor Author

thernstig commented Mar 28, 2022

@bnoordhuis fork has no callback but exec does, so exec can be promisified. You mention that:

Node's child process APIs are all wrappers around uv_spawn() and uv_spawn() is synchronous.

As exec can be promisified, even though it is not effectively async it is possible and reads nicers to user using async/await.

One of the big gains of async/await is that reads as sync code (no callbacks). So supplying a pre-promisified version means users do not have to do this extra step themselves, no matter if it is sync or not.

edit: One of the great things with Node.js is that it has taken lots of care to give easy-to-read APIs and I think any minor wins in this regards benefits the community. The famous "you read a 100 more times than you write code".

@bnoordhuis
Copy link
Member

Okay, reopened - which isn't the same as accepted. 10 months with no activity suggests there is little interest.

@bnoordhuis bnoordhuis reopened this Mar 28, 2022
@targos
Copy link
Member

targos commented Mar 28, 2022

I think @aduh95 is working on something.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Sep 26, 2022
@aduh95
Copy link
Contributor

aduh95 commented Sep 26, 2022

I suffer some data lost and don't have my WIP commits anymore (I know, I know, I should have pushed them to my fork), but it still on my list.

@thernstig
Copy link
Contributor Author

@aduh95 great ❤️ Can you unstale this? cc @targos

@github-actions github-actions bot removed the stale label Sep 27, 2022
@bnoordhuis
Copy link
Member

It's been 1.5 years without movement so I'm going to close this. Pull request still welcome, of course.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Dec 29, 2022
@thernstig
Copy link
Contributor Author

thernstig commented Dec 29, 2022

@bnoordhuis that is unfortunate, considering it at least has quite a few upvotes. Lack of progress does not indicate lack of interest. It was on the second page of most upvoted features, even though being relatively new. Many projects do not close tickets with X amount of upvotes, maybe something to follow?

@aduh95
Copy link
Contributor

aduh95 commented Dec 29, 2022

Reopening since it’s still wanted.

Somewhat related: #45774

@aduh95 aduh95 reopened this Dec 29, 2022
@bnoordhuis
Copy link
Member

Lack of progress does not indicate lack of interest

Seems like a pretty good proxy to me. I don't want to turn this into an open/close war but if sufficiently many people thought it was a worthwhile feature, it would've happened by now.

@aduh95 Unless you plan on working on this in the near future, I suggest you close this again. At 1,300+ open issues we need to be a little more aggressive pruning dead stuff.

@thernstig
Copy link
Contributor Author

thernstig commented Dec 30, 2022

@bnoordhuis I do not agree, but I am also not a maintainer so it is my subjective opinion. I respect yours of course. My two cents about this is that most Node.js APIs are moving towards allowing promised-based versions. Adding this to child_process shows a thought of coherence for the Node.js project, and show quality to me.

I understand getting someone to work on it might be hard, but to me that is never a reason to close something if it is a valid request, which I think this is due to the aforementioned reasons. Being amongst the top 50 upvoted features shows it is wanted.

With the same reasoning, one could decide to close every other feature (not bugs) that are not in the 50 most upvoted features as no one has yet to work on them.

(Please mark this as off topic is needed, that is perfectly fine to me and up to your discretion).

@bnoordhuis
Copy link
Member

Closed doesn't mean rejected. The bug tracker is for maintainers first and foremost, and in its current state it's useless to me. I never look beyond the first 1 or 2 pages.

@danielbayley
Copy link
Contributor

danielbayley commented Jan 5, 2023

I just blindly tried:

import { exec } from 'node:child_process/promises'

based on previous use of the same syntax for fs, expecting it to work…
Surely it should be included for a more consistent API?

@aduh95
Copy link
Contributor

aduh95 commented Jan 5, 2023 via email

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2023

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Jul 4, 2023
@thernstig
Copy link
Contributor Author

Wish the github-actions did not try to close features with 20+ upvotes

@github-actions github-actions bot removed the stale label Jul 5, 2023
@bnoordhuis
Copy link
Member

I'm giving this issue a few more months but if nothing happens, I'm going to close it out again, upvotes or not.

@mifi
Copy link
Contributor

mifi commented Sep 26, 2023

We might be able to take inspiration from the popular npm module execa when designing / implementing a more convenient promise based child_process api. I've used it for many years and it's a breeze to work with.

@bnoordhuis
Copy link
Member

Oh hey, this issue again. Coming up at the 2.5 year mark.

So, I think the initial ask here is based on the mistaken assumption that promise == async == non-blocking / work done in background. Read #41964 (comment) to dispel yourself of that illusion.

The post facto counterargument offered in #38823 (comment) is unconvincing: "okay, maybe it's not really async but it reads nicer."

No one (to my utter lack of surprise) has offered up:

  1. a credible sketch of what a promise-based API should look like ("be like execa" doesn't cut it), and
  2. a good analysis of the material benefits a new API offers over the existing child_process module

@aduh95 tried (1) and concluded you end up sacrificing functionality. My answer to (2) is "practically nil."

Really the only argument people have advanced for keeping this issue open is "but it has many upvotes!"

Good thing I'm not swayed by popularity contests. Closing.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Sep 26, 2023
@mifi
Copy link
Contributor

mifi commented Sep 27, 2023

So, I think the initial ask here is based on the mistaken assumption that promise == async == non-blocking / work done in background. Read #41964 (comment) to dispel yourself of that illusion.

Correct me if I'm wrong but I don't think what OP in this issue (#38823) is asking is for an async version of uv_spawn. I think he's asking about:

  1. child_process.fork and child_process.spawn should have a callback that's called with the output when process terminates, just like child_process.exec has.
  2. All of these 3 function should have a corresponding child_process/promises counterpart where the callback has been removed and the function instead returns something like a ChildProcess+Promise hybrid (or an object containing { promise, process }), where the promise will resolve or reject once the process terminates, and stdout,stderr should be accessible in some easy way, like for example properties on the resolved object or properties on the rejected Error object.

In addition to that, I would personally like to see the following:

  1. If the process returns a non-zero exit code, the promise should be rejected.
  2. a new timeout property that when set, will send a SIGKILL to the process after the process has run for timeout ms. The error object should the contain a boolean timedOut: true

This would allow the user to do something like this:

import { exec } from 'child_process/promises';

try {
  const { stdout, stderr } = await spawn('ls', ['/']);
  console.log('child process finished successfully and the result was:', stdout);
} catch (err) {
  console.error('Process exited with code', err.exitCode, err);
  console.error('stderr was:', error.stderr)
}

@bnoordhuis
Copy link
Member

child_process.fork and child_process.spawn should have a callback that's called with the output when process terminates, just like child_process.exec

I've read through the comments again but I really don't know how you came to that conclusion... and anyway, that misses the point of fork() and spawn(). If you want exec-like behavior, use exec().

If the process returns a non-zero exit code, the promise should be rejected.

Bad design choice. Many child processes terminate with a non-zero exit code - but in a benign fashion - after e.g. SIGPIPE or ^C.

a new timeout property that [..]

Already exists.

@thernstig
Copy link
Contributor Author

Honestly the ask is basically that child_process/promises could just return a variant such as const exec = util.promisify(process.exec); as a conventience. To then already be used like the already existing code examples of this from https://nodejs.org/api/child_process.html#child_processexeccommand-options-callback:

// Using ESM Module syntax:
import { exec } from 'child_process/promises';

try {
  const { stdout } = await exec(
    'sysctl -n net.ipv4.ip_local_port_range'
  );
  console.log('successfully executed the child process command');
} catch (error) {
  console.error('there was an error:', error.message);

Ergo exec() can already be used with async/await but needs and extra promisify. I cannot grok still from this thread why child_process/promises could just do this wrapping and export it.

@bnoordhuis
Copy link
Member

It's better if you split that out into a new issue. A promise-ified child_process.exec may be useful (or maybe not, I'm reserving judgment) but this issue is about promise-ifying the child_process module wholesale. Different goalposts.

@thernstig
Copy link
Contributor Author

@bnoordhuis I do not think I ever put expectations on the entire module to be promisified. The intention was to make the functions that are feasible into async/await, such as exec.

I explicitly gave an example of the fs module which has undergone the same journey. I.e. if you look at https://nodejs.org/docs/latest-v10.x/api/fs.html there were only very few promise-based versions as compared to https://nodejs.org/api/fs.html which now has many. But it hard to start from somewhere.

This topic was about even creating child_process/promises to start with the ones that can be promisified. That might be exec for now, but maybe more in the future. I do not know, it has to be done for whatever makes sense.

But for anything to even happen a child_process/promises module is a prerequisite.

Do you still want a new issue even though this seems to be an unclear ask from me from the start, that obviously everyone has read differently into.

@bnoordhuis
Copy link
Member

I do not think I ever put expectations on the entire module to be promisified.

It's right there in the title.

The intention was to make the functions that are feasible into async/await, such as exec.

Sure, that's a more reasonable ask. If you'd been clear about that from the start, that would've saved a lot of back and forth.

The reason you can util.promisify(child_process.exec) and execFile but not spawn or fork is because people already thought through the use cases, concluded promises make sense for the former but not the latter, and added the appropriate hooks (the child_process.exec[util.promisify.custom] hook.)

You're welcome to open a pull request that adds a child_process/promises module and see how it's received.

@thernstig
Copy link
Contributor Author

I do not think I ever put expectations on the entire module to be promisified.

It's right there in the title.

It never goes into the extent. The title does not mention anything about the entire module being promisfied. Multiple posts follow up talking about exec only. I can amend the title/description even more, but closing it and opening a new one will lose upvotes and thus people tracking this. But I did it anyway to stop the fastidious back and forth: #49904

@dead-claudia
Copy link

Got a (mostly) concrete proposal in #54799.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. feature request Issues that request new features to be added to Node.js. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

No branches or pull requests

9 participants