Cancellation for Executable

[gerhardol]

Fixes #9440
#9440 is mostly closed in #9505, this handles a few effects
Closes #9501

Proposed changes

Long running process like "git difftool --tool-help" are currently never cancelled, they must run to exit.
I have seen this command require over a minute at work.
After #9505, there is no major problem due to this, it is possible to open the console and close GE quickly after opening, but it could be a potential issue.

• CancellationToken for Executable
  Possibility to cancel processes, not hanging until closing.
  There may be other ways to do this too.

• CancellationToken handling for CustomDiffMergeTools
  Runs in the background and was never cancelled before. This add cancellation token handling and also cancels the Git command.
  A few other cleanups, review commit by commit

Test methodology

Manual

• Start Settings to clear the cache for merge/diff tools
• F12 for command log
• Open Solve merge conflicts... form
• Notice mergetool --tool-help in the list
• Close ResolveConflicts
• Notice that the time is updated immediately, no exit code is written (before the command always ran to completion

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

---

✒️ I contribute this code under The Developer Certificate of Origin.

[gerhardol]

We need to kill the process when cancellation is requested

See #9522 (comment)

[gerhardol]

Minor review comments, rebased to add cancellation after #9524 was merged #9524 (review)

[RussKie]

👍

[gerhardol]

Rebased and squashed and updated commit messages, rebase merge tomorrow

Renamed one more exitTask method

[RussKie]

This name is confusing and wrong. It returns an int and not a task. The same goes for the async version.

[gerhardol]

The method "waits for the exitTask" created when the process exits, including the extra work to add to log etc.
It is not a normal WaitForExit() that waits for the process task to exit.

I still think the name is better.
But what about WaitForProcessExit()?

[drewnoakes]

It is the non-async form of WaitForExitAsync so should be called WaitForExit in order to be consistent with the rest of the framework (e.g. Read/ReadAsync, Write/WriteAsycn, ...).

[gerhardol]

But there are two variants of WaitForExit() now: One that waits for the process to exit (that I added, cancellable) and the existing that waits for the additional exitTask to exit. I find the new method to follow the intention of the WaitForExit() method better, but I can rename that instead.

[RussKie]

👍

Once on .NET 6.0 we will have this: https://devblogs.microsoft.com/dotnet/new-dotnet-6-apis-driven-by-the-developer-community/#waitasync-improvements

[gerhardol]

👍

Was that for calling the existing exitTask as WaitForProcessExit() or reverting to WaitForExit() and rename the new cancellable method (call that WaitForProcessExit() instead?`)?

Once on .NET 6.0 we will have this: https://devblogs.microsoft.com/dotnet/new-dotnet-6-apis-driven-by-the-developer-community/#waitasync-improvements

Unfortunately no change here, the additional exitTask is the problem here it seems.

[RussKie]

It's almost a midnight here, and you're asking complex questions :D

[RussKie]

So now we have:

/// Blocks the calling thread until the process exits, or when this object is disposed.
int WaitForProcessExit();

/// Returns a task that completes when the process exits, or when this object is disposed.
Task<int> WaitForProcessExitAsync();

/// Instructs the process component to wait for the associated process to exit, or for the cancellationToken to be cancelled (cancells the process).
Task WaitForExitAsync(CancellationToken token);

Maybe we can do something like this, and remove the need for the new method?

            public Task<int> WaitForProcessExitAsync(CancellationToken token)
            {
                token.Register(() => _exitTaskCompletionSource.SetCanceled(token));
                return _exitTaskCompletionSource.Task;
            }

[gerhardol]

Not really an option, exitTask is awaiten together with tasks for stdout and possibly stderr.

[gerhardol]

May be an option, I do not get the cancellation working though.
Will be away from the compiler for a few days

[gerhardol]

I did not get this working, just renaming

[gerhardol]

Rebased, renamed WaitForExitAsync back and the new cancellation method to WaitForProcessExitAsync (was not able to merge them, probable possible but lets settle with this).
Plan to squash tomorrow, merge one day later

[RussKie]

We can iterate on this later, if necessary.

[gerhardol]

We can iterate on this later, if necessary.

I am considering setting the priority for the long running and Git "background" processes to BelowNormal
https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.priorityclass?view=net-5.0

Probably very little difference for users though

[RussKie]

How do we tell whether a process is expected to be a long running one?

[gerhardol]

How do we tell whether a process is expected to be a long running one?

By knowledge, those we know may be long (currently these two in the PR).
In the same way GE could decrease prio for git --no-optional-locks and maybe no console visible.

But this fit in other PRs. I may look at this after the blame/filter PRs are merged