Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix interrupting git clone via Ctrl+C #41

Closed
dscho opened this issue Mar 19, 2015 · 10 comments
Closed

Fix interrupting git clone via Ctrl+C #41

dscho opened this issue Mar 19, 2015 · 10 comments
Assignees
Labels

Comments

@dscho
Copy link
Member

dscho commented Mar 19, 2015

This is a big problem that also extends to suspending via Ctrl+Z. The problem is that the processes spawned by git clone (git-remote-https and git-fetch-pack) continue running unfazed when the main process is interrupted or suspended.

This happens only when git clone was started in mintty, though, and a workaround is to call winpty git clone.

@dscho
Copy link
Member Author

dscho commented Mar 19, 2015

I already spent a dozen hours on this vexing problem. It appears as if GetConsoleProcessList() contains the subprocesses. I also ensured that CreateProcess() is not called with CREATE_NEW_PROCESS_GROUP.

The most curious thing is that according to MSDN's signal() documentation:

SIGINT is not supported for any Win32 application. When a CTRL+C interrupt occurs, Win32 operating systems generate a new thread to specifically handle that interrupt. This can cause a single-thread application, such as one in UNIX, to become multithreaded and cause unexpected behavior.

And indeed, the main process only receives a SIGALRM every second or so instead.

@nalla
Copy link

nalla commented Mar 20, 2015

If we evaluate that SIGALRM in the main process, can we diffenciate between CTRL+C and CTRL+Z? Maybe we can then retrieve a list of the sub processes and notify them?
Please note that this is just a very abstract theory.

@dscho
Copy link
Member Author

dscho commented Mar 20, 2015

If we evaluate that SIGALRM in the main process, can we diffenciate between CTRL+C and CTRL+Z?

Actually, I think that the SIGALRM is active whether Ctrl+C / Ctrl+Z have been pressed or not...

@nalla
Copy link

nalla commented Mar 20, 2015

I wonder.. is this issue something that was/is present on MSYSGIT?

@dscho
Copy link
Member Author

dscho commented Mar 20, 2015

More findings: GenerateConsoleCtrlEvent(CTRL_C_EVENT, GetCurrentProcessId()); seems to be unable to terminate the Git process nor its children when launched in mintty, but after this function is called, a TerminateProcess(GetCurrentProcess()) does manage to kill the children.

When run from cmd, the Git process calling GenerateConsoleCtrlEvent() managed to kill itself (and its children, of course, that always works in cmd).

When calling from mintty and calling AttachConsole(ATTACH_PARENT_PROCESS), however, the GenerateConsoleCtrlEvent() does kill the process and its kids!

@dscho
Copy link
Member Author

dscho commented Mar 20, 2015

is this issue something that was/is present on MSYSGIT?

@nalla yep, but it was hidden by the fact that our default terminal was the cmd.exe-launched one.

@dscho
Copy link
Member Author

dscho commented Mar 20, 2015

When calling from mintty and calling AttachConsole(ATTACH_PARENT_PROCESS), however, the GenerateConsoleCtrlEvent() does kill the process and its kids!

Sadly, Ctrl+C still does the wrong thing, then. It fails to kill the spawned process, only the main process is terminated.

Which leads me to believe that we might need to call AttachConsole(ATTACH_PARENT_PROCESS) in mingw_startup() and change mintty, bash or msys2-runtime to call both GenerateConsoleCtrlEvent() and TerminateProcess() to kill a process when Ctrl+C was pressed. Or something similar.

@dscho
Copy link
Member Author

dscho commented Mar 20, 2015

I opened a PR with the work in progress: git-for-windows/msys2-runtime#6

It appears to fix the issue here...

@nalla
Copy link

nalla commented Mar 24, 2015

I believe since msys2-runtime#6 is merged, this issue can be closed as well?

@dscho
Copy link
Member Author

dscho commented Mar 25, 2015

Sorry for the delay; I managed to shoot my development setup real good by missing one goto next1; when resolving merge conflicts during the merging rebase of the msys2-runtime.

All is fixed now, and new msysw-runtime versions are uploaded.

@dscho dscho closed this as completed Mar 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants