From b9d69a439382aecab4f87f3ecec232bd5f907ace Mon Sep 17 00:00:00 2001 From: leonvandermeer <64834803+leonvandermeer@users.noreply.github.com> Date: Thu, 7 Nov 2024 21:46:34 +0100 Subject: [PATCH] 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 --- .../EtwDiagnoser.cs | 22 +++---- .../EtwProfiler.cs | 19 ++----- .../Sessions.cs | 23 ++------ .../Helpers/DisposeAtProcessTermination.cs | 57 +++++++++++++++++++ .../Helpers/TaskbarProgress.cs | 15 ++--- src/BenchmarkDotNet/Running/ConsoleTitler.cs | 7 ++- .../Running/PowerManagementApplier.cs | 9 ++- 7 files changed, 89 insertions(+), 63 deletions(-) create mode 100644 src/BenchmarkDotNet/Helpers/DisposeAtProcessTermination.cs diff --git a/src/BenchmarkDotNet.Diagnostics.Windows/EtwDiagnoser.cs b/src/BenchmarkDotNet.Diagnostics.Windows/EtwDiagnoser.cs index 80423d72cf..4205195dc5 100644 --- a/src/BenchmarkDotNet.Diagnostics.Windows/EtwDiagnoser.cs +++ b/src/BenchmarkDotNet.Diagnostics.Windows/EtwDiagnoser.cs @@ -8,6 +8,7 @@ using BenchmarkDotNet.Analysers; using BenchmarkDotNet.Diagnosers; using BenchmarkDotNet.Exporters; +using BenchmarkDotNet.Helpers; using BenchmarkDotNet.Loggers; using Microsoft.Diagnostics.Tracing; using Microsoft.Diagnostics.Tracing.Parsers; @@ -15,7 +16,7 @@ namespace BenchmarkDotNet.Diagnostics.Windows { - public abstract class EtwDiagnoser where TStats : new() + public abstract class EtwDiagnoser : DisposeAtProcessTermination where TStats : new() { internal readonly LogCapture Logger = new LogCapture(); protected readonly Dictionary BenchmarkToProcess = new Dictionary(); @@ -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) diff --git a/src/BenchmarkDotNet.Diagnostics.Windows/EtwProfiler.cs b/src/BenchmarkDotNet.Diagnostics.Windows/EtwProfiler.cs index 6641dd5a45..57f57d9e1a 100644 --- a/src/BenchmarkDotNet.Diagnostics.Windows/EtwProfiler.cs +++ b/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); diff --git a/src/BenchmarkDotNet.Diagnostics.Windows/Sessions.cs b/src/BenchmarkDotNet.Diagnostics.Windows/Sessions.cs index cf0b6fa88f..f0f5dcc475 100644 --- a/src/BenchmarkDotNet.Diagnostics.Windows/Sessions.cs +++ b/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); diff --git a/src/BenchmarkDotNet/Helpers/DisposeAtProcessTermination.cs b/src/BenchmarkDotNet/Helpers/DisposeAtProcessTermination.cs new file mode 100644 index 0000000000..12aa89a6fa --- /dev/null +++ b/src/BenchmarkDotNet/Helpers/DisposeAtProcessTermination.cs @@ -0,0 +1,57 @@ +using System; +using System.Runtime.InteropServices; + +namespace BenchmarkDotNet.Helpers +{ + /// + /// Ensures that explicit Dispose is called at termination of the Process. + /// + /// + /// + /// 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. + /// + /// + /// Usage: Derive your clas that changes system state of this class. Revert system state in + /// override of 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'. + /// + /// + /// Note: This class is explicitly not responsible for cleanup of Native resources. Of course, + /// derived classes can cleanup their Native resources (usually managed via + /// derived classes), by delegating explicit Disposal to their + /// fields. + /// + /// + 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. + } + + /// + /// Called when the user presses Ctrl-C or Ctrl-Break. + /// + private void OnCancelKeyPress(object? sender, ConsoleCancelEventArgs e) + { + if (!e.Cancel) { Dispose(); } + } + + /// + /// Called when the user clicks on the X in the upper right corner to close the Benchmark's Window. + /// + private void OnProcessExit(object? sender, EventArgs e) => Dispose(); + + public virtual void Dispose() + { + Console.CancelKeyPress -= OnCancelKeyPress; + AppDomain.CurrentDomain.ProcessExit -= OnProcessExit; + } + } +} diff --git a/src/BenchmarkDotNet/Helpers/TaskbarProgress.cs b/src/BenchmarkDotNet/Helpers/TaskbarProgress.cs index 94afb75311..e3910a0924 100644 --- a/src/BenchmarkDotNet/Helpers/TaskbarProgress.cs +++ b/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 diff --git a/src/BenchmarkDotNet/Running/ConsoleTitler.cs b/src/BenchmarkDotNet/Running/ConsoleTitler.cs index 7458388c42..d045e498aa 100644 --- a/src/BenchmarkDotNet/Running/ConsoleTitler.cs +++ b/src/BenchmarkDotNet/Running/ConsoleTitler.cs @@ -1,7 +1,7 @@ using System; using System.IO; using BenchmarkDotNet.Detectors; -using BenchmarkDotNet.Portability; +using BenchmarkDotNet.Helpers; namespace BenchmarkDotNet.Running { @@ -9,7 +9,7 @@ namespace BenchmarkDotNet.Running /// Updates Console.Title, subject to platform capabilities and Console availability. /// Restores the original (or fallback) title upon disposal. /// - internal class ConsoleTitler : IDisposable + internal class ConsoleTitler : DisposeAtProcessTermination { /// /// 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(); } } } diff --git a/src/BenchmarkDotNet/Running/PowerManagementApplier.cs b/src/BenchmarkDotNet/Running/PowerManagementApplier.cs index f240c1a209..f48d95d61e 100644 --- a/src/BenchmarkDotNet/Running/PowerManagementApplier.cs +++ b/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];