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

spawn error doesn't utilize event handlers #990

Closed
ORESoftware opened this issue Nov 25, 2017 · 3 comments
Closed

spawn error doesn't utilize event handlers #990

ORESoftware opened this issue Nov 25, 2017 · 3 comments

Comments

@ORESoftware
Copy link

ORESoftware commented Nov 25, 2017

Nodejs version 6.10.x

I have this:

const file = path.resolve(__dirname + '/foo.js');
const  n = cp.spawn(file, []);  // EACCES error is thrown here

n.once('error', function () {
  // this does not get hit !!
});

so it seems like I should do this instead (but the try/catch seems unfortunate):

let n;

try{
  n = cp.spawn(file, []);  //  EACCES error is thrown here
}
catch(err){
  return cb(err);
}

n.once('error', function () {
  // this does not get hit !!
});

seems like the 'error' handler should be given the chance to capture error, instead of necessitating a try/catch, am I doing something wrong?

Perhaps because Node core does not even create the n child process object, that seems like an implementation "mistake".


HOW TO REPLICATE:

Create a file without executable permissions, then try to execute it directly:

#!/usr/bin/env bash
touch my-non-executable-file.js
./my-non-executable-file.js

so that would be something like:

const k = cp.spawn('/foo/bar/my-non-executable-file.js');

the above should throw an error, instead of using k.on('error', fn)

@gireeshpunathil
Copy link
Member

one can argue that n represents a child process and in this case as the process was never created, n not being fully formed makes sense.

Technically it may be possible to first compose a JS object that is going to abstract the child process that is being spawned. But a more natural way of doing this is to materialize the entity first, and then abstract the entity. (the entity here being the native process).

Is it against your convictions?

@ORESoftware
Copy link
Author

ORESoftware commented Nov 25, 2017

eh, because the child process was never "started", I can't use an 'exit' handler on the child process, which forces me to rethink my own program design. It seems like a child was started in reality, but I don't know the internals that well. I don't see why you'd ever have to put a try/catch around spawn. IMO a better design would be to always use n.on('error').

@bnoordhuis this seems like a design mistake in node core. Having to put try/catch around

try{
cp.spawn('node')
}
catch(err){
}

seems wrong :) I hate try/catch lol

@gireeshpunathil
Copy link
Member

this block of code defines what is runtime error and what is a linkage error, and the current list does not contain UV_EACCES. If I add this patch the above code runs fine:

--- a/lib/internal/child_process.js
+++ b/lib/internal/child_process.js
@@ -26,7 +26,8 @@ const {
   UV_ENFILE,
   UV_ENOENT,
   UV_ENOSYS,
-  UV_ESRCH
+  UV_ESRCH,
+  UV_EACCES
 } = process.binding('uv');
 
 const errnoException = errors.errnoException;
@@ -308,6 +309,7 @@ ChildProcess.prototype.spawn = function(options) {
   if (err === UV_EAGAIN ||
       err === UV_EMFILE ||
       err === UV_ENFILE ||
+      err === UV_EACCES ||
       err === UV_ENOENT) {
     process.nextTick(onErrorNT, this, err);
     // There is no point in continuing when we've hit EMFILE or ENFILE

question is on the protocol definition of runtime error and whether UV_EACCES comes under its purview or not.

/cc @nodejs/child_process

gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Mar 12, 2018

Verified

This commit was signed with the committer’s verified signature.
hediet Henning Dieterichs
Access permission on the target child is currently thrown
as an exception. Bring this under the runtime error definition,
much like ENOENT and friends.

Fixes: nodejs/help#990
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants