exec and execSync git clone private_repo + timeout hangs the REPL

[amasad]

• Version: v6.7 (also v5.12)
• Platform: Linux src: redo unaligned access workaround #138-Ubuntu SMP Fri Jun 24 17:00:34 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

Expected behavior: REPL resumes working
Actual behavior: Process hangs

Description:
When git cloning a repo that requires entering a username and password in child_process.exec or child_process.execSync and hitting a timeout then the program and shell will hang. I've tried this on multiple machines. Here is repro code that uses a private repo of mine (notice how it asks for the username and then the program and shell hangs):

> child_process.execSync('git clone https://github.com/amasad/repl-it-web', { timeout: 1000, stdio: ['ignore']})
Username for 'https://github.com': Error: spawnSync /bin/sh ETIMEDOUT
    at exports._errnoException (util.js:893:11)
    at spawnSync (child_process.js:448:20)
    at Object.execSync (child_process.js:504:13)
    at repl:1:15
    at REPLServer.defaultEval (repl.js:270:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:439:10)
    at emitOne (events.js:95:20)
    at REPLServer.emit (events.js:182:7)

Note that if I pkill node from another shell then the shell starts working again.

[bnoordhuis]

Is this a bug report or a help question? If it's the former, can you be explicit about what the actual and expected behavior is? If it's the latter, can you move it to https://github.com/nodejs/help/issues? Thanks.

[amasad]

@bnoordhuis bug report. I thought it's clear that the process hangs and that shouldn't be expected. I edited to make it clear.

[bnoordhuis]

Okay, thanks. What happens is that the timeout signal terminates /bin/sh but not git. Try adding { detached: true }, that makes the signal get propagated across the process group. Programs may elect to ignore SIGTERM, use { killSignal: 'SIGKILL' } if that is an issue.

It's not a node.js bug, just an artifact of the UNIX process model. I'll add documentation labels because it looks like detached isn't documented for child_process.execSync().

[amasad]

Ah, great -- detached works.
It looks like exec doesn't pass on the detached option though, does it make sense to add it?

[bnoordhuis]

I suppose that wouldn't hurt. Consistency is good.

[jalafel]

Hi! I'm still fairly new to contributing, I have technically done a first contribution, but still know nothing. Would anyone mind if I took this on just to go over the documentations and understand the child_process a bit more? Thanks!

[sam-github]

detached has a different meaning on Windows and Unix, I believe, been a while since I looked. I think it actually means something more like "detached" on Windows... i.e., it allows the child to outlive the parent process (something which is always allowed on Unix). This may be why detached isn't passed in .exec(), since node waits for the child's output with exec, it doesn't really make sense to request the child have a life time longer than the parent.

Which isn't to say that it wouldn't be more consistent to have detached be possible to specify with exec, but the docs will need to be clear about what it does (on both process models, Windows and Unix).

[sam-github]

@jessicaquynh go for it, perhaps you even want to PR a change to allowe .detached with exec? Do you have the ability to test on Windows, to confirm if/how its different? Process model differences can be hard to determine experimentally.

/cc @orangemocha Who might know what detached really does on Windows.

[jalafel]

@sam-github Thanks. I will do that, and yup! I have access to Windows! I will get on it. :)

[jalafel]

@sam-github I've opened up a PR for the suggested changes. Although I ran a experimental tests between Windows and Unix, I wasn't able to spot a large difference between what's been documented. I am hoping @orangemocha may be able to clarify.

[cjihrig]

@orangemocha isn't around as much anymore. We could try @joaocgreis or @nodejs/platform-windows instead.

[Trott]

This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.