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

Feature request: ChildProcess 'spawn' event #35288

Open
zenflow opened this issue Sep 21, 2020 · 5 comments · May be fixed by #35369
Open

Feature request: ChildProcess 'spawn' event #35288

zenflow opened this issue Sep 21, 2020 · 5 comments · May be fixed by #35369

Comments

@zenflow
Copy link

@zenflow zenflow commented Sep 21, 2020

Is your feature request related to a problem? Please describe.

After calling child_process.spawn, I'd like to know when the child process has successfully spawned and there's no longer the possibility of an 'error' event from failing to spawn (i.e. error type # 1 in the docs for that 'error' event, e.g. EPERM, ENOENT).

Currently I just wait 100 milliseconds (after calling child_process.spawn), and if the child process hasn't emitted an 'error' event by that time, I assume it spawned successfully. My code looks something like this:

const { spawn } = require('child_process');
const { promisify } = require('util');
const { once } = require('events');
const timeout = promisify(setTimeout);

async function doSomethingWithChildProcess(){
  const subprocess = spawn(...spawnArgs);
  try {
    await Promise.race([
      timeout(100),
      once(subprocess, 'error').then(([error]) => Promise.reject(error))
    ]);
  } catch (error) {
    // handle error spawning child process
    return;
  }
  // do something with the running child process...
}

This seems to work, but I'm not sure how reliable it is, and anyways, it is certainly a hack.
I'm wondering if there could be a better (more correct) way..

Describe the solution you'd like

Is there some point of execution in Node where we know there was no error spawning the child process?

If so, we could introduce a new 'spawn' event for the ChildProcess class, to be emitted at that point in execution?

That would remove the need for the unreliable hack I described above.

It would also work nicely with Node.js's events.once function. For example, the code from above could be updated like this:

const { spawn } = require('child_process');
-const { promisify } = require('util');
const { once } = require('events');
-const timeout = promisify(setTimeout);

async function doSomethingWithChildProcess(){
  const subprocess = spawn(...spawnArgs);
  try {
-    await Promise.race([
-      timeout(100),
-      once(subprocess, 'error').then(([error]) => Promise.reject(error))
-    ]);
+    await once(subprocess, 'spawn');
  } catch (error) {
    // handle error spawning child process
    return;
  }
  // do something with the running child process...
}

Describe alternatives you've considered

Just the hack I described above (of expecting any error spawning to happen within 100 milliseconds of calling child_process.spawn).

I can't think of any other possible solutions at this time.

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Sep 22, 2020

It's not hard to implement, see diff. Feel free to steal for a PR, no attribution needed. Needs docs and tests, however.

diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
index 31d9f02df3..5672f3429f 100644
--- a/lib/internal/child_process.js
+++ b/lib/internal/child_process.js
@@ -400,6 +400,8 @@ ChildProcess.prototype.spawn = function(options) {
     this._handle.close();
     this._handle = null;
     throw errnoException(err, 'spawn');
+  } else {
+    process.nextTick(onSpawnNT, this);
   }
 
   this.pid = this._handle.pid;
@@ -465,6 +467,11 @@ function onErrorNT(self, err) {
 }
 
 
+function onSpawnNT(self) {
+  self.emit('spawn');
+}
+
+
 ChildProcess.prototype.kill = function(sig) {
 
   const signal = sig === 0 ? sig :

Apropos this:

Is there some point of execution in Node where we know there was no error spawning the child process?

Yes, but there's a subtlety that is likely to trip up some people (mostly the magical thinker kind): while bash bob spawns successfully, bash then fails to spawn bob. You'll still get a spawn event.

That caveat also applies to { shell: true } and it seems to be a perennial source of confusion to novice users who expect it to DWIM rather than do what the documentation says it does.

@kaushik94
Copy link

@kaushik94 kaushik94 commented Sep 22, 2020

@bnoordhuis can I take this up?

@zenflow
Copy link
Author

@zenflow zenflow commented Sep 22, 2020

I'd also like to claim this issue 😄 @kaushik94 Would that be ok with you?

@kaushik94
Copy link

@kaushik94 kaushik94 commented Sep 22, 2020

@zenflow I'm sorry, I thought you were a maintainer. Sure you claim the issue as you raised the issue. Thanks, looking forward!

@zenflow
Copy link
Author

@zenflow zenflow commented Sep 22, 2020

Thanks @kaushik94 😃

zenflow added a commit to zenflow/node that referenced this issue Sep 26, 2020
The new event signals that the subprocess has spawned successfully and
no 'error' event will be emitted from failing to spawn.

This change is accompanied by a new test for the logical ordering of
events emitted by a typical ChildProcess object.

Fixes: nodejs#35288
zenflow added a commit to zenflow/node that referenced this issue Sep 27, 2020
The new event signals that the subprocess has spawned successfully and
no 'error' event will be emitted from failing to spawn.

Fixes: nodejs#35288
@zenflow zenflow linked a pull request that will close this issue Sep 27, 2020
3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.