nodejs / node-v0.x-archive

Moved to https://github.com/nodejs/node
34.44k stars 7.3k forks source link

Minimal execSync API Proposal #5358

Closed isaacs closed 10 years ago

isaacs commented 11 years ago

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 commented 11 years ago

cc: @piscisaureus @bmeck

isaacs commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

trevnorris commented 11 years ago

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

myrne commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

isaacs commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 10 years ago

landed in fa4eb47 and e8df267

myrne commented 10 years ago

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 commented 10 years ago

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

rlidwka commented 10 years ago

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