Add AbandonOnRepeatCancellation extension method #1227
+140
−0
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| // This file is used by Code Analysis to maintain SuppressMessage | ||
| // attributes that are applied to this project. | ||
| // Project-level suppressions either have no target or are given | ||
| // a specific target and scoped to a namespace, type, member, etc. | ||
|
|
||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
||
| [assembly: SuppressMessage("Reliability", | ||
| "CA2016: Forward the 'CancellationToken' parameter to methods that take one", | ||
| Justification = "Test is designed to illustrate behaviour with abandonment when cancellation token is not observed.")] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| using System.CommandLine.Builder; | ||
| using System.CommandLine.Invocation; | ||
| using System.CommandLine.Parsing; | ||
| using System.CommandLine.IO; | ||
| using System.Diagnostics; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace System.CommandLine.ManualTest.AbandonOnRepeatCancellation | ||
| { | ||
| public static class Program | ||
| { | ||
| public static async Task Main(string[] args) | ||
| { | ||
| var command = new RootCommand | ||
| { | ||
| Handler = CommandHandler.Create( | ||
| async (IConsole console, CancellationToken cancelToken) => | ||
| { | ||
| console.Out.WriteLine("Invocation started. Press Ctrl+C to request cancellation"); | ||
| using var cancelReg = cancelToken.Register(state => | ||
| { | ||
| var console = (IConsole)state; | ||
| console.Out.WriteLine("Cancellation requested. Press Ctrl+C again, to abandon invocation."); | ||
| }, console); | ||
| var stopwatch = new Stopwatch(); | ||
| stopwatch.Start(); | ||
| while (true) | ||
| { | ||
| console.Out.WriteLine($"Time since start: {stopwatch.Elapsed}, Is cancellation requested: {cancelToken.IsCancellationRequested}"); | ||
| await Task.Delay(TimeSpan.FromSeconds(0.75)) | ||
| .ConfigureAwait(continueOnCapturedContext: false); | ||
| } | ||
| }), | ||
| }; | ||
| var parser = new CommandLineBuilder(command) | ||
| .CancelOnProcessTermination() | ||
| .AbandonOnRepeatCancellation() | ||
| .Build(); | ||
| try | ||
| { | ||
| await parser | ||
| .InvokeAsync(args ?? Array.Empty<string>()) | ||
| .ConfigureAwait(continueOnCapturedContext: false); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| Console.WriteLine("Invocation was cancelled or abandoned."); | ||
| } | ||
|
|
||
| Console.WriteLine($"Back in {nameof(Main)}, waiting 5 seconds to exit."); | ||
| await Task.Delay(TimeSpan.FromSeconds(5)) | ||
| .ConfigureAwait(continueOnCapturedContext: false); | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <TargetFramework>net5.0</TargetFramework> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\System.CommandLine\System.CommandLine.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if enabling this functionality would make sense as a parameter to
CancelOnProcessTerminationrather than a separate method.Thoughts, @tmds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe as an optional parameter with a struct/class containing behaviour options. Among them bool abandon and int threshold?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we should add it to
CancelOnProcessTerminationto avoid weird interactions like https://github.com/dotnet/command-line-api/pull/1227/files#r605602596.CancelOnProcessTerminationgets included byUseDefaults. There is no pattern for passing patterns to the default middlewares that are used byUseDefaults.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there tools that you've seen where the force quit behavior would involve more than two CTRL-C presses? I'm wondering if the threshold needs to be configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, okay, we don't need a threshold. But @tmds has a good point. Should UseDefault include this abandoning behaviour? How would someone use UseDefault and abandon on secondary cancel?
Should we maybe also inject a state object into the Invocation that keeps a count of cancellations, so that people could react to a secondary cancel? In hosting the StopAsync method accepts a cancellation token to abort graceful shutdown and just force shutdown the host. We could use such an injected status object to control cancellation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intuition is that this behavior should be enabled by default so that the path of least resistance for the developer leads to the most consistent experience for end users.
@KathleenDollard?