Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Minimal execSync API Proposal #5358

Closed
isaacs opened this issue Apr 23, 2013 · 19 comments
Closed

Minimal execSync API Proposal #5358

isaacs opened this issue Apr 23, 2013 · 19 comments
Assignees

Comments

@isaacs
Copy link

isaacs commented Apr 23, 2013

A few people have done a few passes at implementing execSync in such a way as to get maximum feature parity with child_process.spawn(), while being synchronous. However, this is rather challenging, since it requires a lot of complexity to separate out stdio data, etc.

I think that this is mostly unnecessary, and despite admirable efforts, has proven very challenging, and what most people really want is just something like the back-tick operator from PHP or Bash. So, here's a radically simplified proposal:

var result = execFileSync('ls', ['-laF', 'some/dir/']);
var result = execSync('ls -laF some/dir/');

Normally, this should return just the stdout results. If the user wants to get stderr as well, or the status code, perhaps an optional argument could cause it merge stderr and stdout together, or return an object {output:String, code:Integer}.

@isaacs
Copy link
Author

isaacs commented Apr 23, 2013

cc: @piscisaureus @bmeck

@isaacs
Copy link
Author

isaacs commented Apr 23, 2013

If we keep with the back-tick or $(...) style of behavior, then stderr would be inherited from the parent, and stdout is what's returned, unless the user explicitly redirects stderr to be merged with stdout.

@trevnorris
Copy link

So unless I'm understanding incorrectly it seems that the only time this would throw would be on improper argument format. Any other errors by the OS would be returned through the function. Is this correct?

@isaacs
Copy link
Author

isaacs commented Apr 24, 2013

It would throw if the fork or exec failed. Basically, the cases where the spawned child process would emit('error') today.

@trevnorris
Copy link

ok. then otherwise you're suggesting every return basically comes in the general format of { status: Integer, output: String }.

@myrne
Copy link

myrne commented Apr 24, 2013

First of all, I don't know the history of execSync. I just want to share some thoughts on this; hopefully constructive.

I recently wanted to have a promise-based variant on child_process.exec. Adapting an async function to become a promise means that you can only have a single result or a single error object. Much similar to a sync function that either returns a result, or throws an error object. child_process.exec does not conform to this rule. If sucessful, there are effectively two results: stdout and stderr. And also, if there's an error, there can still be data in the stdout and stderr arguments. Hence a need for some adaptation.

In faithful-exec I let the result if successful be an object with stdout and stderr properties, and if it fails, I let it add stdout and stderr properties be added to the error object that's given.

I think there's something to be said for the simplicity of only returning stdout, but writing execSync('ls -laF some/dir/').stdout is not that much harder, and keeps flexibility.

In any case, I think it's very handy to always have access to stdout and stderr, even in case of an error.

I kind of like the idea of not considering a non-zero exit code as an error. It's somewhat similar to what most would expect from a (low-level) http library: Error should only happen in case something goes wrong on the wire, not if response to the request happened to be 5xx or 4xx or so. Having a function fail in these cases is more suitable for a higher level abstraction.

However, it wouldn't be that consistent with current exec behavior. I like that - at least most of the time - there's a quite obvious mapping between how abc and abcSync works. I'd personally would like to see any execSync to mimic current exec behavior as much as possible. Both exec and execSync could use a more low-level variant (in the sense described in previous paragraph). Perhaps this behavior could also be activated by setting an option.

@isaacs
Copy link
Author

isaacs commented Apr 24, 2013

@meryn Well, to keep it simple, we can't reasonably return both stdout and stderr separately, since that requires a separate event loop, etc, so that we can poll on both file descriptors. We should just pass 2 as the stderr fd, and collect the output from stdout, like the ``` operator does in PHP and bash.

@isaacs
Copy link
Author

isaacs commented Apr 24, 2013

The tricky thing then is getting the exit code. In bash/sh, you have to access that via the $? special variable, and I'm not even sure what PHP has for that.

@myrne
Copy link

myrne commented Apr 24, 2013

We should just pass 2 as the stderr fd

I'm not sure what you mean by this. What are the implications of this?

I can somewhat understand that people would like to have an easy way to do "shell scripting" with Node, but I do wonder if execSync would be the right name for it then. Most *sync functions behave very similar to their regular (async) counterparts.
In my mind, it could even warrant a different module. Call it "shell" or so?

Assuming a different module, this module could function quite differently from others. Aside from an exec function that works like you describe, there could be a getLastExitCode, returning the equivalent of $? in bash. Maybe also add support for configuring the environment once, for all commands to be run.

@myrne
Copy link

myrne commented Apr 24, 2013

PHP has following api: string exec ( string $command [, array &$output [, int &$return_var ]] )
& denotes a variable passed by reference.
http://php.net/manual/en/function.exec.php

@OrangeDog
Copy link

Please, please don't draw inspiration from PHP.
Issac's original proposal sounds just fine.

@isaacs
Copy link
Author

isaacs commented Apr 25, 2013

Drawing inspiration from php is not so bad. We do a lot, and I think is actually fine, if it's done responsibly -- the opposite of stupid is not smart, and like most platforms, PHP has a lot of good ideas buried in the muck. Certainly, backtick operators are a really nice way to do execSync. I spent a lot of years writing PHP professionally (from php 3.something through php 5.2) so I feel somewhat qualified to evaluate its good parts and bad parts :)

However, even if we wanted to borrow php's exec function, we can't pass ints byref in javascript, and mutating array arguments is kind of just an oddball thing to do in JS, so that isn't really an option.

We should just pass 2 as the stderr fd
I'm not sure what you mean by this. What are the implications of this?

It would mean that the stderr of the child goes to the parent's stderr. (Ie, like spawn(c,a,{stdio:['pipe','pipe','inherit']}))

@OrangeDog
Copy link

All the good parts were taken from languages that use them better. Backticks for example from bash and perl.
That exec signature however, is clearly based on C, and has no place in a JavaScript library.

@myrne
Copy link

myrne commented Apr 25, 2013

To me, this all seems really high-level. Useful, but high-level. I think it would be better to aim for a low-level execSync function that functions as much as possible as exec. If that is too hard too implement quickly, "since that requires a separate event loop, etc, so that we can poll on both file descriptors.", I think it's best to leave the execSync "slot" open for a while, and choose a different name for the functionality proposed. The people get their "shell scripting" facilitation, the project keeps its relative overall consistency.

@isaacs, I suggested returning both stdout and stderr separately, but you said

we can't reasonably return both stdout and stderr separately, since that requires a separate event loop, etc, so that we can poll on both file descriptors.

Is this "hard" in terms of how long it takes too write, or is it something that may never land in Node, because it adds too much complexity for too little benefit?

If it's the former, I'd be interested in at least researching it to see how much sense I can make of it. I think I stumbled upon something interesting here. Note that I don't have any experience with Node internals, or "uv" or so. This is plain curiosity on my side.

If it's the latter, I can understand the motivation behind making execSync an oddball much more. It's more or less forced by implementation details.

In a discussion thread about capturing stderr output in PHP, I saw a remark about redirecting a program's stderr output to a plain file first, then reading that after the program has finished. Could that be done internally by Node, or would you consider that to be too much of a kludge (or brittle?)?

@myrne
Copy link

myrne commented Apr 25, 2013

I think that if there was an execSync function (however called) as you described, the technique for capturing stderr I described could be done in user-land. Just a simple sync function that does a readFileSync after the execSync. It would require control over where stderr goes to though.

@tjfontaine
Copy link

landed in fa4eb47 and e8df267

@myrne
Copy link

myrne commented Feb 11, 2014

Of note: execSync and execFileSync both return just the output on stdout, like @isaacs's original proposal. They will throw if an error occurs.

spawnSync however, returns an object which includes process id, stdout, stderr, exit code and kill signal. If an error occurs, the function won't throw, but instead the returned object will have an extra error property holding this error.

@piscisaureus Thanks for making spawnSync possible!

@trevnorris
Copy link

I haven't had a chance to look at the code well. What happens if I "cat /dev/zero"?

@rlidwka
Copy link

rlidwka commented Feb 13, 2014

Thank you very much for making require('child_process').execSync('curl http://google.com/') possible, it's now so much easier to write a bad code. :P

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants