`execTask.cancel()` doesn't kill the git process properly

[maifeeulasad]

Hello good people of Desktopland,

Hope you are all doing well. I was working on a task to cancel the cloning of a repository, but I was facing some issues.

So at eleventh hour (I thought about it earlier and forgot), I thought about creating a toy project, where I will simply try to clone something and cancel that task before it completes, and see what happens. So I think I have found some issue.

So let's take this piece of code for example:

import { GitProcess, GitTaskCancelResult } from 'dugite'

const args = ['-c', 'credential.helper=', '-c', 'init.defaultBranch=main', 'clone', '--recursive', '--progress', '--', 'https://github.com/maifeeulasad/maifeeulasad.git', 'C:\\dugite-test/maifee']
const path = "C:\\dugite--abort"

try{
    const task = GitProcess.execTask(args, path);

    

    task.result
    .then((res)=>{    
        console.log('-----------res------------')
        console.log(res)
        console.log('^^^^^^^^^^^res^^^^^^^^^^^^')
    })
    .catch((e)=>{
        console.log('-----------dugite internal------------')
        console.log(e)
        console.log('^^^^^^^^^^^dugite internal^^^^^^^^^^^^')
    })

    setTimeout(()=>{
        task.cancel()
        .then((res)=>{
            console.log('-----------cancel res------------')
            console.log(res as GitTaskCancelResult)
            console.log('^^^^^^^^^^^cancel res^^^^^^^^^^^^')
        })
        .catch((err)=>{
            console.log('-----------cancel err------------')
            console.log(err)
            console.log('^^^^^^^^^^^cancel err^^^^^^^^^^^^')
        })
    }, 10)


}catch(e){
        console.log('-----------error------------')
        console.log(e)
        console.log('^^^^^^^^^^^error^^^^^^^^^^^^')
}

This will return 0 from cancel() operation. But eventually the clone task will continue.

Feel free to fire away any question.

Here is the toy project: https://github.com/maifeeulasad/dugite-exectask-playland. Right now this project is marked as private, but I have invited @tidy-dev and @sergiou87. Here is the invitation link: https://github.com/maifeeulasad/dugite-exectask-playland/invitations. If any collaborator thinks I should make it public, just let me know. And I'm avaiable to work on this problem. Really looking forward to contribute to GitHub Desktop.

[maifeeulasad]

Okay, here is my investigation (till now):

During just one clone operation, multiple process of Git is being created/instantiated. And they are not child of the parent process, so it can be one reason, why this is not working.

Here is a snap showing, multiple instance of Git, and they are not child of one process:

Additional information: Rows: CPU, Memory, Disk, Network So the last process is cloing the repo for sure. But look at the second last process, it is also doing something

[tidy-dev]

@maifeeulasad Love your enthusiasm to help! Just wanted to let you know that currently the team is pretty covered up, and cancelling a clone is not a high priority as it impacts a small number of users. Thus, we likely won't be able to give this much time, but of course feel free to continue exploring on your own.

[maifeeulasad]

Dear,

Today, I'll try to do a quick knowledge transfer session for this issue. This only contains the most recent and needed information. I'll share a more detailed version sometimes later on. And, I've tried adding all the necessary links as references at the very bottom. If I miss something, I'll mention them later.

Now, let's get into business.

Introduction

When we implemented the git operation cancelling functionality by introducing a new class GitTask and a new method in it called cancel, it was working fine with the test cases we considered.

But when I was trying to cancel a clone process, I couldn't. Upon further investigation, I found that the git clone operation is actually creating 5 different child processes.

• To cancel them, you need to press CTRL + C in the attached console/terminal, which means I have to terminate the process in the application level.
• Or we could create a parent process and assign those 5 child processes to it, and if the parent process exists, it will also terminate those child processes as well. But this will require changing the Git's code base. I even mailed to the git mailing list ([email protected]). But they didn't even responded. Maybe we will talk about it sometime later, or maybe not.

So I tried exploring the first step, and all my work here revolves / evolves around the first approach.

The Issue

