nodejs / node-v0.x-archive

Moved to https://github.com/nodejs/node
34.42k stars 7.31k forks source link

Any chance of a synchronous child_process.exec/spawn in the future? #1167

Closed ghost closed 10 years ago

ghost commented 13 years ago

It would be mighty useful for a data migration script I'm writing at the moment.

Chris

isaacs commented 13 years ago

Not likely.

ry commented 13 years ago

I'd like to have a sync child process thing.

ry commented 13 years ago

@felixge wants to do this

isaacs commented 13 years ago

Really? That seems kinda hazardous.

pquerna commented 13 years ago

execSync is hazardous, just like readFileSync.

readFileSync's usecase is most often loading 'small', say <100kb, files into memory quickly/easily. This is a valuable use case for using node as a command line scripting tool and initialization code. You can abuse this of couse and try to load a 1gb file in via this method, but we would just tell people not to do that, but the original use case is still valid.

The argument for execSync is the same; It is very helpful to be able to easily call a short lived program and get its output. Using it to call curl on a remote server of course would be abusive and we would yell at anyone who does this, but it doesn't mean its not a useful thing to have.

FWIW, I was moderately opposed to this originally, then Ryan brought me another beer... well now i see it as acceptable in light of our existing API, and for making node less of a pain in the ass to use for short scripts than it needs to be.

ghost commented 13 years ago

Thanks @pquerna, I write most, if not all of my command line scripts in node these days :-)... one language to rule them all!

eboto commented 13 years ago

I understand why execSync is hazardous in a server context, but without it writing little scripts becomes unnecessarily complicated.

I wanted to write some build scripts using node, and the inability to drop down and synchronously exec some small command line tools is really making that difficult.

felixge commented 13 years ago

https://github.com/felixge/node/commits/exec-sync is the branch I'm working in for anybody interested. Just waiting on some feedback from ryan before I finish it up and clean the commits a bit.

For those who think of this as hazardous, have a look at #1329. But as along as we call this stuff *Sync, we should be good. --fg

JonDum commented 13 years ago

+1 execSync would make simple build scripts in a non-webserver env so much easier.

felixge commented 13 years ago

It's gonna land in 0.5 soon, the libuv part is almost done: https://github.com/felixge/libuv/compare/master...spawn_sync

JonDum commented 13 years ago

How stable is your spawn_sync? Would you like me to play around with it and see if I can break it? :P

felixge commented 13 years ago

I have not done the node bindings for new libuv version of this yet, so I don't think there is much for you to play around with yet.

sh1mmer commented 13 years ago

+1 for execSync for scripts

There is only so much you can do to protect people from themselves.

Mithgol commented 12 years ago

Currently (in Node.js version 0.6.3), in order to console.log(…) some UTF-8 output from a Windows shell command in Russian locale, you need «chcp 65001» to work around #2190 and «chcp 866» to work around #2196. The script looks like the following:

var forker = require('child_process');

forker.exec('chcp 65001 | dir', function(err, outstr){
   forker.exec('chcp 866', function(){
      console.log('\n' + outstr);
   });
});

However, the two .exec(…) calls are not synchronous here, and the Node.js console possibly stays in CP65001 mode for a few ticks between them (and that would affect any intermediate console.log(…) output from a parallel worker).

An .execSync(…) method could prevent that, if existed.

janekp commented 12 years ago

"It's gonna land in 0.5 soon"

Any updates? Even 0.6.6 docs don't seem to mention it

isaacs commented 12 years ago

@felixge Still interested in doing this? If not (which is fine), can you comment on what's still remaining? Thanks.

/cc @ry

felixge commented 12 years ago

@felixge Still interested in doing this?

