Skip to content
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

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

Merged
merged 2 commits into from
Nov 7, 2024

Conversation

leonvandermeer
Copy link
Contributor

Set ConsoleCancelEventArgs.Cancel to true so that Benchmark process continues and PowerPlan is reverted at end of aborted Benchmark.

Note that at Ctrl-C (or Ctrl-Break) press, current benchmark fails (as all its processes are killed). The main run loop continues running remaining benchmarks.

As it would (imho) require some complicated code to set stop to true at Ctrl-C press, I decided to document current behavior.

(If you do like to have all benchmarks aborted at Ctrl-C press, let me know and I'll implement such behavior)

@timcassell
Copy link
Collaborator

I don't think it's desirable for ctrl-c to abort only a single benchmark. I use it frequently to abort a benchmark run altogether.

@leonvandermeer leonvandermeer marked this pull request as draft October 29, 2024 17:34
…otnet#2483

* PowerManagementApplier and ConsoleTitler system state is now reverted at
  Process termination.
* To prevent code duplication, DisposeAtProcessTermination class is
  introduced.
@leonvandermeer
Copy link
Contributor Author

Tim, I first tried to implement a kind of 'ConsoleControl' that ensures the process is not terminated (by setting ConsoleCancelEventArgs.Cancel to true from Console.CancelKeyPress event handler. And then bail out from the benchmark runner loop. More or less similar to the CancellationToken that's passed to ExecuteAsync in a .NET Generic Host worker app.
However, that would not work during compilation stage. It also did not work for the inprocess toolchain.

While browsing BDN's source code for what to do, I discovered that some other classes already reverted system state at process termination. Then I choose to follow that pattern for both the power settings and the console title.

@leonvandermeer leonvandermeer marked this pull request as ready for review November 2, 2024 19:53
@timcassell
Copy link
Collaborator

I like your solution. One thing that I wonder is whether DisposeAtProcessTermination should inherit from CriticalFinalizerObject and implement the full disposable pattern.

@leonvandermeer
Copy link
Contributor Author

I like your solution. One thing that I wonder is whether DisposeAtProcessTermination should inherit from CriticalFinalizerObject and implement the full disposable pattern.

I added some xml documentation to explain the design and reason not to implement full disposable pattern. Please read 1.

Constrained Execution Regions and related classes as CriticalFinalizerObject (a good explanation is written here) are intended for high availability processes like SQL Server that must stay alive. The clr guarantees that your finalizer is called completely and in some more situations 2.

A finalizer is only needed when a class maintains native resources (e.g. a handle). DisposeAtProcessTermination does not and derived classes should not do either directly.
Derived classes shall delegate maintaining and closing of native resources to a more granular level, preferably using SafeHandle derived classes (and delegate explicit disposal to their Idisposable fields).
Note that some system state is pure managed code and does not keep any native resource alive (e.g. set and revert of Console.Title)

Footnotes

  1. Note that when BDN process is killed via e.g. task manager, not any finalizer will be called (and not any system state is reverted). But that's a more rare occasion, whereas Ctrl-C to abort is a feature.

  2. This is only supported on .Net Framework, not in .Net Core.

@timcassell
Copy link
Collaborator

timcassell commented Nov 7, 2024

A finalizer is only needed when a class maintains native resources

While not technically native resources, I would argue that system state falls in that category.

Anyway, I'm not saying it needs to be done, I was just curious if it should. Since you also hooked it up to the process exit event, the cases where a finalizer would be used are probably covered.

@timcassell timcassell linked an issue Nov 7, 2024 that may be closed by this pull request
Copy link
Collaborator

@timcassell timcassell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @leonvandermeer!

@timcassell timcassell merged commit b9d69a4 into dotnet:master Nov 7, 2024
8 checks passed
@timcassell timcassell added this to the v0.14.1 milestone Nov 7, 2024
@leonvandermeer leonvandermeer deleted the resetPowerPlan branch November 7, 2024 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pressing Ctrl-C does not reset power plan back
2 participants