-
Notifications
You must be signed in to change notification settings - Fork 267
feat: add safe command termination #1012
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
feat: add safe command termination #1012
Conversation
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
@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
I'll check this PR and plan to merge it into version 2025.07
e9d4377
into
sourcegit-scm:develop
Signed-off-by: leo <longshuang@msn.cn>
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); } } }
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!
Signed-off-by: leo <longshuang@msn.cn>
...h time (#1012) Signed-off-by: leo <longshuang@msn.cn>
Signed-off-by: leo <longshuang@msn.cn>
...1012) Signed-off-by: leo <longshuang@msn.cn>
I pushed some new commits. It seems to fix the above issue (at least in my tests).
@aikawayataro Could you please test it?
Remove unnecessary `process.WaitForExit()` since it has been called in `Command.Exec()` (We use `process.WaitForExit(2000)` on Windows because it is needed after `GenerateConsoleCtrlEvent`) Signed-off-by: leo <longshuang@msn.cn>
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.
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.
It hangs at
sourcegit/src/Commands/Command.cs
Line 102 in 76ab7d2
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();
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(); } }
Will crash with:
Crash::: System.AggregateException: One or more errors occurred. (Cannot mix synchronous and asynchronous operation on process stream.)
----------------------------
Version: 202560.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 ---
If the workaround metioned above (the while statement) fix the issue, I'll add it and test it on my mac later.
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.
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.
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
I suggest changing it in the Linux.TerminateSafely method. For example: try to add proc.WaitForExit (1000) after the kill is successful.
It will not work since WaitForExit() in Command.Exec will still hang
try this?
if (kill(process.Id, (int)SIGNAL.TERM) == 0) { process.CancelOutputRead(); process.CancelErrorRead(); }
Say sorry again. My Linux device is not around.
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
Signed-off-by: leo <longshuang@msn.cn>
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();
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 callingkill(process.Id, 15), the maingit cloneprocess has gone but its children processgit-remote-httpsleaves.
One more thing. I've tested the orignal code on my Mac, it works fine.
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.
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.
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}"); }
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.
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.
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).
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.
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).
the key problem remaining in the child process has not been solved.
Yes could be a problem, even if they die out after time
Yes, have seen that one. Sadly, there's no movement on this feature
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