When I did more digging, I found that this issue only persists in Windows; in my Linux environment, it was working fine. The kill method from Node.js's process works fine, but it doesn't work on Windows, more specifically *nix. CTRL + C creates a new thread in the child process to do its work, and there's no supported way to do this programmatically. However, there are some unsupported ways to achieve this. One is to create a new thread in the target application and let it do the signaling. Another option is to create a native host process that does nothing but create a console and run the actual program you want to run, then listen for an event and call GenerateConsoleCtrlEvent when the event is signaled. This is something I have found on Stack Overflow.

The Solution

After lots of digging, here is a native code that I could think of, to get our job done:

Function SigintWindows takes FunctionCallbackInfo args as input:
  If OS is Windows: # as including windows was the reason behind build failure in other OS than windows
    Create a DWORD processId from the argument
    Open the process with SYNCHRONIZE and PROCESS_TERMINATE access rights and processId
    If the process is not opened successfully:
      Throw an Exception
    Try to attach to the console of the process with processId
    If attaching to the console fails:
      Throw an Exception
      Try sending a Ctrl-C event directly
      If sending the Ctrl-C event fails:
        Throw an Exception
        Close the process handle and return
      Else:
        Set the return value of args to true and return
    Else:
      Disable the Ctrl-C handling for the program
      If disabling the Ctrl-C handling fails:
        Throw an Exception
        Close the process handle and return
      Send a Ctrl-C event to the console of the process
      If sending the Ctrl-C event fails:
        Throw an Exception
        Re-enable the Ctrl-C handling for the program
        Free the console
        Close the process handle and return
      Else:
        Wait for the process to exit within 2 seconds
        If the process does not exit within 2 seconds:
          Throw an Exception
        Re-enable the Ctrl-C handling for the program
        Free the console
        Close the process handle and return
      End If
    End If
  End If
End Function

The Implementation

Finally, back in our codebase, after figuring out all these, I've made some major changes in the code:

• Introducing native code to our code base
  • You may find a file named lb/ctrlc.cc; it contains the implementation of the pseudo code I just shared.
• Project Modification
  • And I've added some configuration using node-gyp to build the native code
  • And I've added a script in the scripts folder to actually copy the newly created/built ctrlc.node to our lib directory. You may find script/copy-native-build.js.
• Existing source (lib) modification
  • We need to keep track of our operation type, as all the operations can not be cancelled the same way; clone takes CTRL + C, exiting merge window takes :q.
  • So, I introduced a GitTaskInfo and GitActionType to hold operation type along with the PID.
  • Then, I was checking the operation type and OS and calling that native function, or just kill

Litmus Test:

Now, I've introduced a new test, specifically for cloning. After canceling the clone operation, it will list the target directory (where we tried cloning), and if the cancel operation successfully completes, it should give us with an empty directory, along with returning a success code.

The Outcome

Yes, the operation is being canceled successfully, and as a result, the target directory is empty. I've checked from Task Manager and Process Explorer.

The Issue with the Solution

As we are putting CTRL + C in the terminal, it is also canceling the parent process, for our case, the running test-suits. So, the suite is returning an exit code of 1 and the remaining tests are not even running.

Now I'm working on this "issue with the solution" part.

Here is the PR link: https://github.com/desktop/dugite/pull/536/files. As this PR is already messed up with lots commits, instead of squashing commits, maybe I'll create a fresh PR when I'm all done

Ref:

- https://github.com/desktop/dugite/pull/448
- https://github.com/desktop/dugite/pull/495
- https://stackoverflow.com/q/41976651/10305444
- https://stackoverflow.com/a/1179124/10305444
- https://stackoverflow.com/a/15281070/10305444

Intentionally setting it as a codeblock so that our notification inbox doesn't get spammed

Surely it's a major modification, but I'm all in into this. Please let me know if you have any questions, doubts, or anything. And I thought it's better to share what I am doing / have done with you all.

Good day. Keep me in your prayers. 🌻

[zhoushaokun]

Is there a way to kill git process by pid using some node libs, for examples:

• https://www.npmjs.com/package/terminate
• https://www.npmjs.com/package/fkill which are all need a pid. But the pid has't returned by execTask.

I think if it is too difficult to kill a process，shell we just export the pid?

[niik]

Hey @maifeeulasad, as I stated in https://github.com/desktop/dugite/pull/536#issuecomment-2419714751 this is out of scope for dugite I'm afraid. Try using the processCallback option to get a reference to the ChildProcess which will let you send the signals you'd like directly to the process.