heroku / heroku-pipelines

this code is now in https://github.com/heroku/cli
https://www.npmjs.com/package/@heroku-cli/plugin-pipelines
33 stars 19 forks source link

pipelines:add fails to exit when called from another node process #19

Open redbugz opened 8 years ago

redbugz commented 8 years ago

We are executing heroku commands with Node and when we call heroku pipelines:add if the command succeeds, the child process never exits which hangs the parent process.

We were using execFile, but I switched to spawn so we could watch the output. If the command is run directly from the terminal, it works fine. Since pipelines are not supported in the API yet, exec-ing the toolbelt is the easiest way to implement this that I know. If we should try to require this module and use it directly instead, we could try that I guess.

I tried this with Node 0.10, 0.12, and 4.2 with the same result. We can't use 5 yet, so I haven't tried that. We are transitioning off of 0.10 over the next few months, but we still need this work until then.

What our code is doing (with dummy names for pipeline and app instead of the variables we pass):

      var execFile = require('child_process').execFile,
        child;

      console.log('calling heroku pipelines ...');
      child = execFile('heroku', ['pipelines:add', 'my-pipeline', '--stage', 'development', '--app', 'my-app-name'],
        function (error, stdout, stderr) {
          if (error !== null) {
            console.log('$execFile error: ' + error);
          }
          console.log('$stdout: ' + stdout);
          console.log('$stderr: ' + stderr);
        });

With the resulting output:

calling heroku pipelines ...

but the callback is never called, the process doesn't exit, and the ps command still shows the heroku command running.

When using spawn to watch the output more carefully:

      var spawn = require('child_process').spawn,
        ls    = spawn('heroku', ['pipelines:add', 'my-pipeline', '--stage', 'development', '--app', 'my-app-name']);

      ls.stdout.on('data', function (data) {
        console.log('$stdout: ' + data);
      });

      ls.stderr.on('data', function (data) {
        console.log('$stderr: ' + data);
        //if (/^done/.test(data)) {
        //  console.log('killing');
        //  ls.kill('SIGQUIT');
        //}
      });

      ls.on('close', function (code) {
        console.log('$child process exited with code ' + code);
      });

With output:

$stdout: 
$stderr: Adding my-app-name to my-pipeline pipeline as development... 
$stderr: done

But we never get the 'close' event, and the process hangs.

Running the command directly in the terminal works fine:

$ heroku pipelines:add my-pipeline --stage development --app my-app-name
Adding my-app-name to my-pipeline pipeline as development... done
$ echo $?
0
$

Using the regex check for done in the stderr stream in the spawn example and force killing the process, or using the {timeout: 3000} option in execFile also work around the issue, but aren't very tidy.

appleton commented 8 years ago

I'm going to cc @heroku/cli because I think this is probably an issue with the plugin architecture in general.

Having said that you can make API calls directly from your node script here's an example. The only thing to be aware of with that is that when we promote the API from beta you'll need to change the Accept header to 'application/vnd.heroku+json; version=3' when we do.

jdx commented 8 years ago

if you take these lines out it works: https://github.com/heroku/heroku-pipelines/blob/master/commands/pipelines/add.js#L40-L41

jdx commented 8 years ago

I'm pretty sure this is some kind of issue with stdin not being a tty, but I wasn't able to reproduce it. echo '' | heroku pipelines ... doesn't seem to have the same issue. Likely because that buffer is closed.