Prevent premature end of the Benchmark process at Ctrl-C, fixes #2483 (#2661)

* Ensure revert of system state at Benchmark Process termination, fixes #2483

* PowerManagementApplier and ConsoleTitler system state is now reverted at
  Process termination.
* To prevent code duplication, DisposeAtProcessTermination class is
  introduced.

* Apply suggestions from code review and add documentation

Author: leonvandermeer

src/BenchmarkDotNet.Diagnostics.Windows/EtwDiagnoser.cs

@@ -8,14 +8,15 @@
 using BenchmarkDotNet.Analysers;
 using BenchmarkDotNet.Diagnosers;
 using BenchmarkDotNet.Exporters;
+using BenchmarkDotNet.Helpers;
 using BenchmarkDotNet.Loggers;
 using Microsoft.Diagnostics.Tracing;
 using Microsoft.Diagnostics.Tracing.Parsers;
 using Microsoft.Diagnostics.Tracing.Session;
 
 namespace BenchmarkDotNet.Diagnostics.Windows
 {
-    public abstract class EtwDiagnoser<TStats> where TStats : new()
+    public abstract class EtwDiagnoser<TStats> : DisposeAtProcessTermination where TStats : new()
     {
         internal readonly LogCapture Logger = new LogCapture();
         protected readonly Dictionary<BenchmarkCase, int> BenchmarkToProcess = new Dictionary<BenchmarkCase, int>();
@@ -39,11 +40,6 @@ protected void Start(DiagnoserActionParameters parameters)
             BenchmarkToProcess.Add(parameters.BenchmarkCase, parameters.Process.Id);
             StatsPerProcess.TryAdd(parameters.Process.Id, GetInitializedStats(parameters));
 
-            // Important: Must wire-up clean-up events prior to acquiring IDisposable instance (Session property)
-            // This is in effect the inverted sequence of actions in the Stop() method.
-            Console.CancelKeyPress += OnConsoleCancelKeyPress;
-            AppDomain.CurrentDomain.ProcessExit += OnProcessExit;
-
             Session = CreateSession(parameters.BenchmarkCase);
 
             EnableProvider();
@@ -80,11 +76,13 @@ protected virtual void EnableProvider()
         protected void Stop()
         {
             WaitForDelayedEvents();
+            Dispose();
+        }
 
-            Session.Dispose();
-
-            Console.CancelKeyPress -= OnConsoleCancelKeyPress;
-            AppDomain.CurrentDomain.ProcessExit -= OnProcessExit;
+        public override void Dispose()
+        {
+            Session?.Dispose();
+            base.Dispose();
         }
 
         private void Clear()
@@ -93,10 +91,6 @@ private void Clear()
             StatsPerProcess.Clear();
         }
 
-        private void OnConsoleCancelKeyPress(object sender, ConsoleCancelEventArgs e) => Session?.Dispose();
-
-        private void OnProcessExit(object sender, EventArgs e) => Session?.Dispose();
-
         private static string GetSessionName(string prefix, BenchmarkCase benchmarkCase, ParameterInstances? parameters = null)
         {
             if (parameters != null && parameters.Items.Count > 0)

src/BenchmarkDotNet.Diagnostics.Windows/EtwProfiler.cs

@@ -120,21 +120,10 @@ private void Start(DiagnoserActionParameters parameters)
         private void Stop(DiagnoserActionParameters parameters)
         {
             WaitForDelayedEvents();
-            string userSessionFile;
-            try
-            {
-                kernelSession.Stop();
-                heapSession?.Stop();
-                userSession.Stop();
-
-                userSessionFile = userSession.FilePath;
-            }
-            finally
-            {
-                kernelSession.Dispose();
-                heapSession?.Dispose();
-                userSession.Dispose();
-            }
+            string userSessionFile = userSession.FilePath;
+            kernelSession.Dispose();
+            heapSession?.Dispose();
+            userSession.Dispose();
 
             // Merge the 'primary' etl file X.etl (userSession) with any files that match .clr*.etl .user*.etl. and .kernel.etl.
             TraceEventSession.MergeInPlace(userSessionFile, TextWriter.Null);

src/BenchmarkDotNet.Diagnostics.Windows/Sessions.cs

@@ -5,13 +5,13 @@
 using BenchmarkDotNet.Diagnosers;
 using BenchmarkDotNet.Engines;
 using BenchmarkDotNet.Exporters;
+using BenchmarkDotNet.Extensions;
 using BenchmarkDotNet.Helpers;
 using BenchmarkDotNet.Loggers;
-using BenchmarkDotNet.Extensions;
+using BenchmarkDotNet.Running;
 using Microsoft.Diagnostics.Tracing;
 using Microsoft.Diagnostics.Tracing.Parsers;
 using Microsoft.Diagnostics.Tracing.Session;
-using BenchmarkDotNet.Running;
 
 namespace BenchmarkDotNet.Diagnostics.Windows
 {
@@ -90,7 +90,7 @@ internal override Session EnableProviders()
         }
     }
 
-    internal abstract class Session : IDisposable
+    internal abstract class Session : DisposeAtProcessTermination
     {
         private const int MaxSessionNameLength = 128;
 
@@ -114,27 +114,16 @@ protected Session(string sessionName, DiagnoserActionParameters details, EtwProf
                 BufferSizeMB = config.BufferSizeInMb,
                 CpuSampleIntervalMSec = config.CpuSampleIntervalInMilliseconds,
             };
-
-            Console.CancelKeyPress += OnConsoleCancelKeyPress;
-            AppDomain.CurrentDomain.ProcessExit += OnProcessExit;
         }
 
-        public void Dispose() => TraceEventSession.Dispose();
-
-        internal void Stop()
+        public override void Dispose()
         {
-            TraceEventSession.Stop();
-
-            Console.CancelKeyPress -= OnConsoleCancelKeyPress;
-            AppDomain.CurrentDomain.ProcessExit -= OnProcessExit;
+            TraceEventSession.Dispose();
+            base.Dispose();
         }
 
         internal abstract Session EnableProviders();
 
-        private void OnConsoleCancelKeyPress(object sender, ConsoleCancelEventArgs e) => Stop();
-
-        private void OnProcessExit(object sender, EventArgs e) => Stop();
-
         protected static string GetSessionName(BenchmarkCase benchmarkCase)
         {
             string benchmarkName = FullNameProvider.GetBenchmarkName(benchmarkCase);

src/BenchmarkDotNet/Helpers/DisposeAtProcessTermination.cs

@@ -0,0 +1,57 @@
+using System;
+using System.Runtime.InteropServices;
+
+namespace BenchmarkDotNet.Helpers
+{
+    /// <summary>
+    /// Ensures that explicit Dispose is called at termination of the Process.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// This class exists to help in reverting system state where C#'s using statement does not
+    /// suffice. I.e. when Benchmark's process is aborted via Ctrl-C, Ctrl-Break or via click on the
+    /// X in the upper right of Window.
+    /// </para>
+    /// <para>
+    /// Usage: Derive your clas that changes system state of this class. Revert system state in
+    /// override of <see cref="Dispose"/> implementation.
+    /// Use your class in C#'s using statement, to ensure system state is reverted in normal situations.
+    /// This class ensures your override is also called at process 'abort'.
+    /// </para>
+    /// <para>
+    /// Note: This class is explicitly not responsible for cleanup of Native resources. Of course,
+    /// derived classes can cleanup their Native resources (usually managed via
+    /// <see cref="SafeHandle"/> derived classes), by delegating explicit Disposal to their
+    /// <see cref="IDisposable"/> fields.
+    /// </para>
+    /// </remarks>
+    public abstract class DisposeAtProcessTermination : IDisposable
+    {
+        public DisposeAtProcessTermination()
+        {
+            Console.CancelKeyPress += OnCancelKeyPress;
+            AppDomain.CurrentDomain.ProcessExit += OnProcessExit;
+            // It does not make sense to include a Finalizer. We do not manage any native resource and:
+            // as we are subscribed to static events, it would never be called.
+        }
+
+        /// <summary>
+        /// Called when the user presses Ctrl-C or Ctrl-Break.
+        /// </summary>
+        private void OnCancelKeyPress(object? sender, ConsoleCancelEventArgs e)
+        {
+            if (!e.Cancel) { Dispose(); }
+        }
+
+        /// <summary>
+        /// Called when the user clicks on the X in the upper right corner to close the Benchmark's Window.
+        /// </summary>
+        private void OnProcessExit(object? sender, EventArgs e) => Dispose();
+
+        public virtual void Dispose()
+        {
+            Console.CancelKeyPress -= OnCancelKeyPress;
+            AppDomain.CurrentDomain.ProcessExit -= OnProcessExit;
+        }
+    }
+}

src/BenchmarkDotNet/Helpers/TaskbarProgress.cs

@@ -14,7 +14,7 @@ internal enum TaskbarProgressState
         Warning = Paused
     }
 
-    internal class TaskbarProgress : IDisposable
+    internal class TaskbarProgress : DisposeAtProcessTermination
     {
         private static readonly bool OsVersionIsSupported = OsDetector.IsWindows()
             // Must be windows 7 or greater
@@ -31,10 +31,6 @@ internal TaskbarProgress(TaskbarProgressState initialTaskbarState)
             {
                 com = Com.MaybeCreateInstanceAndSetInitialState(initialTaskbarState);
                 terminal = Terminal.MaybeCreateInstanceAndSetInitialState(initialTaskbarState);
-                if (IsEnabled)
-                {
-                    Console.CancelKeyPress += OnConsoleCancelEvent;
-                }
             }
         }
 
@@ -51,23 +47,20 @@ internal void SetProgress(float progressValue)
             {
                 throw new ArgumentOutOfRangeException(nameof(progressValue), "progressValue must be between 0 and 1 inclusive.");
             }
-            uint value = (uint) (progressValue * 100);
+            uint value = (uint)(progressValue * 100);
             com?.SetValue(value);
             terminal?.SetValue(value);
         }
 
-        private void OnConsoleCancelEvent(object sender, ConsoleCancelEventArgs e)
-            => Dispose();
-
-        public void Dispose()
+        public override void Dispose()
         {
             if (IsEnabled)
             {
                 SetState(TaskbarProgressState.NoProgress);
                 com = null;
                 terminal = null;
-                Console.CancelKeyPress -= OnConsoleCancelEvent;
             }
+            base.Dispose();
         }
 
         private sealed class Com

src/BenchmarkDotNet/Running/ConsoleTitler.cs

@@ -1,15 +1,15 @@
 using System;
 using System.IO;
 using BenchmarkDotNet.Detectors;
-using BenchmarkDotNet.Portability;
+using BenchmarkDotNet.Helpers;
 
 namespace BenchmarkDotNet.Running
 {
     /// <summary>
     /// Updates Console.Title, subject to platform capabilities and Console availability.
     /// Restores the original (or fallback) title upon disposal.
     /// </summary>
-    internal class ConsoleTitler : IDisposable
+    internal class ConsoleTitler : DisposeAtProcessTermination
     {
         /// <summary>
         /// Whether this instance has any effect. This will be false if the platform doesn't support Console retitling,
@@ -76,13 +76,14 @@ public void UpdateTitle(string title)
             }
         }
 
-        public void Dispose()
+        public override void Dispose()
         {
             if (IsEnabled)
             {
                 Console.Title = oldConsoleTitle;
                 IsEnabled = false;
             }
+            base.Dispose();
         }
     }
 }

src/BenchmarkDotNet/Running/PowerManagementApplier.cs

@@ -4,11 +4,10 @@
 using BenchmarkDotNet.Environments;
 using BenchmarkDotNet.Helpers;
 using BenchmarkDotNet.Loggers;
-using BenchmarkDotNet.Portability;
 
 namespace BenchmarkDotNet.Running
 {
-    internal class PowerManagementApplier : IDisposable
+    internal class PowerManagementApplier : DisposeAtProcessTermination
     {
         private static readonly Guid UserPowerPlan = new Guid("67b4a053-3646-4532-affd-0535c9ea82a7");
 
@@ -28,7 +27,11 @@ internal class PowerManagementApplier : IDisposable
 
         internal PowerManagementApplier(ILogger logger) => this.logger = logger;
 
-        public void Dispose() => ApplyUserPowerPlan();
+        public override void Dispose()
+        {
+            ApplyUserPowerPlan();
+            base.Dispose();
+        }
 
         internal static Guid Map(PowerPlan value) => PowerPlansDict[value];