You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
exit_process.h: fix handling of SIGINT and SIGTERM
Handle SIGINT and SIGTERM by injecting into the process
a thread that runs ExitProcess. Use TerminateProcess otherwise.
In both cases, enumerate the entire process tree.
This fixesgit-for-windows/git#1219
Signed-off-by: Adam Smith <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
@dscho Unfortunately after further investigation it seems the change I added doesn't fix the node ctrl+c issue.
All of the child processes are killed, but processes still can't respond to SIGINT or SIGKILL. It seems that line 179 always evaluates to false, and TerminateProcess is always used. It looks like after this lineexit_code === SIGINT and exit_code === SIGTERM always evaluates to false. If I force the use of terminate_process_with_remote_thread. The node child processes don't get closed and this cleanup code doesn't run either:
process.on('SIGTERM',function(){console.log("\nresponding to SIGTERM");// other cleanup codeprocess.exit(1);});process.on('SIGINT',function(){console.log("\nresponding to SIGINT");// other cleanup codeprocess.exit(1);});
I'm continuing to investigate. I'll see if I can make a change to terminate_process_with_remote_thread that resolves the issue.
Where is exit_process called when responding to ctrl+c? If it's passing the exit code in the same way e.g. sig + 128 then the exit_code == SIGINT check will fail.
I tested this with Node v6.10.2 as shipped in MSYS2 and with this script:
#!/usr/bin/env node
process.on('SIGTERM',function(){console.log("\nresponding to SIGTERM");// other cleanup codeprocess.exit(1);});process.on('SIGINT',function(){console.log("\nresponding to SIGINT");// other cleanup codeprocess.exit(1);});varhttp=require('http');http.createServer(function(request,response){response.writeHead(200,{'Content-Type': 'text/plain','Access-Control-Allow-Origin' : '*'});response.end('Hello World\n');}).listen(1337);
Actually... After testing a bit further, it looks like the new code path is not hit in that case, and I cannot get it to work with kill.exe. Will keep digging, even if I do not really have any time to spend on this ;-)
After chasing down this rabbit hole, I finally have something that really seems to work: dscho@fix-ctrl-c-again
@afsmith92 would you mind testing this? It also requires kill.exe to be rebuilt (cd ../utils && make kill.exe after you built msys0.dll), as it has to spawn kill.exe process so that that one can attach to the Console of the target process and send the Ctrl event.
Works perfectly. Tested in a PortableGit with node v6.11.2. Killed all child processes and was able to handle SIGINT:
process.on('SIGINT',function(){console.log("\nresponding to SIGINT");// other cleanup codeprocess.exit(1);});
logged responding to SIGINT to the console after ctrl +c
Thank you so much for taking the time to figure this out, and I'm sorry for getting you dragged into this issue. I was only following the issue on git-for-windows -- not the issue logged to node -- and I wasn't aware of the issues with SIGINT and SIGTERM handling (until you tagged me in the node issue) and thus only tested for the child processes issue described in the git-for-windows issue.
I'll keep an eye on the issues list for this repo, and I'll jump in if I see anything I think I can help with in the future.
16 commit comments
afsmith92 commentedon Jan 11, 2018
@dscho Unfortunately after further investigation it seems the change I added doesn't fix the node ctrl+c issue.
All of the child processes are killed, but processes still can't respond to
SIGINT
orSIGKILL
. It seems that line 179 always evaluates to false, andTerminateProcess
is always used. It looks like after this lineexit_code === SIGINT
andexit_code === SIGTERM
always evaluates to false. If I force the use ofterminate_process_with_remote_thread
. The node child processes don't get closed and this cleanup code doesn't run either:I'm continuing to investigate. I'll see if I can make a change to
terminate_process_with_remote_thread
that resolves the issue.dscho commentedon Jan 11, 2018
The
kill
utility should not be involved at all, so that line 179 should not be hit at all...afsmith92 commentedon Jan 11, 2018
Where is
exit_process
called when responding to ctrl+c? If it's passing the exit code in the same way e.g.sig + 128
then theexit_code == SIGINT
check will fail.dscho commentedon Jan 12, 2018
It's called via
signal()
inexceptions.cc
. AFAICT you callexit_process()
correctly there...dscho commentedon Jan 12, 2018
Oh no. I was mistaken!
msys2-runtime/winsup/cygwin/exceptions.cc
Line 1564 in 53e5c03
dscho commentedon Jan 12, 2018
Maybe we should pass the real code instead, and let
exit_process()
decide whether to add 128 or not?dscho commentedon Jan 12, 2018
I made up my mind and think that testing
exit_code & 0x7f
might be better. My proposed fix: dscho@fix-ctrl-c-againWhat do you think?
dscho commentedon Jan 12, 2018
I tested this with Node v6.10.2 as shipped in MSYS2 and with this script:
This is the "screenshot":
So I call this success and will push my fix ;-)
dscho commentedon Jan 12, 2018
Actually... After testing a bit further, it looks like the new code path is not hit in that case, and I cannot get it to work with
kill.exe
. Will keep digging, even if I do not really have any time to spend on this ;-)afsmith92 commentedon Jan 12, 2018
@dscho I'm continuing to investigate as well. Would it be best to revert the patch at this point?
That sounds reasonable to me.
afsmith92 commentedon Jan 12, 2018
Also, I found a different implementation of the safe ExitProcess in this thread -- I've been messing with this.
dscho commentedon Jan 12, 2018
After chasing down this rabbit hole, I finally have something that really seems to work: dscho@fix-ctrl-c-again
@afsmith92 would you mind testing this? It also requires
kill.exe
to be rebuilt (cd ../utils && make kill.exe
after you builtmsys0.dll
), as it has to spawnkill.exe
process so that that one can attach to the Console of the target process and send the Ctrl event.afsmith92 commentedon Jan 12, 2018
Absolutely. I'll have it tested within the next half hour.
afsmith92 commentedon Jan 13, 2018
Still compiling..
afsmith92 commentedon Jan 13, 2018
Works perfectly. Tested in a PortableGit with
node v6.11.2
. Killed all child processes and was able to handleSIGINT
:logged
responding to SIGINT
to the console after ctrl +cThank you so much for taking the time to figure this out, and I'm sorry for getting you dragged into this issue. I was only following the issue on git-for-windows -- not the issue logged to node -- and I wasn't aware of the issues with
SIGINT
andSIGTERM
handling (until you tagged me in the node issue) and thus only tested for the child processes issue described in the git-for-windows issue.I'll keep an eye on the issues list for this repo, and I'll jump in if I see anything I think I can help with in the future.
dscho commentedon Jan 13, 2018
No worries! You helped. That means a lot to me. And thanks for verifying my latest attempt at a fix!