ExitCode can be obtained only by explicitly awaiting for it.

Eliminated the race condition when OnProcessExit and Dispose are running concurrently.
Only these two methods are synced because other public methods of ProcessWrapper are not
designed to be threadsafe - the same way as the Process class.

Closes gitextensions#5362.

Author: jbialobr

GitCommands/Git/Executable.cs

@@ -7,6 +7,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using GitCommands.Logging;
+using GitUI;
 using GitUIPluginInterfaces;
 using JetBrains.Annotations;
 
@@ -53,17 +54,15 @@ public IProcess Start(ArgumentString arguments = default, bool createWindow = fa
         private sealed class ProcessWrapper : IProcess
         {
             // TODO should this use TaskCreationOptions.RunContinuationsAsynchronously
-            private readonly TaskCompletionSource<int?> _exitTaskCompletionSource = new TaskCompletionSource<int?>();
+            private readonly TaskCompletionSource<int> _exitTaskCompletionSource = new TaskCompletionSource<int>();
 
+            private readonly object _syncRoot = new object();
             private readonly Process _process;
             private readonly ProcessOperation _logOperation;
             private readonly bool _redirectInput;
             private readonly bool _redirectOutput;
 
-            private int _disposed;
-
-            /// <inheritdoc />
-            public int? ExitCode { get; private set; }
+            private bool _disposed;
 
             [SuppressMessage("ReSharper", "AssignNullToNotNullAttribute")]
             public ProcessWrapper(string fileName, string arguments, string workDir, bool createWindow, bool redirectInput, bool redirectOutput, [CanBeNull] Encoding outputEncoding)
@@ -102,15 +101,17 @@ public ProcessWrapper(string fileName, string arguments, string workDir, bool cr
 
             private void OnProcessExit(object sender, EventArgs eventArgs)
             {
-                // The Exited event can be raised after the process is disposed, however
-                // if the Process is disposed then reading ExitCode will throw.
-                if (_disposed == 0)
+                lock (_syncRoot)
                 {
-                    ExitCode = _process.ExitCode;
+                    // The Exited event can be raised after the process is disposed, however
+                    // if the Process is disposed then reading ExitCode will throw.
+                    if (!_disposed)
+                    {
+                        var exitCode = _process.ExitCode;
+                        _logOperation.LogProcessEnd(exitCode);
+                        _exitTaskCompletionSource.TrySetResult(exitCode);
+                    }
                 }
-
-                _logOperation.LogProcessEnd(ExitCode);
-                _exitTaskCompletionSource.TrySetResult(ExitCode);
             }
 
             /// <inheritdoc />
@@ -159,32 +160,30 @@ public StreamReader StandardError
             public void WaitForInputIdle() => _process.WaitForInputIdle();
 
             /// <inheritdoc />
-            public Task<int?> WaitForExitAsync() => _exitTaskCompletionSource.Task;
+            public Task<int> WaitForExitAsync() => _exitTaskCompletionSource.Task;
 
             /// <inheritdoc />
-            public int? WaitForExit()
+            public int WaitForExit()
             {
-                if (_disposed != 0)
-                {
-                    return ExitCode;
-                }
-
-                _process.WaitForExit();
-
-                return GitUI.ThreadHelper.JoinableTaskFactory.Run(() => _exitTaskCompletionSource.Task);
+                return ThreadHelper.JoinableTaskFactory.Run(() => WaitForExitAsync());
             }
 
             /// <inheritdoc />
             public void Dispose()
             {
-                if (Interlocked.CompareExchange(ref _disposed, 1, 0) != 0)
+                lock (_syncRoot)
                 {
-                    return;
+                    if (_disposed)
+                    {
+                        return;
+                    }
+
+                    _disposed = true;
                 }
 
                 _process.Exited -= OnProcessExit;
 
-                _exitTaskCompletionSource.TrySetResult(null);
+                _exitTaskCompletionSource.TrySetCanceled();
 
                 _process.Dispose();
 

GitCommands/Git/ExecutableExtensions.cs

@@ -100,7 +100,7 @@ public static async Task<string> GetOutputAsync(
                 output = outputBuffer.ToArray();
                 error = errorBuffer.ToArray();
 
-                if (cache != null && process.ExitCode == 0)
+                if (cache != null && await exitTask == 0)
                 {
                     cache.Add(arguments, output, error);
                 }

GitCommands/Git/IProcess.cs

@@ -43,26 +43,17 @@ public interface IProcess : IDisposable
         /// when calling <see cref="IExecutable.Start"/>.</exception>
         StreamReader StandardError { get; }
 
-        /// <summary>
-        /// Gets any exit code returned by this process.
-        /// </summary>
-        /// <remarks>
-        /// This value will be <c>null</c> if the process is still running, or if this object was disposed
-        /// before the process exited.
-        /// </remarks>
-        int? ExitCode { get; }
-
         /// <summary>
         /// Blocks the calling thread until the process exits, or when this object is disposed.
         /// </summary>
         /// <returns>The process's exit code, or <c>null</c> if this object was disposed before the process exited.</returns>
-        int? WaitForExit();
+        int WaitForExit();
 
         /// <summary>
         /// Returns a task that completes when the process exits, or when this object is disposed.
         /// </summary>
         /// <returns>A task that yields the process's exit code, or <c>null</c> if this object was disposed before the process exited.</returns>
-        Task<int?> WaitForExitAsync();
+        Task<int> WaitForExitAsync();
 
         /// <summary>
         /// Waits for the process to reach an idle state.

GitCommands/Logging/CommandLog.cs

@@ -44,6 +44,7 @@ public void NotifyDisposed()
             if (_entry.Duration == null)
             {
                 _entry.Duration = _stopwatch.Elapsed;
+                _raiseCommandsChanged();
             }
         }
     }

UnitTests/GitCommandsTests/MockExecutable.cs

@@ -6,6 +6,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using GitCommands;
+using GitUI;
 using GitUIPluginInterfaces;
 using JetBrains.Annotations;
 using NUnit.Framework;
@@ -94,37 +95,45 @@ public IProcess Start(ArgumentString arguments, bool createWindow, bool redirect
 
         private sealed class MockProcess : IProcess
         {
-            public MockProcess([CanBeNull] string output, int? exitCode)
+            public MockProcess([CanBeNull] string output, int? exitCode = 0)
             {
                 StandardOutput = new StreamReader(new MemoryStream(Encoding.UTF8.GetBytes(output ?? "")));
                 StandardError = new StreamReader(new MemoryStream());
                 StandardInput = new StreamWriter(new MemoryStream());
-                ExitCode = exitCode;
+                _exitCode = exitCode;
             }
 
             public MockProcess()
             {
                 StandardOutput = new StreamReader(new MemoryStream());
                 StandardError = new StreamReader(new MemoryStream());
                 StandardInput = new StreamWriter(new MemoryStream());
+                _exitCode = 0;
             }
 
+            private int? _exitCode;
             public StreamWriter StandardInput { get; }
             public StreamReader StandardOutput { get; }
             public StreamReader StandardError { get; }
 
-            public int? ExitCode { get; }
-
-            public int? WaitForExit()
+            public int WaitForExit()
             {
-                // TODO implement if needed
-                return 0;
+                return ThreadHelper.JoinableTaskFactory.Run(() => WaitForExitAsync());
             }
 
-            public Task<int?> WaitForExitAsync()
+            public Task<int> WaitForExitAsync()
             {
-                // TODO implement if needed
-                return Task.FromResult((int?)0);
+                if (_exitCode.HasValue)
+                {
+                    return Task.FromResult(_exitCode.Value);
+                }
+                else
+                {
+                    var cts = new CancellationTokenSource();
+                    var ct = cts.Token;
+                    cts.Cancel();
+                    return Task.FromCanceled<int>(ct);
+                }
             }
 
             public void WaitForInputIdle()