feat: add safe command termination

[aikawayataro]

WIP implementation of command cancellation. Right now it only implements API for safe process termination, further implementation will require some refactoring.

This will address #753

[aikawayataro]

Cancelling git commands this way is safe, as git installs handlers for CTRL+C on Windows and few common signals on other systems (incl. SIGTERM), reference: sigchain.h

[aikawayataro]

@love-linger will need your help with this, don't know how to wrap it properly. Feel free to close this pull if you don't want to go for this feature

[love-linger]

I'll check this PR and plan to merge it into version 2025.07

[love-linger]

I've pushed a new branch feat/cancellable-command that contains this PR and makes Fetch popup cancellable.

I found that the callback of CancellationToken.Register has been called on Windows, but GenerateConsoleCtrlEvent((int)CTRL_EVENT.CTRL_C, process.Id) returns false

// Code in src/Commands/Command.cs
if (CancellationToken.CanBeCanceled)
{
    CancellationToken.Register(() =>
    {
        if (_proc != null && !_isDone)
            Native.OS.TerminateSafely(_proc); // this line has been called
    });
}

// Code in src/Native/Windows.cs
public void TerminateSafely(Process process)
{
    if (SetConsoleCtrlHandler(IntPtr.Zero, true))
    {
        try
        {
            if (GenerateConsoleCtrlEvent((int)CTRL_EVENT.CTRL_C, process.Id)) // Always returns false
                process.WaitForExit();
        }
        finally
        {
            SetConsoleCtrlHandler(IntPtr.Zero, false);
        }
    }
}

[aikawayataro]

Oh, it should be a draft, not ready for merge, you could push to this merge request. I will investigate the issue, now I can debug it properly, thanks!

[love-linger]

I pushed some new commits. It seems to fix the above issue (at least in my tests).

@aikawayataro Could you please test it?

[aikawayataro]

I've tested it on Linux, it does work, git process exits properly and cleans up partially cloned repository, but the clone popup will stay forever. Windows works just good.

[love-linger]

I've tested it on Linux, it does work, git process exits properly and cleans up partially cloned repository, but the clone popup will stay forever. Windows works just good.

Can you debug on Linux to see why the popup stacks? My Linux equipment is not around.

[aikawayataro]

It hangs at

sourcegit/src/Commands/Command.cs

Line 102 in 76ab7d2

proc.WaitForExit();

because of dotnet/runtime#29232 (comment)

I found out that WaitForExit(int) behaves differently and came up with this workaround:

diff --git a/src/Commands/Command.cs b/src/Commands/Command.cs
index 4d368d30..a0bb8bf2 100644
--- a/src/Commands/Command.cs
+++ b/src/Commands/Command.cs
@@ -99,7 +99,8 @@ namespace SourceGit.Commands
             {
                 proc.BeginOutputReadLine();
                 proc.BeginErrorReadLine();
-                proc.WaitForExit();
+                while (!proc.HasExited)
+                    proc.WaitForExit(1000);
 
                 exitCode = proc.ExitCode;
                 proc.Close();

[love-linger]

How about close output/error stream manually?

public void TerminateSafely(Process process)
{
    if (kill(process.Id, (int)SIGNAL.TERM) == 0)
    {
        process.StandardOutput.Close();
        process.StandardError.Close();
    }    
}

[aikawayataro]

Will crash with:

Crash::: System.AggregateException: One or more errors occurred. (Cannot mix synchronous and asynchronous operation on process stream.)

----------------------------
Version: 2025.6.0.0
OS: Unix 6.12.15.1
Framework: .NETCoreApp,Version=v9.0
Source: System.Private.CoreLib
Thread Name: Unnamed
User: user
App Start Time: 2/25/2025 10:39:03 AM
Exception Time: 2/25/2025 10:39:15 AM
Memory Usage: 254 MB
---------------------------

System.AggregateException: One or more errors occurred. (Cannot mix synchronous and asynchronous operation on process stream.)
 ---> System.InvalidOperationException: Cannot mix synchronous and asynchronous operation on process stream.
   at System.Diagnostics.Process.get_StandardOutput()
   at SourceGit.Native.Linux.TerminateSafely(Process process) in /home/user/Projects/sourcegit/src/Native/Linux.cs:line 112
   at SourceGit.Native.OS.TerminateSafely(Process process) in /home/user/Projects/sourcegit/src/Native/OS.cs:line 175
   at SourceGit.Commands.Command.<>c__DisplayClass34_0.<Exec>b__2() in /home/user/Projects/sourcegit/src/Commands/Command.cs:line 79
   at System.Threading.CancellationTokenSource.Invoke(Delegate d, Object state, CancellationTokenSource source)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---

[love-linger]

If the workaround metioned above (the while statement) fix the issue, I'll add it and test it on my mac later.

[aikawayataro]

Yes, it fixes the issue. There is remark at https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.waitforexit?view=net-9.0#system-diagnostics-process-waitforexit(system-int32) that says:

When standard output has been redirected to asynchronous event handlers, it is possible that output processing will not have completed when this method returns. To ensure that asynchronous event handling has been completed, call the WaitForExit() overload that takes no parameter after receiving a true from this overload. To help ensure that the Exited event is handled correctly in Windows Forms applications, set the SynchronizingObject property.

But I think it's fine as we don't need any output after operation is canceled.

[love-linger]

I found that this code (while (!proc.HasExited) proc.WaitForExit(1000);) introduces more issues on Windows. For example: the local changes did not calculated properly.

[aikawayataro]

Yeah, it can be used only with selected commands I think. It is still a workaround, I will try to rewrite Command.Exec to fix the issue

[love-linger]

I suggest changing it in the Linux.TerminateSafely method. For example: try to add proc.WaitForExit (1000) after the kill is successful.

[aikawayataro]

It will not work since WaitForExit() in Command.Exec will still hang

[love-linger]

try this?

if (kill(process.Id, (int)SIGNAL.TERM) == 0)
{
    process.CancelOutputRead();
    process.CancelErrorRead();
}

Say sorry again. My Linux device is not around.

[aikawayataro]

Doesn't change anything.

Say sorry again. My Linux device is not around.

No problem 😁 I think I have an idea, will try it right now

[aikawayataro]

A bit dirty, but can't figure out anything else

diff --git a/src/Commands/Command.cs b/src/Commands/Command.cs
index 4d368d30..4ebda965 100644
--- a/src/Commands/Command.cs
+++ b/src/Commands/Command.cs
@@ -4,6 +4,7 @@ using System.Diagnostics;
 using System.Text;
 using System.Text.RegularExpressions;
 using System.Threading;
+using System.Threading.Tasks;
 using Avalonia.Threading;
 
 namespace SourceGit.Commands
@@ -39,37 +40,56 @@ namespace SourceGit.Commands
             var errs = new List<string>();
             var proc = new Process() { StartInfo = start };
 
-            proc.OutputDataReceived += (_, e) =>
+            var linelock = new object();
+            var stdoutTask = new Task(() =>
             {
-                if (e.Data != null)
-                    OnReadline(e.Data);
-            };
-
-            proc.ErrorDataReceived += (_, e) =>
+                string line;
+                while ((line = proc.StandardOutput.ReadLine()) != null)
+                {
+                    if (CancellationToken.IsCancellationRequested)
+                        break;
+                    lock (linelock)
+                    {
+                        OnReadline(line);
+                    }
+                }
+            });
+            var stderrTask = new Task(() =>
             {
-                if (string.IsNullOrEmpty(e.Data))
+                string line;
+                while (!CancellationToken.IsCancellationRequested && (line = proc.StandardError.ReadLine()) != null)
                 {
-                    errs.Add(string.Empty);
-                    return;
+                    if (CancellationToken.IsCancellationRequested)
+                        break;
+                    if (string.IsNullOrEmpty(line))
+                    {
+                        errs.Add(string.Empty);
+                        return;
+                    }
+
+                    if (TraitErrorAsOutput)
+                    {
+                        lock (linelock)
+                        {
+                            OnReadline(line);
+                        }
+                    }
+
+                    // Ignore progress messages
+                    if (line.StartsWith("remote: Enumerating objects:", StringComparison.Ordinal))
+                        return;
+                    if (line.StartsWith("remote: Counting objects:", StringComparison.Ordinal))
+                        return;
+                    if (line.StartsWith("remote: Compressing objects:", StringComparison.Ordinal))
+                        return;
+                    if (line.StartsWith("Filtering content:", StringComparison.Ordinal))
+                        return;
+                    if (REG_PROGRESS().IsMatch(line))
+                        return;
+
+                    errs.Add(line);
                 }
-
-                if (TraitErrorAsOutput)
-                    OnReadline(e.Data);
-
-                // Ignore progress messages
-                if (e.Data.StartsWith("remote: Enumerating objects:", StringComparison.Ordinal))
-                    return;
-                if (e.Data.StartsWith("remote: Counting objects:", StringComparison.Ordinal))
-                    return;
-                if (e.Data.StartsWith("remote: Compressing objects:", StringComparison.Ordinal))
-                    return;
-                if (e.Data.StartsWith("Filtering content:", StringComparison.Ordinal))
-                    return;
-                if (REG_PROGRESS().IsMatch(e.Data))
-                    return;
-
-                errs.Add(e.Data);
-            };
+            });
 
             if (CancellationToken.CanBeCanceled)
             {
@@ -83,6 +103,8 @@ namespace SourceGit.Commands
             try
             {
                 proc.Start();
+                stdoutTask.Start();
+                stderrTask.Start();
             }
             catch (Exception e)
             {
@@ -97,9 +119,9 @@ namespace SourceGit.Commands
             int exitCode;
             try
             {
-                proc.BeginOutputReadLine();
-                proc.BeginErrorReadLine();
                 proc.WaitForExit();
+                if (!CancellationToken.IsCancellationRequested)
+                    Task.WaitAll([stdoutTask, stderrTask]);
 
                 exitCode = proc.ExitCode;
                 proc.Close();

[love-linger]

I found that follow code just works find on my Ubuntu 24.04.

public void TerminateSafely(Process process)
{
    Process.Start("kill", $"-15 {process.Id}");           
}

And I found that when the orignal code proc.WaitForExit() hangs after calling kill(process.Id, 15), the main git clone process has gone but its children process git-remote-https leaves.

[love-linger]

One more thing. I've tested the orignal code on my Mac, it works fine.

[love-linger]

I found that follow code just works find on my Ubuntu 24.04.

public void TerminateSafely(Process process)
{
    Process.Start("kill", $"-15 {process.Id}");           
}

Sorry. This was the wrong conclusion...

Confirmed. macOS has the same issue.

[aikawayataro]

Does my patch works to fix the issue on your side? The motivation is avoid async reading and to stop reading after cancellation is requested.

And I found that when the orignal code proc.WaitForExit() hangs after calling kill(process.Id, 15), the main git clone process has gone but its children process git-remote-https leaves.

That is the exact reason it hangs, because of children and grandchildren keeping output open, WaitForExit() when used with async readline API will wait until output is closed. I also have an another idea:

diff --git a/src/Commands/Command.cs b/src/Commands/Command.cs
index 4d368d30..af1f0955 100644
--- a/src/Commands/Command.cs
+++ b/src/Commands/Command.cs
@@ -41,12 +41,18 @@ namespace SourceGit.Commands
 
             proc.OutputDataReceived += (_, e) =>
             {
+                if (CancellationToken.IsCancellationRequested)
+                    return;
+
                 if (e.Data != null)
                     OnReadline(e.Data);
             };
 
             proc.ErrorDataReceived += (_, e) =>
             {
+                if (CancellationToken.IsCancellationRequested)
+                    return;
+
                 if (string.IsNullOrEmpty(e.Data))
                 {
                     errs.Add(string.Empty);
@@ -99,7 +105,12 @@ namespace SourceGit.Commands
             {
                 proc.BeginOutputReadLine();
                 proc.BeginErrorReadLine();
-                proc.WaitForExit();
+                while (!proc.HasExited)
+                {
+                    proc.WaitForExit(1000);
+                }
+                if (!CancellationToken.IsCancellationRequested)
+                    proc.WaitForExit();
 
                 exitCode = proc.ExitCode;
                 proc.Close();

It should fix this:

I found that this code (while (!proc.HasExited) proc.WaitForExit(1000);) introduces more issues on Windows. For example: the local changes did not calculated properly.

[love-linger]

I just do not want to change the orignal Command.Exec since this issue is not caused by it.

Finally, I found the solution:

public void TerminateSafely(Process process)
{
    Process.Start("pkill", $"--signal 15 -P {process.Id}");
}

[love-linger]

The main reason for this problem is exactly what I observed above, taking git clone as an example: after calling kill(process.Id, 15), the git clone process has exited correctly, but its children process git-remote-https remains.

After changing to pkill --signal 15 -P PARENT_PID, the git process and all its children processes exit correctly.

[aikawayataro]

I don't think pkill -P is the right thing, it will not send SIGTERM to git process but to its children and grandchildren, leaving some possibility of bugs.

EDIT: it will send SIGTERM only to its children, grandchildren will not get any signal.

[love-linger]

how about add an extra `kill()` after pkill?

[aikawayataro]

I don't think that it is a good idea to kill children first, but it should work. I think the best solution is to spawn git in separate process group, and use kill((-pid), 15) to kill the whole process group, it is more complicated though (dotnet doesn't expose API for that).

[love-linger]

I tested the two solutions you provided above. After aborting the process, it is true that the interface logic will not get stuck, but the key problem remaining in the child process has not been solved. Although after a certain period of time (very long in my test, about 30 seconds at least), these child processes will be recycled.

[love-linger]

I don't think that it is a good idea to kill children first, but it should work. I think the best solution is to spawn git in separate process group, and use kill((-pid), 15) to kill the whole process group, it is more complicated though (dotnet doesn't expose API for that).

dotnet/runtime#44944

[aikawayataro]

the key problem remaining in the child process has not been solved.

Yes could be a problem, even if they die out after time

dotnet/runtime#44944

Yes, have seen that one. Sadly, there's no movement on this feature