-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Comments
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:
Yes, but there's a subtlety that is likely to trip up some people (mostly the magical thinker kind): while That caveat also applies to |
@bnoordhuis can I take this up? |
I'd also like to claim this issue 😄 @kaushik94 Would that be ok with you? |
@zenflow I'm sorry, I thought you were a maintainer. Sure you claim the issue as you raised the issue. Thanks, looking forward! |
Thanks @kaushik94 😃 |
The new event signals that the subprocess has spawned successfully and no 'error' event will be emitted from failing to spawn. Fixes: nodejs#35288
this this event never fire sometimes? the docs say "if emitted" but don't specify why it wouldn't be. It is sometimes not emitted when things to smoothly (on windows, i am doing |
Making this clarification in response to a comment on GitHub: nodejs#35288 (comment)
@alexp1917 Yeah I agree the docs could be more explicit about the case where this event does not fire. Please see my PR and let me know if it makes things perfectly clear: https://github.com/nodejs/node/pull/37833/files
If the answer to both of these questions is "yes", then I think we may have a bug in this feature.. |
The answer to the second question is yes I will have to double check on the
1st
…On Sat, Mar 20, 2021, 5:25 AM Matthew Francis Brunetti < ***@***.***> wrote:
@alexp1917 <https://github.com/alexp1917> Yeah I agree the docs could be
more explicit about the case where this event does *not* fire. Please see
my PR and let me know if it makes things perfectly clear:
https://github.com/nodejs/node/pull/37833/files
@alexp1917 <https://github.com/alexp1917>
- Are you always using a supported Node.js version? This event was
only added as of v15.1.0.
- Is the 'error' event not firing in these cases where 'spawn' is not
fired? Either a 'spawn' event or an 'error' event should fire, every
time.
If the answer to both of these questions is "yes", then I think we may
have a bug in this feature..
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#35288 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/APYQZL2G6NKGXU7CFGX7QDDTERSY5ANCNFSM4RVB67YQ>
.
|
Making this clarification in response to a comment on GitHub: #35288 (comment) PR-URL: #37833 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Making this clarification in response to a comment on GitHub: #35288 (comment) PR-URL: #37833 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Making this clarification in response to a comment on GitHub: #35288 (comment) PR-URL: #37833 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
I also have a case of missing 'spawn' event but only in certain circonstances. With node v15 on macos, it works However, if I run the spawn command from an electron main process on the windows vm, So I ended up adding a terrible setTimeout to resolve automatically if no error occured. Side notes, I have tried spawning .exe files and .bat files or linux programs such as 'ls' and it doesn't change what I've said above. |
This comment was marked as resolved.
This comment was marked as resolved.
Note: I accidentally hid my previous comment - this is an unhidden copy of that comment
I seem to experience this issue too - the 'error' event never fires and my child process seems to spawn succesfully (judging by the output on my console) but the 'spawn' event also never fires. Or maybe it fires before the event handler is registered, but either way - the above code does not work for me. I've managed to 'fix' this issue with a bit of a hack - With the absence of the 'error' event getting thrown, I'm running a while-loop until
|
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: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: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.
The text was updated successfully, but these errors were encountered: