process hangs when spawned through child_process

[silverwind]

I'm using grunt-shell to run ncu -ua and it seems like the process hangs indefinitely, same command works from the shell. The shell module config is:

shell: {
  options: {
    stdout: true,
    stderr: true,
    failOnError: true
  },
  update: {
    command: "ncu -ua"
  }
}

It basically spawns a child process, so I think it shoud be reproduceable with just child_process.exec.
I've tried tracking it down a bit and it looks like the promise returned here never resolves.

[raineorshine]

Thanks for tracking this down! Since process.stdin.isTTY was falsey, it assumes that something is being piped through stdin and it is waiting for it (as described here).

get-stdin-promise is as simple as:

var stdin = require('get-stdin')
module.exports = new Promise(function(resolve, reject) {
    stdin(function (data) {
        resolve(data)
    })
})

which ultimately leads to get-stdin (all paths lead so to sindresorhus):

module.exports = function (cb) {
    var stdin = process.stdin;
    var ret = '';

    if (stdin.isTTY) {
        setImmediate(cb, '');
        return;
    }

    stdin.setEncoding('utf8');

    stdin.on('readable', function () {
        var chunk;

        while (chunk = stdin.read()) {
            ret += chunk;
        }
    });

    stdin.on('end', function () {
        cb(ret);
    });
};

get-stdin invokes the callback (which resolves the promise) as soon as stdin closes.

So, my question is, how does grunt-shell handle stdin? When does it receive the end event? I don't think it would be waiting if it didn't detect that stdin was open, but it's possible that I'm missing something.

Please let me know if you are able to troubleshoot further. If you can't get any further, I will investigate more myself. I'm not invested in understanding grunt-shell inside-and-out yet, so if you were able to identify how it handles stdin that would be a big help. Thanks!

[silverwind]

With my configuration, all grunt-shell does to stdin is

process.stdin.resume();
process.stdin.setEncoding('utf8');
process.stdin.pipe(cp.stdin);

get-stdin runs the callback either if stdin.isTTY is true, which it is not in our case of a child-process, so it waits on a readable event, which never fires. I've tried inputting keys, but I don't think grunt actually forwards the stdin to a task. With the process's stdin piped to the child, I don't think an end event actually fires until the task ends.

I thnk the fix should be pretty simple: resolve the promise immediately if stdin.isTTY is false. I just hope it won't break any piping into ncu.

Maybe @sindresorhus has more insights into stdio handling of a grunt task :)

[raineorshine]

I'm a little confused since stdin.isTTY is false when something is being piped, so it doesn't make sense to resolve the promise immediately in that case.

I will run some tests and investigate further.

[raineorshine]

Okay, after some investigation, here is what I have discovered:

• It's not an issue with the promise.
• Because grunt-shell resumes stdin, it is indeed waiting for input and an EOF character. Try typing Ctrl+D when it is hanging and you should see it terminate.
• It waits for input even when grunt-shell's options.stdin is false. I don't really get that.

Ultimately, the way grunt-shell is written, ncu will always wait for a package.json to be piped to it instead of using the package.json in the current working directory. I can't change this without breaking the normal stdin functionality, which works as expected in all other places. Perhaps that is the desired behavior of grunt-shell, but it seems a little suspicious. Not much I can do about that.

Since ncu is waiting for stdin, you should be able to fix your issue by manually piping your package.json. Try this out:

grunt.initConfig({
  shell: {
    target: {
      command: 'cat package.json | ncu'
    }
  }
});

[zenopopovici]

I had the same issue so I figured out I could use grunt.util.spawn to achieve the same. I also wanted to run npm update, that breaks in the same fashion. This works:

// Install npm updates
grunt.registerTask('update-npm', 'Update package.json and update npm modules', function() {
    grunt.log.writeln('If you get an error here, run "npm install -g npm-check-updates".');
    grunt.task.run('npm-write-new');
    grunt.task.run('npm-update');
});
// Check for npm module updates
grunt.registerTask('npm-check', 'Check for npm modules updates', function() {
    var done = this.async();

    grunt.log.writeln('Checking for npm modules updates ...');

    grunt.util.spawn({
        cmd: 'ncu',
        args: '',
        opts: {
            stdio: 'inherit',
        }
    }, function () {
        grunt.log.writeln('No files were modified.');
        done();
    });
});
// Write new versions to packages.json
grunt.registerTask('npm-write-new', 'Write new versions to package.json', function() {
    var done = this.async();

    grunt.log.writeln('Checking for npm modules updates ...');

    grunt.util.spawn({
        cmd: 'ncu',
        args: ['-u'],
        opts: {
            stdio: 'inherit',
        }
    }, function () {
        grunt.log.writeln('New versions were written to "package.json".');
        done();
    });
});
// Update npm modules
grunt.registerTask('npm-update', 'Update npm modules', function() {
    var done = this.async();

    grunt.log.writeln('Installing npm modules updates ...');

    grunt.util.spawn({
        cmd: 'npm',
        args: ['update','--loglevel','warn'],
        opts: {
            stdio: 'inherit',
        }
    }, function () {
        grunt.log.writeln('NPM modules were updated.');
        done();
    });
});

[silverwind]

@metaraine I too think the bug here is in grunt-shell. See sindresorhus/grunt-shell#95 (comment) for what I think is happening. It boils down that you can't seem to detect whether you are piped to in a child-process spawned by exec it seems. And grunt-shell actually never pipes to ncu because the piping happens after exec already ran.

[silverwind]

The real fix would be for grunt-shell to switch to spawn instead of exec, but I don't think it's a viable solution as spawn takes an array of args, which would require some nasty parsing from the command string the module takes.

I'm thinking you could add a small timeout (maybe 1 second), and if nothing is received on stdin, continue as normal.

[raineorshine]

Great points. I agree. Thank you! Hopefully a better way to detect stdin becomes available at a lower level.

The timeout seems too hacky to me. The manual piping should work fine as a workaround, yes? cat package.json | ncu. I've documented this in the README.

[silverwind]

Yeah, it does work. I'm doing that now. Let's see if this issue can be resolved in node: nodejs/node#2339

[Deleted user]

I started a fork yesterday to solve this the "hacky" way. Is the timeout approach too hacky to be a temporary solution until a solution can be provided via a node release? It's an edge case that will only apply to a small percentage of scenarios, but would save users the time of investigating the issue when they come across it. Now I know I can solve the problem without a change, but would like to fix it for anyone else that comes across it.

[silverwind]

The real cause for the hang is in the design of Node's async child process methods. The parent process has to explicitely close stdin so the child can continue. The issue should affect all modules that use child_process to spawn childrens that wait for stdin to end (like through get-stdin). I'm not sure there will ever be a fix because it'd be a big breaking change.

See sindresorhus/get-stdin#13 (comment) for a possible fix.

[raineorshine]

Also tracked in #136

[Deleted user]

I get the cause after reading the updates to node 2339.

I did close stdin from the parent process yesterday, which then caused an error in ncu (didnt dive deeper to see if it was in ncu or get-stdin) for expecting at least a byte to be streamed but getting none. I figured if that needed to be handled in ncu or get-stdin explicitly and it's not as clean as closing stdin from the parent, then it could impact any other child commands and not just ncu.

Thanks for sharing context on it. I'll stick to piping my package.json into ncu for now.

[raineorshine]

@eighty4 ncu will probably (haven't confirmed) fail if the value from stdin is an empty string, rather than falling back to non-stdin packageFile detection. This could be fixed.