Yes, but during JSConf.EU @piscisaureus convinced me that the way I was doing it was sub-optimal, and the right way would be to create a second uv loop which runs within the main loop, but has the main loop waiting/blocking on it until it has executed the child process (Bert: Correct me if I'm wrong).

So basically what I've done so far would be thrown away (well, some of the v8 glue may be salvageable), and need to be redone. If there is still an interest in landing this in 0.7 I could give it a shot and see if I can get this going.

--fg

piscisaureus commented 12 years ago

@felixge do you still have that patch floating around somewhere? Back then the issue was that it relied on some unimplemented APIs of libuv, so we couldn't land it, but these have been added for isolate support.

isaacs commented 12 years ago

Obviously this is off the table for 0.6, but if it doesn't introduce undue risk, it'd be nice to have for 0.7.

If it does introduce undue risk, then I'd say we punt on it altogether, and stop suggesting that it'll show up at some future date.

JonDum commented 12 years ago

@felixge Since we all know that synchronous exec would be horrible for performance, does it really matter if your method is sub-optimal? I'd take what I could get, especially since the async exec's already work fine for when performance does matter.

felixge commented 12 years ago

@felixge do you still have that patch floating around somewhere?

Haven't found it right away, but I should be able to dig it up. Will ping you.

If it does introduce undue risk, then I'd say we punt on it altogether, and stop suggesting that it'll show up at some future date.

I don't think it will be very risky.

@felixge Since we all know that synchronous exec would be horrible for performance, does it really matter if your method is sub-optimal? I'd take what I could get, especially since the async exec's already work fine for when performance does matter.

Oh, the main concern was code duplication / complexity. So I'd really like to do @piscisaureus's suggested solution.

chowey commented 12 years ago

+1 I also could use spawnSync

felixge commented 12 years ago

@felixge do you still have that patch floating around somewhere?

Ok, seems like I can't find it. Anyway, I just dug in and attempted a patch which would allow me to re-use all of the execFile / spawn code:

https://github.com/felixge/node/commit/8aa7a3c38ed860ebbe933e3a4c5bd66668babf7c

There may be other issues with this approach, but the one I just ran into is this:

../deps/uv/src/unix/ev/ev.c:2991: failed assertion `("libev: child watchers are only supported in the default loop", loop == ev_default_loop_ptr)'
Abort trap: 6

So if I read this correctly, this entire approach is doomed unless we patch libev to support child processes in other loops?

--fg

piscisaureus commented 12 years ago

So if I read this correctly, this entire approach is doomed unless we patch libev to support child processes in other loops?

It has to be done anyway, or else isolates can't spawn child processes.

felixge commented 12 years ago

It has to be done anyway, or else isolates can't spawn child processes.

Ok. I think that would be too tricky for me to get right (@bnoordhuis explained the problem of matching signals to the loop they belong to), but if somebody else is going to do it, I should be able to continue / finish this feature.

nibblebot commented 12 years ago

+1 for execSync or spawnSync

arturadib commented 12 years ago

I too would love to see these functions added. We're building maker.js -- a Make/shell tool for Node -- to replace all of our Makefile scripts at pdf.js and other projects.

Right now I'm relying on a major hack to implement an execSync() routine purely in JavaScript:

https://github.com/arturadib/maker.js/blob/4f25f265685c5556204fcf7970b7163345f84496/maker.js#L863

Essentially I spawn another Node process that in turn is responsible for spawning the desired command and spitting out a flag file when the process exits - the original process simply enters a loop that waits for the flag file to be created (via many many calls to fs.existsSync()).

It works, and it works across platforms (Win/Linux/OS X), but as noted above, the wait loop is extraordinarily inefficient CPU-wise. Anything that takes more than 10 sec to execute will spin up the fans on my laptop.

I'm not familiar with any platform primitives that allow the CPU to "go to sleep" until a process is done, perhaps there is none, but if we're going to implement something like this wait loop at whatever layer, it's probably a good idea to reduce the priority of the wait loop process (e.g. via nice or something like it), if at all possible.

Definitely excited to see progress on this and help however I can though! :)

mnlagrasta commented 12 years ago

Is anybody actively working on this? It's beyond my skill level, but I'd like to follow it's implementation and support it in some way if I can.

bnoordhuis commented 12 years ago

@mnlagrasta: No one's working on it. Now that isolates are off the table, there's no longer a pressing need to fix up libev / libuv.

karlbohlmark commented 12 years ago

My usecase today: small CI-script that takes screenshots of all pages in the application. It wants to call ´git log -1´ initially to determine current commit.

Of course I can do it with exec, but I don't see why we can't have execSync for this kind of thing. I mean, look at what @arturadib has to go through... I can understand if the reason is implementation difficulties, but if this is about preventing developers from shooting themselves in the foot, like @isaacs seems to indicate, then it seems misguided to me.

All node programs are not servers

isaacs commented 12 years ago

@karlbohlmark Yeah, this is approved, just waiting for someone to write it.

dstaubsauger commented 12 years ago

I wrote a plugin for node to do this. Unfortunately i'm not a C++ programmer, so the whole thing is basically three tutorials copied together plus some weird type conversions. I strongly recommend against using this in a production environment, since there is no error handling etc, and there might be some more caveats that i'm unaware of. But here's the source:

node_answerme.cpp

#include <v8.h>
#include <node.h>
#include <string>
#include <iostream>
#include <stdio.h>
#include <stdlib.h>

using namespace v8;
using namespace node;
using namespace std;

// using template from http://kkaefer.github.com/node-cpp-modules/

// this function is from http://www.magpcss.org/ceforum/viewtopic.php?f=10&t=450
std::string GetString(v8::Handle<v8::String> str)
{
  // Allocate enough space for a worst-case conversion.
  int len = str->Utf8Length();
  char* buf = new char[len+1];
  str->WriteUtf8(buf, len+1);
  std::string ret(buf, len);
  delete [] buf;
  return ret;
}

Handle<Value> hangman(const Arguments& args) {
    HandleScope scope;
    FILE* pipe = popen(GetString(args[0]->ToString()).c_str(), "r"); // crappy type vonversion i had to look up
    // this is from http://stackoverflow.com/questions/478898/how-to-execute-a-command-and-get-output-of-command-within-c
    char buffer[128];
    std::string result = "";
    while(!feof(pipe)) {
        if(fgets(buffer, 128, pipe) != NULL)
            result += buffer;
    }
    pclose(pipe);
    v8::Handle <v8::String> resultt = v8::String::New(result.c_str()); // thanks to http://www.linuxso.com/linuxbiancheng/2986.html and Google Translate
    return scope.Close(resultt);
}

void RegisterModule(v8::Handle<v8::Object> target) {
    // Add properties to target
    NODE_SET_METHOD(target, "exec", hangman);
}

// Register the module with node.
NODE_MODULE(answerme, RegisterModule);

wscript

#!/usr/bin/env python

def set_options(ctx):
  ctx.tool_options('compiler_cxx')

def configure(ctx):
  ctx.check_tool('compiler_cxx')
  ctx.check_tool('node_addon')

def build(ctx):
  t = ctx.new_task_gen('cxx', 'shlib', 'node_addon')

  t.source = ['node_answerme.cpp']

  # Must be same as first parameter in NODE_MODULE.
  t.target = 'answerme'

after you compiled it, you can access the synchronous exec with

var execSync = require("./answerme.node").exec;
var fortune = execSync("fortune");
console.log(fortune);

//There is an old time toast which is golden for its beauty.
//"When you ascend the hill of prosperity may you not meet a friend."
//      -- Mark Twain

Do with this what you want.

diosney commented 12 years ago

+1 execSync(). What is the state of art of this?

tj commented 12 years ago

+1 I could really use this for a shell

xavi- commented 12 years ago

+1 This would be very help

For the time being there's execSync: https://github.com/mgutz/execSync

It works for the most part, though I'm having trouble with it on windows.

Mithgol commented 12 years ago

Windows systems do not (usually) have any libc but your module requires that.

jescalan commented 12 years ago

+1 on this one, would be super useful

sindresorhus commented 12 years ago

ShellJS has a sync exec method. Though I would really like to have this built-in.

taijinlee commented 11 years ago

+1 on this one for me too. Definitely would like it for initial config

bmeck commented 11 years ago

Will take this over in next couple days if no one else does

bmeck commented 11 years ago

@isaacs any idea on piping data into the sync process? ala stdin etc? could try to drain stdio pipes until they close i guess

ivzhao commented 11 years ago

+1

aponxi commented 11 years ago

+1

isaacs commented 11 years ago

@bmeck I'm not sure. Maybe pass in stdin as an argument? It's synchronous, so it's not like you'll be able to pipe something in. @piscisaureus, what are your thoughts on this?

coreybutler commented 11 years ago

+1

bjouhier commented 11 years ago

I am writing all my non-trivial utilities in node these days. Much cleaner and portable than shell scripts.

But I don't have any need for Sync APIs. I write everything on top of async calls. Why pollute node APIs with Sync calls when it can all be done so easily the async way?

bjouhier commented 11 years ago

Also, the day you'll want to hook your script logic to a web UI, you'll be able to execute it directly inside your node web server if you wrote it the async way.

aponxi commented 11 years ago

@bjouhier The reason I needed this was to be able to see in real time tasks like make. Usually I want to run an external script and use the stdout but when I was making that, if a function echoed something like:

  1. script started
  2. then ran the script
  3. then echoed script finished

it would output in the order of:

  1. script started,
  2. script finished,
  3. script stdout.

Is what you are doing allows this to be done in the right order? or relevant to this in any way?

dstaubsauger commented 11 years ago
exec("./configure",function(){
  exec("make",function(){
    exec("sudo make install");
  })
})

vs.

exec("./configure");
exec("make");
exec("sudo make install");

(silly example, but you get the point)

bjouhier commented 11 years ago

@aponxi @dstaubsauger

There was a bit of irony in my comment. Here is how I'm writing the chain of exec:

#!/usr/bin/env _node --cache
var exec = require('child_process').exec;
console.log("START")
exec("./configure", _);
exec("make", _);
exec("sudo make install", _);
console.log("DONE")

Notice that this is run by _node, not node. I'm using a tool that transforms this sync-looking code into async code with callbacks. So I don't need any Sync APIs in node because I can use async APIs as if they were sync. For example, I can write things like:

console.log(exec("echo hello", _) + exec("echo world", _));

This approach has been the subject of lots of (sometimes nasty) debates on the mailing list and I don't want to launch another one here but the point I want to make is on APIs:

We don't need no Sync calls!