From 8361a4a52a3e92f54715cbd82720af7d20278ff9 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 6 Feb 2023 13:20:54 +0100 Subject: [PATCH 01/19] add missing usings --- src/System.CommandLine/Invocation/InvocationPipeline.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/System.CommandLine/Invocation/InvocationPipeline.cs b/src/System.CommandLine/Invocation/InvocationPipeline.cs index 0251cbb30b..dcf7b8fb08 100644 --- a/src/System.CommandLine/Invocation/InvocationPipeline.cs +++ b/src/System.CommandLine/Invocation/InvocationPipeline.cs @@ -17,7 +17,7 @@ internal InvocationPipeline(ParseResult parseResult) public async Task InvokeAsync(IConsole? console = null, CancellationToken cancellationToken = default) { - var context = new InvocationContext(_parseResult, console, cancellationToken); + using InvocationContext context = new (_parseResult, console, cancellationToken); try { @@ -46,7 +46,7 @@ static async Task InvokeHandlerWithMiddleware(InvocationContext context) public int Invoke(IConsole? console = null) { - var context = new InvocationContext(_parseResult, console); + using InvocationContext context = new (_parseResult, console); try { From 02fb6a85110701f4f30daf085efb6bc920e06d2a Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 6 Feb 2023 13:24:59 +0100 Subject: [PATCH 02/19] delay the allocation of linked list of CancellationTokenRegistrations --- .../Invocation/InvocationContext.cs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/System.CommandLine/Invocation/InvocationContext.cs b/src/System.CommandLine/Invocation/InvocationContext.cs index 39076d22e8..054ea095a4 100644 --- a/src/System.CommandLine/Invocation/InvocationContext.cs +++ b/src/System.CommandLine/Invocation/InvocationContext.cs @@ -19,7 +19,7 @@ public sealed class InvocationContext : IDisposable private BindingContext? _bindingContext; private IConsole? _console; private readonly CancellationToken _token; - private readonly LinkedList _registrations = new(); + private LinkedList? _registrations; private volatile CancellationTokenSource? _source; /// The result of the current parse operation. @@ -120,7 +120,7 @@ internal void Cancel() public void LinkToken(CancellationToken token) { - _registrations.AddLast(token.Register(Cancel)); + (_registrations ??= new()).AddLast(token.Register(Cancel)); } /// @@ -143,9 +143,13 @@ public T GetValue(Argument argument) void IDisposable.Dispose() { Interlocked.Exchange(ref _source, null)?.Dispose(); - foreach (CancellationTokenRegistration registration in _registrations) + + if (_registrations is not null) { - registration.Dispose(); + foreach (CancellationTokenRegistration registration in _registrations) + { + registration.Dispose(); + } } } } From f4dd075c331c60f035016f24d4df057604e8315b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 6 Feb 2023 13:34:25 +0100 Subject: [PATCH 03/19] _source does not need to be volatile: it's initialized in ctor (always single thread) and accessed only with Interlocked.Exchange --- src/System.CommandLine/Invocation/InvocationContext.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.CommandLine/Invocation/InvocationContext.cs b/src/System.CommandLine/Invocation/InvocationContext.cs index 054ea095a4..950c21e35c 100644 --- a/src/System.CommandLine/Invocation/InvocationContext.cs +++ b/src/System.CommandLine/Invocation/InvocationContext.cs @@ -20,7 +20,7 @@ public sealed class InvocationContext : IDisposable private IConsole? _console; private readonly CancellationToken _token; private LinkedList? _registrations; - private volatile CancellationTokenSource? _source; + private CancellationTokenSource? _source; /// The result of the current parse operation. /// The console to which output is to be written. From 2ae46dfc5d14eebe67097f12a458abe71fba0d52 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 6 Feb 2023 13:34:57 +0100 Subject: [PATCH 04/19] InvocationPipeline can be static --- src/System.CommandLine/CommandExtensions.cs | 15 +++++------- .../Invocation/InvocationPipeline.cs | 23 ++++++++----------- .../Parsing/ParseResultExtensions.cs | 4 ++-- 3 files changed, 17 insertions(+), 25 deletions(-) diff --git a/src/System.CommandLine/CommandExtensions.cs b/src/System.CommandLine/CommandExtensions.cs index fb96cc19f8..4773c10412 100644 --- a/src/System.CommandLine/CommandExtensions.cs +++ b/src/System.CommandLine/CommandExtensions.cs @@ -26,7 +26,9 @@ public static int Invoke( string[] args, IConsole? console = null) { - return GetDefaultInvocationPipeline(command, args).Invoke(console); + ParseResult parseResult = command.GetOrCreateDefaultInvocationParser().Parse(args); + + return InvocationPipeline.Invoke(parseResult, console); } /// @@ -57,7 +59,9 @@ public static async Task InvokeAsync( IConsole? console = null, CancellationToken cancellationToken = default) { - return await GetDefaultInvocationPipeline(command, args).InvokeAsync(console, cancellationToken); + ParseResult parseResult = command.GetOrCreateDefaultInvocationParser().Parse(args); + + return await InvocationPipeline.InvokeAsync(parseResult, console, cancellationToken); } /// @@ -76,13 +80,6 @@ public static Task InvokeAsync( CancellationToken cancellationToken = default) => command.InvokeAsync(CommandLineStringSplitter.Instance.Split(commandLine).ToArray(), console, cancellationToken); - private static InvocationPipeline GetDefaultInvocationPipeline(Command command, string[] args) - { - var parseResult = command.GetOrCreateDefaultInvocationParser().Parse(args); - - return new InvocationPipeline(parseResult); - } - /// /// Parses an array strings using the specified command. /// diff --git a/src/System.CommandLine/Invocation/InvocationPipeline.cs b/src/System.CommandLine/Invocation/InvocationPipeline.cs index dcf7b8fb08..f7387e8c6a 100644 --- a/src/System.CommandLine/Invocation/InvocationPipeline.cs +++ b/src/System.CommandLine/Invocation/InvocationPipeline.cs @@ -8,22 +8,17 @@ namespace System.CommandLine.Invocation { - internal class InvocationPipeline + internal static class InvocationPipeline { - private readonly ParseResult _parseResult; - - internal InvocationPipeline(ParseResult parseResult) - => _parseResult = parseResult ?? throw new ArgumentNullException(nameof(parseResult)); - - public async Task InvokeAsync(IConsole? console = null, CancellationToken cancellationToken = default) + internal static async Task InvokeAsync(ParseResult parseResult, IConsole? console = null, CancellationToken cancellationToken = default) { - using InvocationContext context = new (_parseResult, console, cancellationToken); + using InvocationContext context = new (parseResult, console, cancellationToken); try { - if (context.Parser.Configuration.Middleware.Count == 0 && _parseResult.Handler is not null) + if (context.Parser.Configuration.Middleware.Count == 0 && parseResult.Handler is not null) { - return await _parseResult.Handler.InvokeAsync(context); + return await parseResult.Handler.InvokeAsync(context); } return await InvokeHandlerWithMiddleware(context); @@ -44,15 +39,15 @@ static async Task InvokeHandlerWithMiddleware(InvocationContext context) } } - public int Invoke(IConsole? console = null) + internal static int Invoke(ParseResult parseResult, IConsole? console = null) { - using InvocationContext context = new (_parseResult, console); + using InvocationContext context = new (parseResult, console); try { - if (context.Parser.Configuration.Middleware.Count == 0 && _parseResult.Handler is not null) + if (context.Parser.Configuration.Middleware.Count == 0 && parseResult.Handler is not null) { - return _parseResult.Handler.Invoke(context); + return parseResult.Handler.Invoke(context); } return InvokeHandlerWithMiddleware(context); // kept in a separate method to avoid JITting diff --git a/src/System.CommandLine/Parsing/ParseResultExtensions.cs b/src/System.CommandLine/Parsing/ParseResultExtensions.cs index d4df35a870..f67256d0bf 100644 --- a/src/System.CommandLine/Parsing/ParseResultExtensions.cs +++ b/src/System.CommandLine/Parsing/ParseResultExtensions.cs @@ -28,7 +28,7 @@ public static async Task InvokeAsync( this ParseResult parseResult, IConsole? console = null, CancellationToken cancellationToken = default) => - await new InvocationPipeline(parseResult).InvokeAsync(console, cancellationToken); + await InvocationPipeline.InvokeAsync(parseResult, console, cancellationToken); /// /// Invokes the appropriate command handler for a parsed command line input. @@ -39,7 +39,7 @@ public static async Task InvokeAsync( public static int Invoke( this ParseResult parseResult, IConsole? console = null) => - new InvocationPipeline(parseResult).Invoke(console); + InvocationPipeline.Invoke(parseResult, console); /// /// Formats a string explaining a parse result. From e23d9959492aa5ba599d6c45b7c46a0b03a8a75c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 6 Feb 2023 13:42:38 +0100 Subject: [PATCH 05/19] don't await a task-returning method when we can just return the task (less boilerplate and allocs) --- samples/HostingPlayground/Program.cs | 2 +- .../CommandLine/Perf_Parser_Directives_Suggest.cs | 4 ++-- .../CommandLine/Perf_Parser_TypoCorrection.cs | 4 ++-- .../DragonFruit/Perf_CommandLine_EntryPoint.cs | 8 ++++---- .../DragonFruit/Perf_CommandLine_Help.cs | 4 ++-- src/System.CommandLine.DragonFruit/CommandLine.cs | 8 ++++---- src/System.CommandLine/CommandExtensions.cs | 4 ++-- src/System.CommandLine/Parsing/ParseResultExtensions.cs | 4 ++-- src/System.CommandLine/Parsing/ParserExtensions.cs | 4 ++-- 9 files changed, 21 insertions(+), 21 deletions(-) diff --git a/samples/HostingPlayground/Program.cs b/samples/HostingPlayground/Program.cs index c1105bf04d..90c1096676 100644 --- a/samples/HostingPlayground/Program.cs +++ b/samples/HostingPlayground/Program.cs @@ -12,7 +12,7 @@ namespace HostingPlayground { class Program { - static async Task Main(string[] args) => await BuildCommandLine() + static Task Main(string[] args) => BuildCommandLine() .UseHost(_ => Host.CreateDefaultBuilder(), host => { diff --git a/src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_Directives_Suggest.cs b/src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_Directives_Suggest.cs index 0df2796867..14760434fa 100644 --- a/src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_Directives_Suggest.cs +++ b/src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_Directives_Suggest.cs @@ -46,8 +46,8 @@ public void Setup() public string TestCmdArgs; [Benchmark] - public async Task InvokeSuggest() - => await _testParser.InvokeAsync(TestCmdArgs, _nullConsole); + public Task InvokeSuggest() + => _testParser.InvokeAsync(TestCmdArgs, _nullConsole); } } diff --git a/src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_TypoCorrection.cs b/src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_TypoCorrection.cs index fe00c87af8..9549dc340f 100644 --- a/src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_TypoCorrection.cs +++ b/src/System.CommandLine.Benchmarks/CommandLine/Perf_Parser_TypoCorrection.cs @@ -53,7 +53,7 @@ public IEnumerable> GenerateTestParseResults() [Benchmark] [ArgumentsSource(nameof(GenerateTestParseResults))] - public async Task TypoCorrection(BdnParam parseResult) - => await parseResult.Value.InvokeAsync(_nullConsole); + public Task TypoCorrection(BdnParam parseResult) + => parseResult.Value.InvokeAsync(_nullConsole); } } diff --git a/src/System.CommandLine.Benchmarks/DragonFruit/Perf_CommandLine_EntryPoint.cs b/src/System.CommandLine.Benchmarks/DragonFruit/Perf_CommandLine_EntryPoint.cs index df53f13f54..87fe2309c1 100644 --- a/src/System.CommandLine.Benchmarks/DragonFruit/Perf_CommandLine_EntryPoint.cs +++ b/src/System.CommandLine.Benchmarks/DragonFruit/Perf_CommandLine_EntryPoint.cs @@ -129,8 +129,8 @@ public void Setup() } [Benchmark(Description = "ExecuteAssemblyAsync entry point search.")] - public async Task SearchForStartingPointUsingReflection() - => await System.CommandLine.DragonFruit.CommandLine.ExecuteAssemblyAsync( + public Task SearchForStartingPointUsingReflection() + => System.CommandLine.DragonFruit.CommandLine.ExecuteAssemblyAsync( _testAssembly, new string[] { }, null, @@ -138,8 +138,8 @@ public async Task SearchForStartingPointUsingReflection() _nullConsole); [Benchmark(Description = "ExecuteAssemblyAsync explicit entry point.")] - public async Task SearchForStartingPointWhenGivenEntryPointClass() - => await System.CommandLine.DragonFruit.CommandLine.ExecuteAssemblyAsync( + public Task SearchForStartingPointWhenGivenEntryPointClass() + => System.CommandLine.DragonFruit.CommandLine.ExecuteAssemblyAsync( _testAssembly, new string[] { }, "PerfTestApp.Program", diff --git a/src/System.CommandLine.Benchmarks/DragonFruit/Perf_CommandLine_Help.cs b/src/System.CommandLine.Benchmarks/DragonFruit/Perf_CommandLine_Help.cs index 0bf9d23e9e..0812ef2dc4 100644 --- a/src/System.CommandLine.Benchmarks/DragonFruit/Perf_CommandLine_Help.cs +++ b/src/System.CommandLine.Benchmarks/DragonFruit/Perf_CommandLine_Help.cs @@ -39,8 +39,8 @@ public void Setup() } [Benchmark(Description = "--help")] - public async Task SearchForStartingPointWhenGivenEntryPointClass_Help() - => await System.CommandLine.DragonFruit.CommandLine.ExecuteAssemblyAsync( + public Task SearchForStartingPointWhenGivenEntryPointClass_Help() + => System.CommandLine.DragonFruit.CommandLine.ExecuteAssemblyAsync( _testAssembly, new[] { "--help" }, null, diff --git a/src/System.CommandLine.DragonFruit/CommandLine.cs b/src/System.CommandLine.DragonFruit/CommandLine.cs index 40f8a55fc9..9c2a7bf9f7 100644 --- a/src/System.CommandLine.DragonFruit/CommandLine.cs +++ b/src/System.CommandLine.DragonFruit/CommandLine.cs @@ -28,7 +28,7 @@ public static class CommandLine /// Explicitly defined path to xml file containing XML Docs /// Output console /// The exit code. - public static async Task ExecuteAssemblyAsync( + public static Task ExecuteAssemblyAsync( Assembly entryAssembly, string[] args, string entryPointFullTypeName, @@ -46,7 +46,7 @@ public static async Task ExecuteAssemblyAsync( MethodInfo entryMethod = EntryPointDiscoverer.FindStaticEntryMethod(entryAssembly, entryPointFullTypeName); //TODO The xml docs file name and location can be customized using project property. - return await InvokeMethodAsync(args, entryMethod, xmlDocsFilePath, null, console); + return InvokeMethodAsync(args, entryMethod, xmlDocsFilePath, null, console); } /// @@ -79,7 +79,7 @@ public static int ExecuteAssembly( return InvokeMethod(args, entryMethod, xmlDocsFilePath, null, console); } - public static async Task InvokeMethodAsync( + public static Task InvokeMethodAsync( string[] args, MethodInfo method, string xmlDocsFilePath = null, @@ -88,7 +88,7 @@ public static async Task InvokeMethodAsync( { Parser parser = BuildParser(method, xmlDocsFilePath, target); - return await parser.InvokeAsync(args, console); + return parser.InvokeAsync(args, console); } public static int InvokeMethod( diff --git a/src/System.CommandLine/CommandExtensions.cs b/src/System.CommandLine/CommandExtensions.cs index 4773c10412..b0ecd0af40 100644 --- a/src/System.CommandLine/CommandExtensions.cs +++ b/src/System.CommandLine/CommandExtensions.cs @@ -53,7 +53,7 @@ public static int Invoke( /// The console to which output is written during invocation. /// A token that can be used to cancel the invocation. /// The exit code for the invocation. - public static async Task InvokeAsync( + public static Task InvokeAsync( this Command command, string[] args, IConsole? console = null, @@ -61,7 +61,7 @@ public static async Task InvokeAsync( { ParseResult parseResult = command.GetOrCreateDefaultInvocationParser().Parse(args); - return await InvocationPipeline.InvokeAsync(parseResult, console, cancellationToken); + return InvocationPipeline.InvokeAsync(parseResult, console, cancellationToken); } /// diff --git a/src/System.CommandLine/Parsing/ParseResultExtensions.cs b/src/System.CommandLine/Parsing/ParseResultExtensions.cs index f67256d0bf..89b7a67c02 100644 --- a/src/System.CommandLine/Parsing/ParseResultExtensions.cs +++ b/src/System.CommandLine/Parsing/ParseResultExtensions.cs @@ -24,11 +24,11 @@ public static class ParseResultExtensions /// A console to which output can be written. By default, is used. /// A token that can be used to cancel an invocation. /// A task whose result can be used as a process exit code. - public static async Task InvokeAsync( + public static Task InvokeAsync( this ParseResult parseResult, IConsole? console = null, CancellationToken cancellationToken = default) => - await InvocationPipeline.InvokeAsync(parseResult, console, cancellationToken); + InvocationPipeline.InvokeAsync(parseResult, console, cancellationToken); /// /// Invokes the appropriate command handler for a parsed command line input. diff --git a/src/System.CommandLine/Parsing/ParserExtensions.cs b/src/System.CommandLine/Parsing/ParserExtensions.cs index 4c90a1783b..85cf6c9d55 100644 --- a/src/System.CommandLine/Parsing/ParserExtensions.cs +++ b/src/System.CommandLine/Parsing/ParserExtensions.cs @@ -49,12 +49,12 @@ public static Task InvokeAsync( /// Parses a command line string array and invokes the handler for the indicated command. /// /// The exit code for the invocation. - public static async Task InvokeAsync( + public static Task InvokeAsync( this Parser parser, string[] args, IConsole? console = null, CancellationToken cancellationToken = default) => - await parser.Parse(args).InvokeAsync(console, cancellationToken); + parser.Parse(args).InvokeAsync(console, cancellationToken); /// /// Parses a command line string. From b25ebf79bd79b8aac655bb3db5a18dce38d0d04e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 6 Feb 2023 14:01:14 +0100 Subject: [PATCH 06/19] don't register services when creating BindingContext in InvocationContext.BindingContext getter, as BindingContext ctor already does that --- .../Invocation/InvocationContext.cs | 25 ++----------------- 1 file changed, 2 insertions(+), 23 deletions(-) diff --git a/src/System.CommandLine/Invocation/InvocationContext.cs b/src/System.CommandLine/Invocation/InvocationContext.cs index 950c21e35c..dd7a2af20d 100644 --- a/src/System.CommandLine/Invocation/InvocationContext.cs +++ b/src/System.CommandLine/Invocation/InvocationContext.cs @@ -44,35 +44,14 @@ public InvocationContext( /// /// The binding context for the current invocation. /// - public BindingContext BindingContext - { - get - { - if (_bindingContext is null) - { - _bindingContext = new BindingContext(this); - _bindingContext.ServiceProvider.AddService(_ => GetCancellationToken()); - _bindingContext.ServiceProvider.AddService(_ => this); - } - - return _bindingContext; - } - } + public BindingContext BindingContext => _bindingContext ??= new BindingContext(this); /// /// The console to which output should be written during the current invocation. /// public IConsole Console { - get - { - if (_console is null) - { - _console = new SystemConsole(); - } - - return _console; - } + get =>_console ??= new SystemConsole(); set => _console = value; } From d3e6068e9ab333141397e3b3f2f2d74ec816bc6e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 6 Feb 2023 14:25:46 +0100 Subject: [PATCH 07/19] don't register CancellationToken twice in ServiceProvider --- src/System.CommandLine/Binding/BindingContext.cs | 6 +++--- src/System.CommandLine/Invocation/InvocationContext.cs | 2 +- src/System.CommandLine/Invocation/ServiceProvider.cs | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/System.CommandLine/Binding/BindingContext.cs b/src/System.CommandLine/Binding/BindingContext.cs index 435decc4f2..ccd9b2db5f 100644 --- a/src/System.CommandLine/Binding/BindingContext.cs +++ b/src/System.CommandLine/Binding/BindingContext.cs @@ -6,6 +6,7 @@ using System.CommandLine.Parsing; using System.Diagnostics.CodeAnalysis; using System.Linq; +using System.Threading; namespace System.CommandLine.Binding { @@ -16,12 +17,11 @@ public sealed class BindingContext : IServiceProvider { private HelpBuilder? _helpBuilder; - internal BindingContext(InvocationContext invocationContext) + internal BindingContext(InvocationContext invocationContext, CancellationToken cancellationToken) { InvocationContext = invocationContext; - ServiceProvider = new ServiceProvider(this); + ServiceProvider = new ServiceProvider(this, cancellationToken); ServiceProvider.AddService(_ => InvocationContext); - ServiceProvider.AddService(_ => InvocationContext.GetCancellationToken()); } internal InvocationContext InvocationContext { get; } diff --git a/src/System.CommandLine/Invocation/InvocationContext.cs b/src/System.CommandLine/Invocation/InvocationContext.cs index dd7a2af20d..29cab558f3 100644 --- a/src/System.CommandLine/Invocation/InvocationContext.cs +++ b/src/System.CommandLine/Invocation/InvocationContext.cs @@ -44,7 +44,7 @@ public InvocationContext( /// /// The binding context for the current invocation. /// - public BindingContext BindingContext => _bindingContext ??= new BindingContext(this); + public BindingContext BindingContext => _bindingContext ??= new BindingContext(this, _token); /// /// The console to which output should be written during the current invocation. diff --git a/src/System.CommandLine/Invocation/ServiceProvider.cs b/src/System.CommandLine/Invocation/ServiceProvider.cs index 852a1c9913..6f55479fc1 100644 --- a/src/System.CommandLine/Invocation/ServiceProvider.cs +++ b/src/System.CommandLine/Invocation/ServiceProvider.cs @@ -8,17 +8,17 @@ namespace System.CommandLine.Invocation { - internal class ServiceProvider : IServiceProvider + internal sealed class ServiceProvider : IServiceProvider { private readonly Dictionary> _services; - public ServiceProvider(BindingContext bindingContext) + internal ServiceProvider(BindingContext bindingContext, CancellationToken cancellationToken) { _services = new Dictionary> { [typeof(ParseResult)] = _ => bindingContext.ParseResult, [typeof(IConsole)] = _ => bindingContext.Console, - [typeof(CancellationToken)] = _ => CancellationToken.None, + [typeof(CancellationToken)] = _ => cancellationToken, [typeof(HelpBuilder)] = _ => bindingContext.ParseResult.Parser.Configuration.HelpBuilderFactory(bindingContext), [typeof(BindingContext)] = _ => bindingContext }; From c24e3c51ffa737347bf7648e5c33c9dbe968c2d5 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 6 Feb 2023 14:30:43 +0100 Subject: [PATCH 08/19] don't use BindingContext when there is no need to (it's expensive) --- src/System.CommandLine/Help/HelpOption.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/System.CommandLine/Help/HelpOption.cs b/src/System.CommandLine/Help/HelpOption.cs index 654afd5647..0fea4b51c9 100644 --- a/src/System.CommandLine/Help/HelpOption.cs +++ b/src/System.CommandLine/Help/HelpOption.cs @@ -44,14 +44,12 @@ internal static void Handler(InvocationContext context) { var output = context.Console.Out.CreateTextWriter(); - var helpContext = new HelpContext(context.BindingContext.HelpBuilder, + var helpContext = new HelpContext(context.HelpBuilder, context.ParseResult.CommandResult.Command, output, context.ParseResult); - context.BindingContext - .HelpBuilder - .Write(helpContext); + context.HelpBuilder.Write(helpContext); } } } \ No newline at end of file From 91ac000f32105ce29a3e3478c2ad03ca9f3c3eed Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 6 Feb 2023 15:54:03 +0100 Subject: [PATCH 09/19] don't register CancellationToken inside ServiceProvider, pass the token in explicit way to Handler --- ...ne_Hosting_api_is_not_changed.approved.txt | 2 +- ...tionBinder_api_is_not_changed.approved.txt | 2 +- ...ommandLine_api_is_not_changed.approved.txt | 28 ++++---- .../CommandHandlerSourceGenerator.cs | 4 +- .../HostingHandlerTest.cs | 9 +-- .../InvocationLifetime.cs | 9 +-- ...indingCommandHandlerTests.BindingByName.cs | 12 ++-- .../ModelBindingCommandHandlerTests.cs | 11 ++-- .../ParameterBindingTests.cs | 9 +-- .../ModelBindingCommandHandler.cs | 6 +- .../TerminalModeTests.cs | 2 +- .../SuggestionDispatcher.cs | 5 +- .../Binding/SetHandlerTests.cs | 35 +++++----- .../CancelOnProcessTerminationTests.cs | 12 +--- .../Invocation/InvocationContextTests.cs | 64 ------------------- .../Invocation/InvocationExtensionsTests.cs | 11 ++-- .../Invocation/InvocationPipelineTests.cs | 36 +++++------ .../UseExceptionHandlerTests.cs | 4 +- .../Binding/BindingContext.cs | 5 +- src/System.CommandLine/Handler.Func.cs | 55 ++++++++-------- src/System.CommandLine/ICommandHandler.cs | 4 +- .../Invocation/AnonymousCommandHandler.cs | 18 +++--- .../Invocation/InvocationContext.cs | 52 +-------------- .../Invocation/InvocationPipeline.cs | 21 +++--- .../Invocation/ServiceProvider.cs | 3 +- 25 files changed, 152 insertions(+), 267 deletions(-) delete mode 100644 src/System.CommandLine.Tests/Invocation/InvocationContextTests.cs diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_Hosting_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_Hosting_api_is_not_changed.approved.txt index 2cbc0bafdb..fb206743ff 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_Hosting_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_Hosting_api_is_not_changed.approved.txt @@ -12,7 +12,7 @@ System.CommandLine.Hosting public static System.CommandLine.CommandLineBuilder UseHost(this System.CommandLine.CommandLineBuilder builder, System.Func hostBuilderFactory, System.Action configureHost = null) public static Microsoft.Extensions.Hosting.IHostBuilder UseInvocationLifetime(this Microsoft.Extensions.Hosting.IHostBuilder host, System.CommandLine.Invocation.InvocationContext invocation, System.Action configureOptions = null) public class InvocationLifetime, Microsoft.Extensions.Hosting.IHostLifetime - .ctor(Microsoft.Extensions.Options.IOptions options, Microsoft.Extensions.Hosting.IHostEnvironment environment, Microsoft.Extensions.Hosting.IHostApplicationLifetime applicationLifetime, System.CommandLine.Invocation.InvocationContext context = null, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory = null) + .ctor(Microsoft.Extensions.Options.IOptions options, Microsoft.Extensions.Hosting.IHostEnvironment environment, Microsoft.Extensions.Hosting.IHostApplicationLifetime applicationLifetime, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory = null) public Microsoft.Extensions.Hosting.IHostApplicationLifetime ApplicationLifetime { get; } public Microsoft.Extensions.Hosting.IHostEnvironment Environment { get; } public InvocationLifetimeOptions Options { get; } diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_NamingConventionBinder_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_NamingConventionBinder_api_is_not_changed.approved.txt index e8a6a52be0..993330ea89 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_NamingConventionBinder_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_NamingConventionBinder_api_is_not_changed.approved.txt @@ -99,7 +99,7 @@ public System.Void BindParameter(System.Reflection.ParameterInfo param, System.CommandLine.Argument argument) public System.Void BindParameter(System.Reflection.ParameterInfo param, System.CommandLine.Option option) public System.Int32 Invoke(System.CommandLine.Invocation.InvocationContext context) - public System.Threading.Tasks.Task InvokeAsync(System.CommandLine.Invocation.InvocationContext context) + public System.Threading.Tasks.Task InvokeAsync(System.CommandLine.Invocation.InvocationContext context, System.Threading.CancellationToken cancellationToken) public class ModelDescriptor public static ModelDescriptor FromType() public static ModelDescriptor FromType(System.Type type) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 26cf6ad038..e22a11119f 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -116,27 +116,27 @@ System.CommandLine public static class Handler public static System.Void SetHandler(this Command command, System.Action handle) public static System.Void SetHandler(this Command command, System.Action handle) - public static System.Void SetHandler(this Command command, System.Func handle) - public static System.Void SetHandler(this Command command, System.Func handle) + public static System.Void SetHandler(this Command command, System.Func handle) + public static System.Void SetHandler(this Command command, System.Func handle) public static System.Void SetHandler(this Command command, Action handle, IValueDescriptor symbol) - public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol) + public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol) public static System.Void SetHandler(this Command command, Action handle, IValueDescriptor symbol1, IValueDescriptor symbol2) - public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2) + public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2) public static System.Void SetHandler(this Command command, Action handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3) - public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3) + public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3) public static System.Void SetHandler(this Command command, Action handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4) - public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4) + public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4) public static System.Void SetHandler(this Command command, Action handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5) - public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5) + public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5) public static System.Void SetHandler(this Command command, Action handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5, IValueDescriptor symbol6) - public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5, IValueDescriptor symbol6) + public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5, IValueDescriptor symbol6) public static System.Void SetHandler(this Command command, Action handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5, IValueDescriptor symbol6, IValueDescriptor symbol7) - public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5, IValueDescriptor symbol6, IValueDescriptor symbol7) + public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5, IValueDescriptor symbol6, IValueDescriptor symbol7) public static System.Void SetHandler(this Command command, Action handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5, IValueDescriptor symbol6, IValueDescriptor symbol7, IValueDescriptor symbol8) - public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5, IValueDescriptor symbol6, IValueDescriptor symbol7, IValueDescriptor symbol8) + public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5, IValueDescriptor symbol6, IValueDescriptor symbol7, IValueDescriptor symbol8) public interface ICommandHandler public System.Int32 Invoke(System.CommandLine.Invocation.InvocationContext context) - public System.Threading.Tasks.Task InvokeAsync(System.CommandLine.Invocation.InvocationContext context) + public System.Threading.Tasks.Task InvokeAsync(System.CommandLine.Invocation.InvocationContext context, System.Threading.CancellationToken cancellationToken) public interface IConsole : System.CommandLine.IO.IStandardError, System.CommandLine.IO.IStandardIn, System.CommandLine.IO.IStandardOut public abstract class IdentifierSymbol : Symbol public System.Collections.Generic.IReadOnlyCollection Aliases { get; } @@ -324,8 +324,8 @@ System.CommandLine.Help public System.Boolean Equals(TwoColumnHelpRow other) public System.Int32 GetHashCode() System.CommandLine.Invocation - public class InvocationContext, System.IDisposable - .ctor(System.CommandLine.ParseResult parseResult, System.CommandLine.IConsole console = null, System.Threading.CancellationToken cancellationToken = null) + public class InvocationContext + .ctor(System.CommandLine.ParseResult parseResult, System.CommandLine.IConsole console = null) public System.CommandLine.Binding.BindingContext BindingContext { get; } public System.CommandLine.IConsole Console { get; set; } public System.Int32 ExitCode { get; set; } @@ -334,12 +334,10 @@ System.CommandLine.Invocation public System.CommandLine.LocalizationResources LocalizationResources { get; } public System.CommandLine.Parsing.Parser Parser { get; } public System.CommandLine.ParseResult ParseResult { get; set; } - public System.Threading.CancellationToken GetCancellationToken() public System.Object GetValue(System.CommandLine.Option option) public T GetValue(Option option) public System.Object GetValue(System.CommandLine.Argument argument) public T GetValue(Argument argument) - public System.Void LinkToken(System.Threading.CancellationToken token) public delegate InvocationMiddleware : System.MulticastDelegate, System.ICloneable, System.Runtime.Serialization.ISerializable .ctor(System.Object object, System.IntPtr method) public System.IAsyncResult BeginInvoke(InvocationContext context, System.Func next, System.AsyncCallback callback, System.Object object) diff --git a/src/System.CommandLine.Generator/CommandHandlerSourceGenerator.cs b/src/System.CommandLine.Generator/CommandHandlerSourceGenerator.cs index 42ad13ea03..d4ffd1d016 100644 --- a/src/System.CommandLine.Generator/CommandHandlerSourceGenerator.cs +++ b/src/System.CommandLine.Generator/CommandHandlerSourceGenerator.cs @@ -115,10 +115,10 @@ private class GeneratedHandler_{handlerCount} : {ICommandHandlerType} } builder.Append($@" - public int Invoke(global::System.CommandLine.Invocation.InvocationContext context) => InvokeAsync(context).GetAwaiter().GetResult();"); + public int Invoke(global::System.CommandLine.Invocation.InvocationContext context) => InvokeAsync(context, global::System.Threading.CancellationToken.None).GetAwaiter().GetResult();"); builder.Append($@" - public async global::System.Threading.Tasks.Task InvokeAsync(global::System.CommandLine.Invocation.InvocationContext context) + public async global::System.Threading.Tasks.Task InvokeAsync(global::System.CommandLine.Invocation.InvocationContext context, global::System.Threading.CancellationToken cancellationToken) {{"); builder.Append($@" {invocation.InvokeContents()}"); diff --git a/src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs b/src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs index efe928533c..72a4cb4995 100644 --- a/src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs +++ b/src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs @@ -1,6 +1,7 @@ using System.CommandLine; using System.CommandLine.Invocation; using System.CommandLine.Parsing; +using System.Threading; using System.Threading.Tasks; using FluentAssertions; using Microsoft.Extensions.DependencyInjection; @@ -143,7 +144,7 @@ public int Invoke(InvocationContext context) return Act(); } - public Task InvokeAsync(InvocationContext context) + public Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken) { return Task.FromResult(Act()); } @@ -176,7 +177,7 @@ public int Invoke(InvocationContext context) return IntOption; } - public Task InvokeAsync(InvocationContext context) + public Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken) { service.Value = IntOption; return Task.FromResult(IntOption); @@ -222,9 +223,9 @@ public MyHandler(MyService service) public string One { get; set; } - public int Invoke(InvocationContext context) => InvokeAsync(context).GetAwaiter().GetResult(); + public int Invoke(InvocationContext context) => InvokeAsync(context, CancellationToken.None).GetAwaiter().GetResult(); - public Task InvokeAsync(InvocationContext context) + public Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken) { service.Value = IntOption; service.StringValue = One; diff --git a/src/System.CommandLine.Hosting/InvocationLifetime.cs b/src/System.CommandLine.Hosting/InvocationLifetime.cs index 80798191d7..4201436594 100644 --- a/src/System.CommandLine.Hosting/InvocationLifetime.cs +++ b/src/System.CommandLine.Hosting/InvocationLifetime.cs @@ -19,7 +19,6 @@ namespace System.CommandLine.Hosting { public class InvocationLifetime : IHostLifetime { - private readonly CancellationToken invokeCancelToken; private CancellationTokenRegistration invokeCancelReg; private CancellationTokenRegistration appStartedReg; private CancellationTokenRegistration appStoppingReg; @@ -28,7 +27,6 @@ public InvocationLifetime( IOptions options, IHostEnvironment environment, IHostApplicationLifetime applicationLifetime, - InvocationContext context = null, ILoggerFactory loggerFactory = null) { Options = options?.Value ?? throw new ArgumentNullException(nameof(options)); @@ -37,11 +35,6 @@ public InvocationLifetime( ApplicationLifetime = applicationLifetime ?? throw new ArgumentNullException(nameof(applicationLifetime)); - // if InvocationLifetime is added outside of a System.CommandLine - // invocation pipeline context will be null. - // Use default cancellation token instead, and become a noop lifetime. - invokeCancelToken = context?.GetCancellationToken() ?? default; - Logger = (loggerFactory ?? NullLoggerFactory.Instance) .CreateLogger("Microsoft.Hosting.Lifetime"); } @@ -65,7 +58,7 @@ public Task WaitForStartAsync(CancellationToken cancellationToken) }, this); } - invokeCancelReg = invokeCancelToken.Register(state => + invokeCancelReg = cancellationToken.Register(state => { ((InvocationLifetime)state).OnInvocationCancelled(); }, this); diff --git a/src/System.CommandLine.NamingConventionBinder.Tests/ModelBindingCommandHandlerTests.BindingByName.cs b/src/System.CommandLine.NamingConventionBinder.Tests/ModelBindingCommandHandlerTests.BindingByName.cs index e3fd786c39..2c5625d20b 100644 --- a/src/System.CommandLine.NamingConventionBinder.Tests/ModelBindingCommandHandlerTests.BindingByName.cs +++ b/src/System.CommandLine.NamingConventionBinder.Tests/ModelBindingCommandHandlerTests.BindingByName.cs @@ -6,6 +6,7 @@ using System.CommandLine.Tests.Binding; using System.CommandLine.Utility; using System.IO; +using System.Threading; using System.Threading.Tasks; using FluentAssertions; using Xunit; @@ -41,7 +42,8 @@ public async Task Option_arguments_are_bound_by_name_to_method_parameters( var console = new TestConsole(); await handler.InvokeAsync( - new InvocationContext(command.Parse(commandLine), console)); + new InvocationContext(command.Parse(commandLine), console), + CancellationToken.None); console.Out.ToString().Should().Be(expectedValue.ToString()); } @@ -73,7 +75,8 @@ public async Task Option_arguments_are_bound_by_name_to_the_properties_of_method var console = new TestConsole(); await handler.InvokeAsync( - new InvocationContext(command.Parse(commandLine), console)); + new InvocationContext(command.Parse(commandLine), console), + CancellationToken.None); console.Out.ToString().Should().Be($"ClassWithSetter<{type.Name}>: {expectedValue}"); } @@ -105,7 +108,8 @@ public async Task Option_arguments_are_bound_by_name_to_the_constructor_paramete var console = new TestConsole(); await handler.InvokeAsync( - new InvocationContext(command.Parse(commandLine), console)); + new InvocationContext(command.Parse(commandLine), console), + CancellationToken.None); console.Out.ToString().Should().Be($"ClassWithCtorParameter<{type.Name}>: {expectedValue}"); } @@ -133,7 +137,7 @@ public async Task Command_arguments_are_bound_by_name_to_handler_method_paramete var console = new TestConsole(); await handler.InvokeAsync( - new InvocationContext(command.Parse(commandLine), console)); + new InvocationContext(command.Parse(commandLine), console), CancellationToken.None); console.Out.ToString().Should().Be(expectedValue.ToString()); } diff --git a/src/System.CommandLine.NamingConventionBinder.Tests/ModelBindingCommandHandlerTests.cs b/src/System.CommandLine.NamingConventionBinder.Tests/ModelBindingCommandHandlerTests.cs index c50e566f65..14d8748525 100644 --- a/src/System.CommandLine.NamingConventionBinder.Tests/ModelBindingCommandHandlerTests.cs +++ b/src/System.CommandLine.NamingConventionBinder.Tests/ModelBindingCommandHandlerTests.cs @@ -9,6 +9,7 @@ using System.IO; using System.Linq; using System.Reflection; +using System.Threading; using System.Threading.Tasks; using FluentAssertions; using FluentAssertions.Execution; @@ -44,7 +45,7 @@ public async Task Unspecified_option_arguments_with_no_default_value_are_bound_t var invocationContext = new InvocationContext(parseResult); - await handler.InvokeAsync(invocationContext); + await handler.InvokeAsync(invocationContext, CancellationToken.None); BoundValueCapturer.GetBoundValue(invocationContext).Should().Be(expectedValue); } @@ -121,7 +122,7 @@ public async Task Handler_method_receives_option_arguments_bound_to_the_specifie var invocationContext = new InvocationContext(parseResult); - await handler.InvokeAsync(invocationContext); + await handler.InvokeAsync(invocationContext, CancellationToken.None); var boundValue = BoundValueCapturer.GetBoundValue(invocationContext); @@ -184,7 +185,7 @@ public async Task Handler_method_receives_command_arguments_bound_to_the_specifi var invocationContext = new InvocationContext(parseResult); - await handler.InvokeAsync(invocationContext); + await handler.InvokeAsync(invocationContext, CancellationToken.None); var boundValue = BoundValueCapturer.GetBoundValue(invocationContext); @@ -236,7 +237,7 @@ public async Task Handler_method_receives_command_arguments_explicitly_bound_to_ var invocationContext = new InvocationContext(parseResult); - await handler.InvokeAsync(invocationContext); + await handler.InvokeAsync(invocationContext, CancellationToken.None); var boundValue = BoundValueCapturer.GetBoundValue(invocationContext); @@ -289,7 +290,7 @@ public async Task Handler_method_receive_option_arguments_explicitly_bound_to_th var invocationContext = new InvocationContext(parseResult); - await handler.InvokeAsync(invocationContext); + await handler.InvokeAsync(invocationContext, CancellationToken.None); var boundValue = BoundValueCapturer.GetBoundValue(invocationContext); diff --git a/src/System.CommandLine.NamingConventionBinder.Tests/ParameterBindingTests.cs b/src/System.CommandLine.NamingConventionBinder.Tests/ParameterBindingTests.cs index ae4e8bca79..fd104c8e0b 100644 --- a/src/System.CommandLine.NamingConventionBinder.Tests/ParameterBindingTests.cs +++ b/src/System.CommandLine.NamingConventionBinder.Tests/ParameterBindingTests.cs @@ -7,6 +7,7 @@ using System.CommandLine.IO; using System.CommandLine.Parsing; using System.IO; +using System.Threading; using System.Threading.Tasks; using FluentAssertions; using Xunit; @@ -447,9 +448,9 @@ public abstract class AbstractTestCommandHandler : ICommandHandler { public abstract Task DoJobAsync(); - public int Invoke(InvocationContext context) => InvokeAsync(context).GetAwaiter().GetResult(); + public int Invoke(InvocationContext context) => InvokeAsync(context, CancellationToken.None).GetAwaiter().GetResult(); - public Task InvokeAsync(InvocationContext context) + public Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken) => DoJobAsync(); } @@ -463,13 +464,13 @@ public class VirtualTestCommandHandler : ICommandHandler { public int Invoke(InvocationContext context) => 42; - public virtual Task InvokeAsync(InvocationContext context) + public virtual Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken) => Task.FromResult(42); } public class OverridenVirtualTestCommandHandler : VirtualTestCommandHandler { - public override Task InvokeAsync(InvocationContext context) + public override Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken) => Task.FromResult(41); } } \ No newline at end of file diff --git a/src/System.CommandLine.NamingConventionBinder/ModelBindingCommandHandler.cs b/src/System.CommandLine.NamingConventionBinder/ModelBindingCommandHandler.cs index 451cb1591a..4628653da0 100644 --- a/src/System.CommandLine.NamingConventionBinder/ModelBindingCommandHandler.cs +++ b/src/System.CommandLine.NamingConventionBinder/ModelBindingCommandHandler.cs @@ -6,6 +6,7 @@ using System.CommandLine.Invocation; using System.Linq; using System.Reflection; +using System.Threading; using System.Threading.Tasks; namespace System.CommandLine.NamingConventionBinder; @@ -54,8 +55,9 @@ internal ModelBindingCommandHandler( /// Binds values for the underlying user-defined method and uses them to invoke that method. /// /// The current invocation context. + /// A token that can be used to cancel the invocation. /// A task whose value can be used to set the process exit code. - public async Task InvokeAsync(InvocationContext context) + public async Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken) { var bindingContext = context.BindingContext; @@ -130,5 +132,5 @@ private void BindValueSource(ParameterInfo param, IValueSource valueSource) x.ValueType == param.ParameterType); /// - public int Invoke(InvocationContext context) => InvokeAsync(context).GetAwaiter().GetResult(); + public int Invoke(InvocationContext context) => InvokeAsync(context, CancellationToken.None).GetAwaiter().GetResult(); } \ No newline at end of file diff --git a/src/System.CommandLine.Rendering.Tests/TerminalModeTests.cs b/src/System.CommandLine.Rendering.Tests/TerminalModeTests.cs index bf6caf42cb..6f068f63a9 100644 --- a/src/System.CommandLine.Rendering.Tests/TerminalModeTests.cs +++ b/src/System.CommandLine.Rendering.Tests/TerminalModeTests.cs @@ -42,7 +42,7 @@ public async Task Sets_output_mode_to_Ansi_when_specified_by_output_directive(Ou OutputMode detectedOutputMode = OutputMode.Auto; var command = new Command("hello"); - command.SetHandler(ctx => + command.SetHandler((ctx, cancellationToken) => { detectedOutputMode = ctx.Console.DetectOutputMode(); return Task.FromResult(0); diff --git a/src/System.CommandLine.Suggest/SuggestionDispatcher.cs b/src/System.CommandLine.Suggest/SuggestionDispatcher.cs index 6c36afb9e8..4f2e6a7df1 100644 --- a/src/System.CommandLine.Suggest/SuggestionDispatcher.cs +++ b/src/System.CommandLine.Suggest/SuggestionDispatcher.cs @@ -7,6 +7,7 @@ using System.CommandLine.Parsing; using System.IO; using System.Linq; +using System.Threading; using System.Threading.Tasks; namespace System.CommandLine.Suggest @@ -40,7 +41,7 @@ public SuggestionDispatcher(ISuggestionRegistration suggestionRegistration, ISug { Description = "Lists apps registered for suggestions", }; - ListCommand.SetHandler(ctx => + ListCommand.SetHandler((ctx, cancellationToken) => { ctx.Console.Out.WriteLine(ShellPrefixesToMatch(_suggestionRegistration)); return Task.FromResult(0); @@ -61,7 +62,7 @@ public SuggestionDispatcher(ISuggestionRegistration suggestionRegistration, ISug new Option("--suggestion-command", "The command to invoke to retrieve suggestions") }; - RegisterCommand.SetHandler(context => + RegisterCommand.SetHandler((context, cancellationToken) => { Register(context.ParseResult.GetValue(commandPathOption), context.Console); return Task.FromResult(0); diff --git a/src/System.CommandLine.Tests/Binding/SetHandlerTests.cs b/src/System.CommandLine.Tests/Binding/SetHandlerTests.cs index 2a546b900d..29ff9ceb3c 100644 --- a/src/System.CommandLine.Tests/Binding/SetHandlerTests.cs +++ b/src/System.CommandLine.Tests/Binding/SetHandlerTests.cs @@ -5,6 +5,7 @@ using System.CommandLine.Binding; using System.CommandLine.IO; using System.Linq; +using System.Threading; using System.Threading.Tasks; using FluentAssertions; using Xunit; @@ -184,29 +185,29 @@ public void Binding_is_correct_for_Func_overload_having_arity_(int arity) var receivedValues = new List(); Delegate handlerFunc = arity switch { - 1 => new Func( - i1 => + 1 => new Func( + (i1, cancellationToken) => Received(i1)), - 2 => new Func( - (i1, i2) => + 2 => new Func( + (i1, i2, cancellationToken) => Received(i1, i2)), - 3 => new Func( - (i1, i2, i3) => + 3 => new Func( + (i1, i2, i3, cancellationToken) => Received(i1, i2, i3)), - 4 => new Func( - (i1, i2, i3, i4) => + 4 => new Func( + (i1, i2, i3, i4, cancellationToken) => Received(i1, i2, i3, i4)), - 5 => new Func( - (i1, i2, i3, i4, i5) => + 5 => new Func( + (i1, i2, i3, i4, i5, cancellationToken) => Received(i1, i2, i3, i4, i5)), - 6 => new Func( - (i1, i2, i3, i4, i5, i6) => + 6 => new Func( + (i1, i2, i3, i4, i5, i6, cancellationToken) => Received(i1, i2, i3, i4, i5, i6)), - 7 => new Func( - (i1, i2, i3, i4, i5, i6, i7) => + 7 => new Func( + (i1, i2, i3, i4, i5, i6, i7, cancellationToken) => Received(i1, i2, i3, i4, i5, i6, i7)), - 8 => new Func( - (i1, i2, i3, i4, i5, i6, i7, i8) => + 8 => new Func( + (i1, i2, i3, i4, i5, i6, i7, i8, cancellationToken) => Received(i1, i2, i3, i4, i5, i6, i7, i8)), _ => throw new ArgumentOutOfRangeException() @@ -257,7 +258,7 @@ public async Task Unexpected_return_types_result_in_exit_code_0_if_no_exception_ var command = new Command("wat"); - var handle = () => + var handle = (CancellationToken cancellationToken) => { wasCalled = true; return Task.FromResult(new { NovelType = true }); diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index aaae8d2b2b..f38c200dcf 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -30,10 +30,8 @@ public async Task CancelOnProcessTermination_provides_CancellationToken_that_sig { var command = new Command("the-command"); - command.SetHandler(async context => + command.SetHandler(async (context, cancellationToken) => { - var cancellationToken = context.GetCancellationToken(); - try { context.Console.WriteLine(ChildProcessWaiting); @@ -102,10 +100,8 @@ public async Task CancelOnProcessTermination_provides_CancellationToken_that_sig { var command = new Command("the-command"); - command.SetHandler(async context => + command.SetHandler(async (context, cancellationToken) => { - var cancellationToken = context.GetCancellationToken(); - try { context.Console.WriteLine(ChildProcessWaiting); @@ -182,10 +178,8 @@ public async Task CancelOnProcessTermination_provides_CancellationToken_that_sig { var command = new Command("the-command"); - command.SetHandler(async context => + command.SetHandler(async (context, cancellationToken) => { - var cancellationToken = context.GetCancellationToken(); - try { context.Console.WriteLine(ChildProcessWaiting); diff --git a/src/System.CommandLine.Tests/Invocation/InvocationContextTests.cs b/src/System.CommandLine.Tests/Invocation/InvocationContextTests.cs deleted file mode 100644 index 5e168e7191..0000000000 --- a/src/System.CommandLine.Tests/Invocation/InvocationContextTests.cs +++ /dev/null @@ -1,64 +0,0 @@ -using FluentAssertions; -using System.CommandLine; -using System.CommandLine.Invocation; -using System.CommandLine.Parsing; -using System.Threading; -using Xunit; - -namespace System.CommandLine.Tests.Invocation -{ - public class InvocationContextTests - { - [Fact] - public void InvocationContext_with_cancellation_token_returns_it() - { - using CancellationTokenSource cts = new(); - var parseResult = new CommandLineBuilder(new RootCommand()) - .Build() - .Parse(""); - using InvocationContext context = new(parseResult, cancellationToken: cts.Token); - - var token = context.GetCancellationToken(); - - token.IsCancellationRequested.Should().BeFalse(); - cts.Cancel(); - token.IsCancellationRequested.Should().BeTrue(); - } - - [Fact] - public void InvocationContext_with_linked_cancellation_token_can_cancel_by_passed_token() - { - using CancellationTokenSource cts1 = new(); - using CancellationTokenSource cts2 = new(); - var parseResult = new CommandLineBuilder(new RootCommand()) - .Build() - .Parse(""); - using InvocationContext context = new(parseResult, cancellationToken: cts1.Token); - context.LinkToken(cts2.Token); - - var token = context.GetCancellationToken(); - - token.IsCancellationRequested.Should().BeFalse(); - cts1.Cancel(); - token.IsCancellationRequested.Should().BeTrue(); - } - - [Fact] - public void InvocationContext_with_linked_cancellation_token_can_cancel_by_linked_token() - { - using CancellationTokenSource cts1 = new(); - using CancellationTokenSource cts2 = new(); - var parseResult = new CommandLineBuilder(new RootCommand()) - .Build() - .Parse(""); - using InvocationContext context = new(parseResult, cancellationToken: cts1.Token); - context.LinkToken(cts2.Token); - - var token = context.GetCancellationToken(); - - token.IsCancellationRequested.Should().BeFalse(); - cts2.Cancel(); - token.IsCancellationRequested.Should().BeTrue(); - } - } -} diff --git a/src/System.CommandLine.Tests/Invocation/InvocationExtensionsTests.cs b/src/System.CommandLine.Tests/Invocation/InvocationExtensionsTests.cs index 4f6d2e7361..9308314cc1 100644 --- a/src/System.CommandLine.Tests/Invocation/InvocationExtensionsTests.cs +++ b/src/System.CommandLine.Tests/Invocation/InvocationExtensionsTests.cs @@ -80,7 +80,7 @@ public async Task RootCommand_InvokeAsync_returns_1_when_handler_throws() var wasCalled = false; var rootCommand = new RootCommand(); - rootCommand.SetHandler(() => + rootCommand.SetHandler(_ => { wasCalled = true; throw new Exception("oops!"); @@ -103,7 +103,7 @@ public void RootCommand_Invoke_returns_1_when_handler_throws() var wasCalled = false; var rootCommand = new RootCommand(); - rootCommand.SetHandler(() => + rootCommand.SetHandler(_ => { wasCalled = true; throw new Exception("oops!"); @@ -125,7 +125,7 @@ public async Task RootCommand_InvokeAsync_can_set_custom_result_code() { var rootCommand = new RootCommand(); - rootCommand.SetHandler(context => + rootCommand.SetHandler((context, cancellationToken) => { context.ExitCode = 123; return Task.CompletedTask; @@ -141,7 +141,7 @@ public void RootCommand_Invoke_can_set_custom_result_code() { var rootCommand = new RootCommand(); - rootCommand.SetHandler(context => + rootCommand.SetHandler((context, cancellationToken) => { context.ExitCode = 123; return Task.CompletedTask; @@ -157,9 +157,8 @@ public async Task Command_InvokeAsync_with_cancelation_token_invokes_command_han { CancellationTokenSource cts = new(); var command = new Command("test"); - command.SetHandler((InvocationContext context) => + command.SetHandler((InvocationContext context, CancellationToken cancellationToken) => { - CancellationToken cancellationToken = context.GetCancellationToken(); Assert.True(cancellationToken.CanBeCanceled); if (cancellationToken.IsCancellationRequested) { diff --git a/src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs b/src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs index d67eda24d4..157b69ae1d 100644 --- a/src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs +++ b/src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs @@ -120,7 +120,7 @@ public void When_middleware_throws_then_Invoke_does_not_handle_the_exception() public void When_command_handler_throws_then_InvokeAsync_does_not_handle_the_exception() { var command = new Command("the-command"); - command.SetHandler(() => + command.SetHandler(_ => { throw new Exception("oops!"); // Help the compiler pick a CommandHandler.Create overload. @@ -149,7 +149,7 @@ public void When_command_handler_throws_then_InvokeAsync_does_not_handle_the_exc public void When_command_handler_throws_then_Invoke_does_not_handle_the_exception() { var command = new Command("the-command"); - command.SetHandler(() => + command.SetHandler(_ => { throw new Exception("oops!"); // Help the compiler pick a CommandHandler.Create overload. @@ -181,7 +181,7 @@ public async Task ParseResult_can_be_replaced_by_middleware() var command = new Command("the-command"); var implicitInnerCommand = new Command("implicit-inner-command"); command.Subcommands.Add(implicitInnerCommand); - implicitInnerCommand.SetHandler(context => + implicitInnerCommand.SetHandler((context, cancellationToken) => { wasCalled = true; context.ParseResult.Errors.Should().BeEmpty(); @@ -217,7 +217,7 @@ public async Task Invocation_can_be_short_circuited_by_middleware_by_not_calling var handlerWasCalled = false; var command = new Command("the-command"); - command.SetHandler(context => + command.SetHandler((context, cancellationToken) => { handlerWasCalled = true; context.ParseResult.Errors.Should().BeEmpty(); @@ -248,7 +248,7 @@ public void Synchronous_invocation_can_be_short_circuited_by_async_middleware_by var handlerWasCalled = false; var command = new Command("the-command"); - command.SetHandler(context => + command.SetHandler((context, cancellationToken) => { handlerWasCalled = true; context.ParseResult.Errors.Should().BeEmpty(); @@ -278,7 +278,7 @@ public async Task When_no_help_builder_is_specified_it_uses_default_implementati bool handlerWasCalled = false; var command = new Command("help-command"); - command.SetHandler(context => + command.SetHandler((context, cancellationToken) => { handlerWasCalled = true; context.HelpBuilder.Should().NotBeNull(); @@ -305,7 +305,7 @@ public async Task When_help_builder_factory_is_specified_it_is_used_to_create_th HelpBuilder createdHelpBuilder = null; var command = new Command("help-command"); - command.SetHandler(context => + command.SetHandler((context, cancellationToken) => { handlerWasCalled = true; context.HelpBuilder.Should().Be(createdHelpBuilder); @@ -331,37 +331,33 @@ public async Task When_help_builder_factory_is_specified_it_is_used_to_create_th } [Fact] - public async Task Command_InvokeAsync_can_cancel_from_middleware() + public async Task Middleware_can_throw_OperationCancelledException() { var handlerWasCalled = false; - var isCancelRequested = false; var command = new Command("the-command"); - command.SetHandler((InvocationContext context) => + command.SetHandler((InvocationContext context, CancellationToken cancellationToken) => { handlerWasCalled = true; - isCancelRequested = context.GetCancellationToken().IsCancellationRequested; return Task.FromResult(0); }); - - using CancellationTokenSource cts = new(); var parser = new CommandLineBuilder(new RootCommand { command }) - .AddMiddleware(async (context, next) => + .AddMiddleware((context, next) => { - context.LinkToken(cts.Token); - cts.Cancel(); - await next(context); + throw new OperationCanceledException(); }) + .UseExceptionHandler((ex, ctx) => ctx.ExitCode = ex is OperationCanceledException ? 123 : 456) .Build(); - await parser.InvokeAsync("the-command"); + int result = await parser.InvokeAsync("the-command"); - handlerWasCalled.Should().BeTrue(); - isCancelRequested.Should().BeTrue(); + // when the middleware throws, we never get to handler + handlerWasCalled.Should().BeFalse(); + result.Should().Be(123); } } } diff --git a/src/System.CommandLine.Tests/UseExceptionHandlerTests.cs b/src/System.CommandLine.Tests/UseExceptionHandlerTests.cs index d0e3c1d93b..04bf66f321 100644 --- a/src/System.CommandLine.Tests/UseExceptionHandlerTests.cs +++ b/src/System.CommandLine.Tests/UseExceptionHandlerTests.cs @@ -53,7 +53,7 @@ public async Task UseExceptionHandler_catches_middleware_exceptions_and_writes_d public async Task UseExceptionHandler_catches_command_handler_exceptions_and_sets_result_code_to_1() { var command = new Command("the-command"); - command.SetHandler(() => + command.SetHandler(_ => { throw new Exception("oops!"); // Help the compiler pick a CommandHandler.Create overload. @@ -78,7 +78,7 @@ public async Task UseExceptionHandler_catches_command_handler_exceptions_and_set public async Task UseExceptionHandler_catches_command_handler_exceptions_and_writes_details_to_standard_error() { var command = new Command("the-command"); - command.SetHandler(() => + command.SetHandler(_ => { throw new Exception("oops!"); // Help the compiler pick a CommandHandler.Create overload. diff --git a/src/System.CommandLine/Binding/BindingContext.cs b/src/System.CommandLine/Binding/BindingContext.cs index ccd9b2db5f..aa6144aec3 100644 --- a/src/System.CommandLine/Binding/BindingContext.cs +++ b/src/System.CommandLine/Binding/BindingContext.cs @@ -6,7 +6,6 @@ using System.CommandLine.Parsing; using System.Diagnostics.CodeAnalysis; using System.Linq; -using System.Threading; namespace System.CommandLine.Binding { @@ -17,10 +16,10 @@ public sealed class BindingContext : IServiceProvider { private HelpBuilder? _helpBuilder; - internal BindingContext(InvocationContext invocationContext, CancellationToken cancellationToken) + internal BindingContext(InvocationContext invocationContext) { InvocationContext = invocationContext; - ServiceProvider = new ServiceProvider(this, cancellationToken); + ServiceProvider = new ServiceProvider(this); ServiceProvider.AddService(_ => InvocationContext); } diff --git a/src/System.CommandLine/Handler.Func.cs b/src/System.CommandLine/Handler.Func.cs index 97f3a9f1e4..b791fecc53 100644 --- a/src/System.CommandLine/Handler.Func.cs +++ b/src/System.CommandLine/Handler.Func.cs @@ -3,6 +3,7 @@ using System.CommandLine.Binding; using System.CommandLine.Invocation; +using System.Threading; using System.Threading.Tasks; namespace System.CommandLine; @@ -17,15 +18,15 @@ public static partial class Handler /// public static void SetHandler( this Command command, - Func handle) => - command.Handler = new AnonymousCommandHandler(_ => handle()); + Func handle) => + command.Handler = new AnonymousCommandHandler((ctx, cancellationToken) => handle(cancellationToken)); /// /// Sets a command's handler based on a . /// public static void SetHandler( this Command command, - Func handle) => + Func handle) => command.Handler = new AnonymousCommandHandler(handle); /// @@ -33,14 +34,14 @@ public static void SetHandler( /// public static void SetHandler( this Command command, - Func handle, + Func handle, IValueDescriptor symbol) => command.Handler = new AnonymousCommandHandler( - context => + (context, cancellationToken) => { var value1 = GetValueForHandlerParameter(symbol, context); - return handle(value1!); + return handle(value1!, cancellationToken); }); /// @@ -48,16 +49,16 @@ public static void SetHandler( /// public static void SetHandler( this Command command, - Func handle, + Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2) => command.Handler = new AnonymousCommandHandler( - context => + (context, cancellationToken) => { var value1 = GetValueForHandlerParameter(symbol1, context); var value2 = GetValueForHandlerParameter(symbol2, context); - return handle(value1!, value2!); + return handle(value1!, value2!, cancellationToken); }); /// @@ -65,18 +66,18 @@ public static void SetHandler( /// public static void SetHandler( this Command command, - Func handle, + Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3) => command.Handler = new AnonymousCommandHandler( - context => + (context, cancellationToken) => { var value1 = GetValueForHandlerParameter(symbol1, context); var value2 = GetValueForHandlerParameter(symbol2, context); var value3 = GetValueForHandlerParameter(symbol3, context); - return handle(value1!, value2!, value3!); + return handle(value1!, value2!, value3!, cancellationToken); }); /// @@ -84,20 +85,20 @@ public static void SetHandler( /// public static void SetHandler( this Command command, - Func handle, + Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4) => command.Handler = new AnonymousCommandHandler( - context => + (context, cancellationToken) => { var value1 = GetValueForHandlerParameter(symbol1, context); var value2 = GetValueForHandlerParameter(symbol2, context); var value3 = GetValueForHandlerParameter(symbol3, context); var value4 = GetValueForHandlerParameter(symbol4, context); - return handle(value1!, value2!, value3!, value4!); + return handle(value1!, value2!, value3!, value4!, cancellationToken); }); /// @@ -105,14 +106,14 @@ public static void SetHandler( /// public static void SetHandler( this Command command, - Func handle, + Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5) => command.Handler = new AnonymousCommandHandler( - context => + (context, cancellationToken) => { var value1 = GetValueForHandlerParameter(symbol1, context); var value2 = GetValueForHandlerParameter(symbol2, context); @@ -120,7 +121,7 @@ public static void SetHandler( var value4 = GetValueForHandlerParameter(symbol4, context); var value5 = GetValueForHandlerParameter(symbol5, context); - return handle(value1!, value2!, value3!, value4!, value5!); + return handle(value1!, value2!, value3!, value4!, value5!, cancellationToken); }); /// @@ -128,7 +129,7 @@ public static void SetHandler( /// public static void SetHandler( this Command command, - Func handle, + Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, @@ -136,7 +137,7 @@ public static void SetHandler( IValueDescriptor symbol5, IValueDescriptor symbol6) => command.Handler = new AnonymousCommandHandler( - context => + (context, cancellationToken) => { var value1 = GetValueForHandlerParameter(symbol1, context); var value2 = GetValueForHandlerParameter(symbol2, context); @@ -145,7 +146,7 @@ public static void SetHandler( var value5 = GetValueForHandlerParameter(symbol5, context); var value6 = GetValueForHandlerParameter(symbol6, context); - return handle(value1!, value2!, value3!, value4!, value5!, value6!); + return handle(value1!, value2!, value3!, value4!, value5!, value6!, cancellationToken); }); /// @@ -153,7 +154,7 @@ public static void SetHandler( /// public static void SetHandler( this Command command, - Func handle, + Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, @@ -162,7 +163,7 @@ public static void SetHandler( IValueDescriptor symbol6, IValueDescriptor symbol7) => command.Handler = new AnonymousCommandHandler( - context => + (context, cancellationToken) => { var value1 = GetValueForHandlerParameter(symbol1, context); var value2 = GetValueForHandlerParameter(symbol2, context); @@ -172,7 +173,7 @@ public static void SetHandler( var value6 = GetValueForHandlerParameter(symbol6, context); var value7 = GetValueForHandlerParameter(symbol7, context); - return handle(value1!, value2!, value3!, value4!, value5!, value6!, value7!); + return handle(value1!, value2!, value3!, value4!, value5!, value6!, value7!, cancellationToken); }); /// @@ -180,7 +181,7 @@ public static void SetHandler( /// public static void SetHandler( this Command command, - Func handle, + Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, @@ -190,7 +191,7 @@ public static void SetHandler( IValueDescriptor symbol7, IValueDescriptor symbol8) => command.Handler = new AnonymousCommandHandler( - context => + (context, cancellationToken) => { var value1 = GetValueForHandlerParameter(symbol1, context); var value2 = GetValueForHandlerParameter(symbol2, context); @@ -201,6 +202,6 @@ public static void SetHandler( var value7 = GetValueForHandlerParameter(symbol7, context); var value8 = GetValueForHandlerParameter(symbol8, context); - return handle(value1!, value2!, value3!, value4!, value5!, value6!, value7!, value8!); + return handle(value1!, value2!, value3!, value4!, value5!, value6!, value7!, value8!, cancellationToken); }); } \ No newline at end of file diff --git a/src/System.CommandLine/ICommandHandler.cs b/src/System.CommandLine/ICommandHandler.cs index 1f48649742..4644bca98b 100644 --- a/src/System.CommandLine/ICommandHandler.cs +++ b/src/System.CommandLine/ICommandHandler.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.CommandLine.Invocation; +using System.Threading; using System.Threading.Tasks; namespace System.CommandLine @@ -22,7 +23,8 @@ public interface ICommandHandler /// Performs an action when the associated command is invoked on the command line. /// /// Provides context for the invocation, including parse results and binding support. + /// The token to monitor for cancellation requests. /// A value that can be used as the exit code for the process. - Task InvokeAsync(InvocationContext context); + Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken); } } diff --git a/src/System.CommandLine/Invocation/AnonymousCommandHandler.cs b/src/System.CommandLine/Invocation/AnonymousCommandHandler.cs index 962fb97e3e..3d6fdb2679 100644 --- a/src/System.CommandLine/Invocation/AnonymousCommandHandler.cs +++ b/src/System.CommandLine/Invocation/AnonymousCommandHandler.cs @@ -1,19 +1,20 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using System.Threading; using System.Threading.Tasks; namespace System.CommandLine.Invocation { - internal class AnonymousCommandHandler : ICommandHandler + internal sealed class AnonymousCommandHandler : ICommandHandler { - private readonly Func? _asyncHandle; + private readonly Func? _asyncHandle; private readonly Action? _syncHandle; - public AnonymousCommandHandler(Func handle) + internal AnonymousCommandHandler(Func handle) => _asyncHandle = handle ?? throw new ArgumentNullException(nameof(handle)); - public AnonymousCommandHandler(Action handle) + internal AnonymousCommandHandler(Action handle) => _syncHandle = handle ?? throw new ArgumentNullException(nameof(handle)); public int Invoke(InvocationContext context) @@ -25,18 +26,19 @@ public int Invoke(InvocationContext context) } return SyncUsingAsync(context); // kept in a separate method to avoid JITting - } - private int SyncUsingAsync(InvocationContext context) => InvokeAsync(context).GetAwaiter().GetResult(); + int SyncUsingAsync(InvocationContext context) + => InvokeAsync(context, CancellationToken.None).GetAwaiter().GetResult(); + } - public async Task InvokeAsync(InvocationContext context) + public async Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken) { if (_syncHandle is not null) { return Invoke(context); } - object returnValue = _asyncHandle!(context); + object returnValue = _asyncHandle!(context, cancellationToken); int ret; diff --git a/src/System.CommandLine/Invocation/InvocationContext.cs b/src/System.CommandLine/Invocation/InvocationContext.cs index 29cab558f3..49bdfa953e 100644 --- a/src/System.CommandLine/Invocation/InvocationContext.cs +++ b/src/System.CommandLine/Invocation/InvocationContext.cs @@ -1,50 +1,34 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. -using System.Collections.Generic; using System.CommandLine.Binding; using System.CommandLine.Help; using System.CommandLine.IO; using System.CommandLine.Parsing; -using System.Threading; namespace System.CommandLine.Invocation { /// /// Supports command invocation by providing access to parse results and other services. /// - public sealed class InvocationContext : IDisposable + public sealed class InvocationContext { private HelpBuilder? _helpBuilder; private BindingContext? _bindingContext; private IConsole? _console; - private readonly CancellationToken _token; - private LinkedList? _registrations; - private CancellationTokenSource? _source; /// The result of the current parse operation. /// The console to which output is to be written. - /// A cancellation token that can be used to cancel and invocation. - public InvocationContext( - ParseResult parseResult, - IConsole? console = null, - CancellationToken cancellationToken = default) + public InvocationContext(ParseResult parseResult, IConsole? console = null) { ParseResult = parseResult; _console = console; - - _source = new CancellationTokenSource(); - _token = _source.Token; - if (cancellationToken.CanBeCanceled) - { - LinkToken(cancellationToken); - } } /// /// The binding context for the current invocation. /// - public BindingContext BindingContext => _bindingContext ??= new BindingContext(this, _token); + public BindingContext BindingContext => _bindingContext ??= new BindingContext(this); /// /// The console to which output should be written during the current invocation. @@ -86,22 +70,6 @@ public IConsole Console /// As the is passed through the invocation pipeline to the associated with the invoked command, only the last value of this property will be the one applied. public Action? InvocationResult { get; set; } - /// - /// Gets a cancellation token that can be used to check if cancellation has been requested. - /// - public CancellationToken GetCancellationToken() => _token; - - internal void Cancel() - { - using var source = Interlocked.Exchange(ref _source, null); - source?.Cancel(); - } - - public void LinkToken(CancellationToken token) - { - (_registrations ??= new()).AddLast(token.Register(Cancel)); - } - /// public object? GetValue(Option option) => ParseResult.GetValue(option); @@ -117,19 +85,5 @@ public void LinkToken(CancellationToken token) /// public T GetValue(Argument argument) => ParseResult.GetValue(argument); - - /// - void IDisposable.Dispose() - { - Interlocked.Exchange(ref _source, null)?.Dispose(); - - if (_registrations is not null) - { - foreach (CancellationTokenRegistration registration in _registrations) - { - registration.Dispose(); - } - } - } } } diff --git a/src/System.CommandLine/Invocation/InvocationPipeline.cs b/src/System.CommandLine/Invocation/InvocationPipeline.cs index f7387e8c6a..d9b0adfe67 100644 --- a/src/System.CommandLine/Invocation/InvocationPipeline.cs +++ b/src/System.CommandLine/Invocation/InvocationPipeline.cs @@ -10,18 +10,19 @@ namespace System.CommandLine.Invocation { internal static class InvocationPipeline { - internal static async Task InvokeAsync(ParseResult parseResult, IConsole? console = null, CancellationToken cancellationToken = default) + internal static async Task InvokeAsync(ParseResult parseResult, IConsole? console, CancellationToken cancellationToken) { - using InvocationContext context = new (parseResult, console, cancellationToken); + using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + InvocationContext context = new (parseResult, console); try { if (context.Parser.Configuration.Middleware.Count == 0 && parseResult.Handler is not null) { - return await parseResult.Handler.InvokeAsync(context); + return await parseResult.Handler.InvokeAsync(context, cts.Token); } - return await InvokeHandlerWithMiddleware(context); + return await InvokeHandlerWithMiddleware(context, cts.Token); } catch (Exception ex) when (context.Parser.Configuration.ExceptionHandler is not null) { @@ -29,9 +30,9 @@ internal static async Task InvokeAsync(ParseResult parseResult, IConsole? c return context.ExitCode; } - static async Task InvokeHandlerWithMiddleware(InvocationContext context) + static async Task InvokeHandlerWithMiddleware(InvocationContext context, CancellationToken token) { - InvocationMiddleware invocationChain = BuildInvocationChain(context, true); + InvocationMiddleware invocationChain = BuildInvocationChain(context, token, true); await invocationChain(context, _ => Task.CompletedTask); @@ -41,7 +42,7 @@ static async Task InvokeHandlerWithMiddleware(InvocationContext context) internal static int Invoke(ParseResult parseResult, IConsole? console = null) { - using InvocationContext context = new (parseResult, console); + InvocationContext context = new (parseResult, console); try { @@ -60,7 +61,7 @@ internal static int Invoke(ParseResult parseResult, IConsole? console = null) static int InvokeHandlerWithMiddleware(InvocationContext context) { - InvocationMiddleware invocationChain = BuildInvocationChain(context, false); + InvocationMiddleware invocationChain = BuildInvocationChain(context, CancellationToken.None, false); invocationChain(context, static _ => Task.CompletedTask).ConfigureAwait(false).GetAwaiter().GetResult(); @@ -68,7 +69,7 @@ static int InvokeHandlerWithMiddleware(InvocationContext context) } } - private static InvocationMiddleware BuildInvocationChain(InvocationContext context, bool invokeAsync) + private static InvocationMiddleware BuildInvocationChain(InvocationContext context, CancellationToken cancellationToken, bool invokeAsync) { var invocations = new List(context.Parser.Configuration.Middleware.Count + 1); invocations.AddRange(context.Parser.Configuration.Middleware); @@ -78,7 +79,7 @@ private static InvocationMiddleware BuildInvocationChain(InvocationContext conte if (invocationContext.ParseResult.Handler is { } handler) { context.ExitCode = invokeAsync - ? await handler.InvokeAsync(invocationContext) + ? await handler.InvokeAsync(invocationContext, cancellationToken) : handler.Invoke(invocationContext); } }); diff --git a/src/System.CommandLine/Invocation/ServiceProvider.cs b/src/System.CommandLine/Invocation/ServiceProvider.cs index 6f55479fc1..6610fcbc3c 100644 --- a/src/System.CommandLine/Invocation/ServiceProvider.cs +++ b/src/System.CommandLine/Invocation/ServiceProvider.cs @@ -12,13 +12,12 @@ internal sealed class ServiceProvider : IServiceProvider { private readonly Dictionary> _services; - internal ServiceProvider(BindingContext bindingContext, CancellationToken cancellationToken) + internal ServiceProvider(BindingContext bindingContext) { _services = new Dictionary> { [typeof(ParseResult)] = _ => bindingContext.ParseResult, [typeof(IConsole)] = _ => bindingContext.Console, - [typeof(CancellationToken)] = _ => cancellationToken, [typeof(HelpBuilder)] = _ => bindingContext.ParseResult.Parser.Configuration.HelpBuilderFactory(bindingContext), [typeof(BindingContext)] = _ => bindingContext }; From a5d21d087701aeafde0ade67fada28d91a0664f7 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 6 Feb 2023 16:23:24 +0100 Subject: [PATCH 10/19] move CancelOnProcessTermination out of middleware --- .../Binding/SetHandlerTests.cs | 32 +++++++ .../Builder/CommandLineBuilder.cs | 3 + .../Builder/CommandLineBuilderExtensions.cs | 84 +------------------ .../CommandLineConfiguration.cs | 6 +- .../Invocation/InvocationPipeline.cs | 57 +++++++++++-- .../Invocation/MiddlewareOrder.cs | 1 - 6 files changed, 94 insertions(+), 89 deletions(-) diff --git a/src/System.CommandLine.Tests/Binding/SetHandlerTests.cs b/src/System.CommandLine.Tests/Binding/SetHandlerTests.cs index 29ff9ceb3c..5b421460a9 100644 --- a/src/System.CommandLine.Tests/Binding/SetHandlerTests.cs +++ b/src/System.CommandLine.Tests/Binding/SetHandlerTests.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.CommandLine.Binding; using System.CommandLine.IO; +using System.CommandLine.Parsing; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -270,5 +271,36 @@ public async Task Unexpected_return_types_result_in_exit_code_0_if_no_exception_ wasCalled.Should().BeTrue(); exitCode.Should().Be(0); } + + [Fact] + public async Task When_User_Requests_Cancellation_Its_Reflected_By_The_Token_Passed_To_Handler() + { + const int ExpectedExitCode = 123; + + Command command = new ("the-command"); + command.SetHandler(async (context, cancellationToken) => + { + try + { + await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken); + context.ExitCode = ExpectedExitCode * -1; + } + catch (OperationCanceledException) + { + context.ExitCode = ExpectedExitCode; + } + }); + + using CancellationTokenSource cts = new (); + + Parser parser = new CommandLineBuilder(new RootCommand { command }) + .Build(); + + Task invokeResult = parser.InvokeAsync("the-command", null, cts.Token); + + cts.Cancel(); + + (await invokeResult).Should().Be(ExpectedExitCode); + } } } \ No newline at end of file diff --git a/src/System.CommandLine/Builder/CommandLineBuilder.cs b/src/System.CommandLine/Builder/CommandLineBuilder.cs index 956663acc4..d45654bbc8 100644 --- a/src/System.CommandLine/Builder/CommandLineBuilder.cs +++ b/src/System.CommandLine/Builder/CommandLineBuilder.cs @@ -41,6 +41,8 @@ public class CommandLineBuilder /// internal Action? ExceptionHandler; + internal TimeSpan? ProcessTerminationTimeout; + // for every generic type with type argument being struct JIT needs to compile a dedicated version // (because each struct is of a different size) // that is why we don't use List for middleware @@ -112,6 +114,7 @@ public Parser Build() => parseErrorReportingExitCode: ParseErrorReportingExitCode, maxLevenshteinDistance: MaxLevenshteinDistance, exceptionHandler: ExceptionHandler, + processTerminationTimeout: ProcessTerminationTimeout, resources: LocalizationResources, middlewarePipeline: _middlewareList is null ? Array.Empty() diff --git a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs index 3ee71b811d..811706ad96 100644 --- a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs +++ b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs @@ -26,7 +26,7 @@ public static class CommandLineBuilderExtensions /// A command line builder. /// /// Optional timeout for the command to process the exit cancellation. - /// If not passed, or passed null or non-positive timeout (including ), no timeout is enforced. + /// If not passed , a default timeout of 2 seconds is enforced. /// If positive value is passed - command is forcefully terminated after the timeout with exit code 130 (as if was not called). /// Host enforced timeout for ProcessExit event cannot be extended - default is 2 seconds: https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.processexit?view=net-6.0. /// @@ -35,87 +35,7 @@ public static CommandLineBuilder CancelOnProcessTermination( this CommandLineBuilder builder, TimeSpan? timeout = null) { - // https://tldp.org/LDP/abs/html/exitcodes.html - 130 - script terminated by ctrl-c - const int SIGINT_EXIT_CODE = 130; - - if (timeout == null || timeout.Value < TimeSpan.Zero) - { - timeout = Timeout.InfiniteTimeSpan; - } - - builder.AddMiddleware(async (context, next) => - { - ConsoleCancelEventHandler? consoleHandler = null; - EventHandler? processExitHandler = null; - ManualResetEventSlim blockProcessExit = new(initialState: false); - - processExitHandler = (_, _) => - { - // Cancel asynchronously not to block the handler (as then the process might possibly run longer then what was the requested timeout) - Task timeoutTask = Task.Delay(timeout.Value); - Task cancelTask = Task.Factory.StartNew(context.Cancel); - - // The process exits as soon as the event handler returns. - // We provide a return value using Environment.ExitCode - // because Main will not finish executing. - // Wait for the invocation to finish. - if (!blockProcessExit.Wait(timeout > TimeSpan.Zero - ? timeout.Value - : Timeout.InfiniteTimeSpan)) - { - context.ExitCode = SIGINT_EXIT_CODE; - } - // Let's block here (to prevent process bailing out) for the rest of the timeout (if any), for cancellation to finish (if it hasn't yet) - else if (Task.WaitAny(timeoutTask, cancelTask) == 0) - { - // The async cancellation didn't finish in timely manner - context.ExitCode = SIGINT_EXIT_CODE; - } - ExitCode = context.ExitCode; - }; - // Default limit for ProcesExit handler is 2 seconds - // https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.processexit?view=net-6.0 - consoleHandler = (_, args) => - { - // Stop the process from terminating. - // Since the context was cancelled, the invocation should - // finish and Main will return. - args.Cancel = true; - - // If timeout was requested - make sure cancellation processing (or any other activity within the current process) - // doesn't keep the process running after the timeout - if (timeout! > TimeSpan.Zero) - { - Task - .Delay(timeout.Value, default) - .ContinueWith(t => - { - // Prevent our ProcessExit from intervene and block the exit - AppDomain.CurrentDomain.ProcessExit -= processExitHandler; - Environment.Exit(SIGINT_EXIT_CODE); - }, (CancellationToken)default); - } - - // Cancel synchronously here - no need to perform it asynchronously as the timeout is already running (and would kill the process if needed), - // plus we cannot wait only on the cancellation (e.g. via `Task.Factory.StartNew(cts.Cancel).Wait(cancelationProcessingTimeout.Value)`) - // as we need to abort any other possible execution within the process - even outside the context of cancellation processing - context.Cancel(); - }; - - Console.CancelKeyPress += consoleHandler; - AppDomain.CurrentDomain.ProcessExit += processExitHandler; - - try - { - await next(context); - } - finally - { - Console.CancelKeyPress -= consoleHandler; - AppDomain.CurrentDomain.ProcessExit -= processExitHandler; - blockProcessExit?.Set(); - } - }, MiddlewareOrderInternal.Startup); + builder.ProcessTerminationTimeout = timeout ?? TimeSpan.FromSeconds(2); return builder; } diff --git a/src/System.CommandLine/CommandLineConfiguration.cs b/src/System.CommandLine/CommandLineConfiguration.cs index da668dee3c..3632e2855e 100644 --- a/src/System.CommandLine/CommandLineConfiguration.cs +++ b/src/System.CommandLine/CommandLineConfiguration.cs @@ -47,6 +47,8 @@ public class CommandLineConfiguration /// internal readonly int MaxLevenshteinDistance; + internal readonly TimeSpan? ProcessTerminationTimeout; + internal readonly IReadOnlyList Middleware; private Func? _helpBuilderFactory; @@ -72,7 +74,7 @@ public CommandLineConfiguration( IReadOnlyList? middlewarePipeline = null, Func? helpBuilderFactory = null, TryReplaceToken? tokenReplacer = null) - : this(command, enablePosixBundling, enableDirectives, enableTokenReplacement, false, null, false, null, 0, + : this(command, enablePosixBundling, enableDirectives, enableTokenReplacement, false, null, false, null, 0, null, resources, middlewarePipeline, helpBuilderFactory, tokenReplacer, null) { } @@ -87,6 +89,7 @@ internal CommandLineConfiguration( bool enableSuggestDirective, int? parseErrorReportingExitCode, int maxLevenshteinDistance, + TimeSpan? processTerminationTimeout, LocalizationResources? resources, IReadOnlyList? middlewarePipeline, Func? helpBuilderFactory, @@ -102,6 +105,7 @@ internal CommandLineConfiguration( EnableSuggestDirective = enableSuggestDirective; ParseErrorReportingExitCode = parseErrorReportingExitCode; MaxLevenshteinDistance = maxLevenshteinDistance; + ProcessTerminationTimeout = processTerminationTimeout; LocalizationResources = resources ?? LocalizationResources.Instance; Middleware = middlewarePipeline ?? Array.Empty(); diff --git a/src/System.CommandLine/Invocation/InvocationPipeline.cs b/src/System.CommandLine/Invocation/InvocationPipeline.cs index d9b0adfe67..577ebabc7b 100644 --- a/src/System.CommandLine/Invocation/InvocationPipeline.cs +++ b/src/System.CommandLine/Invocation/InvocationPipeline.cs @@ -10,25 +10,51 @@ namespace System.CommandLine.Invocation { internal static class InvocationPipeline { + // https://tldp.org/LDP/abs/html/exitcodes.html - 130 - script terminated by ctrl-c + private const int SIGINT_EXIT_CODE = 130; + internal static async Task InvokeAsync(ParseResult parseResult, IConsole? console, CancellationToken cancellationToken) { - using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); InvocationContext context = new (parseResult, console); + TimeSpan? processTerminationTimeout = parseResult.Parser.Configuration.ProcessTerminationTimeout; + TaskCompletionSource? processTerminationCompletionSource = processTerminationTimeout.HasValue ? new () : null; + + using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + + Task startedInvocation = parseResult.Handler is not null && context.Parser.Configuration.Middleware.Count == 0 + ? parseResult.Handler.InvokeAsync(context, cts.Token) + : InvokeHandlerWithMiddleware(context, cts.Token); + + if (processTerminationTimeout.HasValue) + { + Console.CancelKeyPress += OnControlCPressed; + AppDomain.CurrentDomain.ProcessExit += OnWindowClosed; + } try { - if (context.Parser.Configuration.Middleware.Count == 0 && parseResult.Handler is not null) + if (processTerminationCompletionSource is null) { - return await parseResult.Handler.InvokeAsync(context, cts.Token); + return await startedInvocation; + } + else + { + return await (await Task.WhenAny(startedInvocation, processTerminationCompletionSource.Task)); } - - return await InvokeHandlerWithMiddleware(context, cts.Token); } catch (Exception ex) when (context.Parser.Configuration.ExceptionHandler is not null) { context.Parser.Configuration.ExceptionHandler(ex, context); return context.ExitCode; } + finally + { + if (processTerminationTimeout.HasValue) + { + Console.CancelKeyPress -= OnControlCPressed; + AppDomain.CurrentDomain.ProcessExit -= OnWindowClosed; + } + } static async Task InvokeHandlerWithMiddleware(InvocationContext context, CancellationToken token) { @@ -38,6 +64,27 @@ static async Task InvokeHandlerWithMiddleware(InvocationContext context, Ca return GetExitCode(context); } + + async void OnControlCPressed(object? sender, ConsoleCancelEventArgs e) + { + e.Cancel = true; + + await CancelAsync(); + } + + async void OnWindowClosed(object? sender, EventArgs e) => await CancelAsync(); + + async Task CancelAsync() + { + cts.Cancel(); + + Task finishedFirst = await Task.WhenAny(startedInvocation, Task.Delay(processTerminationTimeout!.Value!)); + + if (finishedFirst != startedInvocation) + { + processTerminationCompletionSource!.SetResult(SIGINT_EXIT_CODE); + } + } } internal static int Invoke(ParseResult parseResult, IConsole? console = null) diff --git a/src/System.CommandLine/Invocation/MiddlewareOrder.cs b/src/System.CommandLine/Invocation/MiddlewareOrder.cs index 419196a2b5..b4eab4ea83 100644 --- a/src/System.CommandLine/Invocation/MiddlewareOrder.cs +++ b/src/System.CommandLine/Invocation/MiddlewareOrder.cs @@ -31,7 +31,6 @@ public enum MiddlewareOrder internal enum MiddlewareOrderInternal { - Startup = -4000, RegisterWithDotnetSuggest = -2400, } } \ No newline at end of file From cf466ca45cb4642fe7839dcbc84a730999292af6 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 6 Feb 2023 22:05:41 +0100 Subject: [PATCH 11/19] fix EndToEndTestApp --- .../EndToEndTestApp/Program.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/System.CommandLine.Suggest.Tests/EndToEndTestApp/Program.cs b/src/System.CommandLine.Suggest.Tests/EndToEndTestApp/Program.cs index b45c16929e..02f4da77fd 100644 --- a/src/System.CommandLine.Suggest.Tests/EndToEndTestApp/Program.cs +++ b/src/System.CommandLine.Suggest.Tests/EndToEndTestApp/Program.cs @@ -1,6 +1,7 @@ using System.CommandLine; using System.CommandLine.Parsing; using System.Threading.Tasks; +using System.Threading; namespace EndToEndTestApp { @@ -22,7 +23,7 @@ static async Task Main(string[] args) }; rootCommand.SetHandler( - (string apple, string banana, string cherry, string durian) => Task.CompletedTask, + (string apple, string banana, string cherry, string durian, CancellationToken cancellationToken) => Task.CompletedTask, appleOption, bananaOption, cherryOption, From 0861eb95c21d8d2d1aa153de4cced1b71f58f541 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 7 Feb 2023 09:30:18 +0100 Subject: [PATCH 12/19] SIGTERM fixes, test improvements --- .../CancelOnProcessTerminationTests.cs | 289 +++++------------- .../Invocation/InvocationPipeline.cs | 28 +- 2 files changed, 96 insertions(+), 221 deletions(-) diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index f38c200dcf..94b6651f80 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -2,11 +2,11 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using FluentAssertions; -using System.CommandLine.Invocation; using System.CommandLine.Parsing; using System.CommandLine.Tests.Utility; using System.Diagnostics; using System.Runtime.InteropServices; +using System.Threading; using System.Threading.Tasks; using Xunit; using Process = System.Diagnostics.Process; @@ -15,216 +15,85 @@ namespace System.CommandLine.Tests.Invocation { public class CancelOnProcessTerminationTests { - private const int SIGINT = 2; - private const int SIGTERM = 15; + private const string ChildProcessWaiting = "Waiting for the command to be cancelled"; + private const int SIGINT_EXIT_CODE = 130; + private const int SIGTERM_EXIT_CODE = 143; + private const int GracefulExitCode = 42; - [LinuxOnlyTheory] - [InlineData(SIGINT/*, Skip = "https://github.com/dotnet/command-line-api/issues/1206"*/)] // Console.CancelKeyPress - [InlineData(SIGTERM)] // AppDomain.CurrentDomain.ProcessExit - public async Task CancelOnProcessTermination_provides_CancellationToken_that_signals_termination_when_no_timeout_is_specified(int signo) + public enum Signals { - const string ChildProcessWaiting = "Waiting for the command to be cancelled"; - const int CancelledExitCode = 42; - - Func> childProgram = (string[] args) => - { - var command = new Command("the-command"); - - command.SetHandler(async (context, cancellationToken) => - { - try - { - context.Console.WriteLine(ChildProcessWaiting); - await Task.Delay(int.MaxValue, cancellationToken); - context.ExitCode = 1; - } - catch (OperationCanceledException) - { - // For Process.Exit handling the event must remain blocked as long as the - // command is executed. - // We are currently blocking that event because CancellationTokenSource.Cancel - // is called from the event handler. - // We'll do an async Yield now. This means the Cancel call will return - // and we're no longer actively blocking the event. - // The event handler is responsible to continue blocking until the command - // has finished executing. If it doesn't we won't get the CancelledExitCode. - await Task.Yield(); - - context.ExitCode = CancelledExitCode; - } - - }); - - return new CommandLineBuilder(new RootCommand - { - command - }) - .CancelOnProcessTermination() - .Build() - .InvokeAsync("the-command"); - }; - - using RemoteExecution program = RemoteExecutor.Execute(childProgram, psi: new ProcessStartInfo { RedirectStandardOutput = true }); - - Process process = program.Process; - - // Wait for the child to be in the command handler. - string childState = await process.StandardOutput.ReadLineAsync(); - childState.Should().Be(ChildProcessWaiting); - - // Request termination - kill(process.Id, signo).Should().Be(0); - - // Verify the process terminates timely - bool processExited = process.WaitForExit(10000); - if (!processExited) - { - process.Kill(); - process.WaitForExit(); - } - processExited.Should().Be(true); - - // Verify the process exit code - process.ExitCode.Should().Be(CancelledExitCode); + SIGINT = 2, // Console.CancelKeyPress + SIGTERM = 15 // AppDomain.CurrentDomain.ProcessExit } [LinuxOnlyTheory] - [InlineData(SIGINT)] - [InlineData(SIGTERM)] - public async Task CancelOnProcessTermination_provides_CancellationToken_that_signals_termination_when_null_timeout_is_specified(int signo) - { - const string ChildProcessWaiting = "Waiting for the command to be cancelled"; - const int CancelledExitCode = 42; - - Func> childProgram = (string[] args) => - { - var command = new Command("the-command"); - - command.SetHandler(async (context, cancellationToken) => - { - try - { - context.Console.WriteLine(ChildProcessWaiting); - await Task.Delay(int.MaxValue, cancellationToken); - context.ExitCode = 1; - } - catch (OperationCanceledException) - { - // For Process.Exit handling the event must remain blocked as long as the - // command is executed. - // We are currently blocking that event because CancellationTokenSource.Cancel - // is called from the event handler. - // We'll do an async Yield now. This means the Cancel call will return - // and we're no longer actively blocking the event. - // The event handler is responsible to continue blocking until the command - // has finished executing. If it doesn't we won't get the CancelledExitCode. - await Task.Yield(); - - // Exit code gets set here - but then execution continues and is let run till code voluntarily returns - // hence exit code gets overwritten below - context.ExitCode = 123; - } - - // This is an example of bad pattern and reason why we need a timeout on termination processing - await Task.Delay(TimeSpan.FromMilliseconds(200)); - - context.ExitCode = CancelledExitCode; - }); - - return new CommandLineBuilder(new RootCommand - { - command - }) - // Unfortunately we cannot use test parameter here - RemoteExecutor currently doesn't capture the closure - .CancelOnProcessTermination(null) - .Build() - .InvokeAsync("the-command"); - }; - - using RemoteExecution program = RemoteExecutor.Execute(childProgram, psi: new ProcessStartInfo { RedirectStandardOutput = true }); - - Process process = program.Process; - - // Wait for the child to be in the command handler. - string childState = await process.StandardOutput.ReadLineAsync(); - childState.Should().Be(ChildProcessWaiting); - - // Request termination - kill(process.Id, signo).Should().Be(0); - - // Verify the process terminates timely - bool processExited = process.WaitForExit(10000); - if (!processExited) - { - process.Kill(); - process.WaitForExit(); - } - processExited.Should().Be(true); - - // Verify the process exit code - process.ExitCode.Should().Be(CancelledExitCode); - } + [InlineData(Signals.SIGINT)] + [InlineData(Signals.SIGTERM)] + public Task CancellableHandler_is_cancelled_on_process_termination_when_no_timeout_is_specified(Signals signal) + => StartKillAndVerify(args => + Program(args, infiniteDelay: false, processTerminationTimeout: null), + signal, + GracefulExitCode); [LinuxOnlyTheory] - [InlineData(SIGINT)] - [InlineData(SIGTERM)] - public async Task CancelOnProcessTermination_provides_CancellationToken_that_signals_termination_and_execution_is_terminated_at_the_specified_timeout(int signo) + [InlineData(Signals.SIGINT)] + [InlineData(Signals.SIGTERM)] + public Task CancellableHandler_is_cancelled_on_process_termination_when_explicit_timeout_is_specified(Signals signo) + => StartKillAndVerify(args => + Program(args, infiniteDelay: false, processTerminationTimeout: TimeSpan.FromSeconds(1)), + signo, + GracefulExitCode); + + [LinuxOnlyTheory] + [InlineData(Signals.SIGINT, SIGINT_EXIT_CODE)] + [InlineData(Signals.SIGTERM, SIGTERM_EXIT_CODE)] + public Task NonCancellableHandler_is_interrupted_on_process_termination_when_no_timeout_is_specified(Signals signo, int expectedExitCode) + => StartKillAndVerify(args => + Program(args, infiniteDelay: true, processTerminationTimeout: null), + signo, + expectedExitCode); + + [LinuxOnlyTheory] + [InlineData(Signals.SIGINT, SIGINT_EXIT_CODE)] + [InlineData(Signals.SIGTERM, SIGTERM_EXIT_CODE)] + public Task NonCancellableHandler_is_interrupted_on_process_termination_when_explicit_timeout_is_specified(Signals signo, int expectedExitCode) + => StartKillAndVerify(args => + Program(args, infiniteDelay: true, processTerminationTimeout: TimeSpan.FromMilliseconds(100)), + signo, + expectedExitCode); + + private static async Task Program(string[] args, bool infiniteDelay, TimeSpan? processTerminationTimeout) { - const string ChildProcessWaiting = "Waiting for the command to be cancelled"; - const int CancelledExitCode = 42; - const int ForceTerminationCode = 130; + var command = new RootCommand(); - Func> childProgram = (string[] args) => + command.SetHandler(async (context, cancellationToken) => { - var command = new Command("the-command"); + context.Console.WriteLine(ChildProcessWaiting); - command.SetHandler(async (context, cancellationToken) => + try { - try - { - context.Console.WriteLine(ChildProcessWaiting); - await Task.Delay(int.MaxValue, cancellationToken); - context.ExitCode = 1; - } - catch (OperationCanceledException) - { - // For Process.Exit handling the event must remain blocked as long as the - // command is executed. - // We are currently blocking that event because CancellationTokenSource.Cancel - // is called from the event handler. - // We'll do an async Yield now. This means the Cancel call will return - // and we're no longer actively blocking the event. - // The event handler is responsible to continue blocking until the command - // has finished executing. If it doesn't we won't get the CancelledExitCode. - await Task.Yield(); - - context.ExitCode = CancelledExitCode; - - // This is an example of bad pattern and reason why we need a timeout on termination processing - await Task.Delay(TimeSpan.FromMilliseconds(1000)); - - // Execution should newer get here as termination processing has a timeout of 100ms - Environment.Exit(123); - } - - // This is an example of bad pattern and reason why we need a timeout on termination processing - await Task.Delay(TimeSpan.FromMilliseconds(1000)); - - // Execution should newer get here as termination processing has a timeout of 100ms - Environment.Exit(123); - }); - - return new CommandLineBuilder(new RootCommand - { - command - }) - // Unfortunately we cannot use test parameter here - RemoteExecutor currently doesn't capture the closure - .CancelOnProcessTermination(TimeSpan.FromMilliseconds(100)) - .Build() - .InvokeAsync("the-command"); - }; + // Passing CancellationToken.None here is an example of bad pattern + // and reason why we need a timeout on termination processing. + CancellationToken token = infiniteDelay ? CancellationToken.None : cancellationToken; + await Task.Delay(Timeout.InfiniteTimeSpan, token); + } + catch (OperationCanceledException) + { + context.ExitCode = GracefulExitCode; + } + }); + + int result = await new CommandLineBuilder(command) + .CancelOnProcessTermination(processTerminationTimeout) + .Build() + .InvokeAsync("the-command"); + + return result; + } - using RemoteExecution program = RemoteExecutor.Execute(childProgram, psi: new ProcessStartInfo { RedirectStandardOutput = true }); + private static async Task StartKillAndVerify(Func> childProgram, Signals signo, int expectedExitCode) + { + using RemoteExecution program = RemoteExecutor.Execute(childProgram, psi: new ProcessStartInfo { RedirectStandardOutput = true, RedirectStandardInput = true }); Process process = program.Process; @@ -233,22 +102,26 @@ public async Task CancelOnProcessTermination_provides_CancellationToken_that_sig childState.Should().Be(ChildProcessWaiting); // Request termination - kill(process.Id, signo).Should().Be(0); + kill(process.Id, (int)signo).Should().Be(0); // Verify the process terminates timely - bool processExited = process.WaitForExit(10000); - if (!processExited) + try + { + using CancellationTokenSource cts = new (TimeSpan.FromSeconds(10)); + await process.WaitForExitAsync(cts.Token); + } + catch (OperationCanceledException) { process.Kill(); - process.WaitForExit(); + + throw; } - processExited.Should().Be(true); // Verify the process exit code - process.ExitCode.Should().Be(ForceTerminationCode); + process.ExitCode.Should().Be(expectedExitCode); + + [DllImport("libc", SetLastError = true)] + static extern int kill(int pid, int sig); } - - [DllImport("libc", SetLastError = true)] - private static extern int kill(int pid, int sig); } -} +} \ No newline at end of file diff --git a/src/System.CommandLine/Invocation/InvocationPipeline.cs b/src/System.CommandLine/Invocation/InvocationPipeline.cs index 577ebabc7b..0beb11e9e2 100644 --- a/src/System.CommandLine/Invocation/InvocationPipeline.cs +++ b/src/System.CommandLine/Invocation/InvocationPipeline.cs @@ -11,7 +11,7 @@ namespace System.CommandLine.Invocation internal static class InvocationPipeline { // https://tldp.org/LDP/abs/html/exitcodes.html - 130 - script terminated by ctrl-c - private const int SIGINT_EXIT_CODE = 130; + private const int SIGINT_EXIT_CODE = 130, SIGTERM_EXIT_CODE = 143; internal static async Task InvokeAsync(ParseResult parseResult, IConsole? console, CancellationToken cancellationToken) { @@ -27,8 +27,8 @@ internal static async Task InvokeAsync(ParseResult parseResult, IConsole? c if (processTerminationTimeout.HasValue) { - Console.CancelKeyPress += OnControlCPressed; - AppDomain.CurrentDomain.ProcessExit += OnWindowClosed; + Console.CancelKeyPress += OnCancelKeyPress; + AppDomain.CurrentDomain.ProcessExit += OnProcessExit; } try @@ -51,8 +51,8 @@ internal static async Task InvokeAsync(ParseResult parseResult, IConsole? c { if (processTerminationTimeout.HasValue) { - Console.CancelKeyPress -= OnControlCPressed; - AppDomain.CurrentDomain.ProcessExit -= OnWindowClosed; + Console.CancelKeyPress -= OnCancelKeyPress; + AppDomain.CurrentDomain.ProcessExit -= OnProcessExit; } } @@ -65,24 +65,26 @@ static async Task InvokeHandlerWithMiddleware(InvocationContext context, Ca return GetExitCode(context); } - async void OnControlCPressed(object? sender, ConsoleCancelEventArgs e) + // Windows: user presses Ctrl+C + // Unix: the same + kill(SIGINT) + void OnCancelKeyPress(object? sender, ConsoleCancelEventArgs e) { e.Cancel = true; - await CancelAsync(); + Cancel(SIGINT_EXIT_CODE); } - async void OnWindowClosed(object? sender, EventArgs e) => await CancelAsync(); + // Windows: user closes console app windows + // Unix: kill(SIGTERM) + void OnProcessExit(object? sender, EventArgs e) => Cancel(SIGTERM_EXIT_CODE); - async Task CancelAsync() + void Cancel(int forcedTerminationExitCode) { cts.Cancel(); - Task finishedFirst = await Task.WhenAny(startedInvocation, Task.Delay(processTerminationTimeout!.Value!)); - - if (finishedFirst != startedInvocation) + if (!startedInvocation.Wait(processTerminationTimeout.Value)) { - processTerminationCompletionSource!.SetResult(SIGINT_EXIT_CODE); + processTerminationCompletionSource!.SetResult(forcedTerminationExitCode); } } } From ef9c1a1508417baf2315271851888cec8fd752f8 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 7 Feb 2023 17:47:44 +0100 Subject: [PATCH 13/19] use the new PosixSignalRegistration type --- .../CancelOnProcessTerminationTests.cs | 8 +- .../Invocation/InvocationPipeline.cs | 46 ++------- .../Invocation/ProcessTerminationHandler.cs | 93 +++++++++++++++++++ 3 files changed, 102 insertions(+), 45 deletions(-) create mode 100644 src/System.CommandLine/Invocation/ProcessTerminationHandler.cs diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index 94b6651f80..3e3522181f 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -62,7 +62,7 @@ public Task NonCancellableHandler_is_interrupted_on_process_termination_when_exp signo, expectedExitCode); - private static async Task Program(string[] args, bool infiniteDelay, TimeSpan? processTerminationTimeout) + private static Task Program(string[] args, bool infiniteDelay, TimeSpan? processTerminationTimeout) { var command = new RootCommand(); @@ -83,12 +83,10 @@ private static async Task Program(string[] args, bool infiniteDelay, TimeSp } }); - int result = await new CommandLineBuilder(command) + return new CommandLineBuilder(command) .CancelOnProcessTermination(processTerminationTimeout) .Build() - .InvokeAsync("the-command"); - - return result; + .InvokeAsync(""); } private static async Task StartKillAndVerify(Func> childProgram, Signals signo, int expectedExitCode) diff --git a/src/System.CommandLine/Invocation/InvocationPipeline.cs b/src/System.CommandLine/Invocation/InvocationPipeline.cs index 0beb11e9e2..576f666e6c 100644 --- a/src/System.CommandLine/Invocation/InvocationPipeline.cs +++ b/src/System.CommandLine/Invocation/InvocationPipeline.cs @@ -10,14 +10,9 @@ namespace System.CommandLine.Invocation { internal static class InvocationPipeline { - // https://tldp.org/LDP/abs/html/exitcodes.html - 130 - script terminated by ctrl-c - private const int SIGINT_EXIT_CODE = 130, SIGTERM_EXIT_CODE = 143; - internal static async Task InvokeAsync(ParseResult parseResult, IConsole? console, CancellationToken cancellationToken) { InvocationContext context = new (parseResult, console); - TimeSpan? processTerminationTimeout = parseResult.Parser.Configuration.ProcessTerminationTimeout; - TaskCompletionSource? processTerminationCompletionSource = processTerminationTimeout.HasValue ? new () : null; using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); @@ -25,21 +20,19 @@ internal static async Task InvokeAsync(ParseResult parseResult, IConsole? c ? parseResult.Handler.InvokeAsync(context, cts.Token) : InvokeHandlerWithMiddleware(context, cts.Token); - if (processTerminationTimeout.HasValue) - { - Console.CancelKeyPress += OnCancelKeyPress; - AppDomain.CurrentDomain.ProcessExit += OnProcessExit; - } + ProcessTerminationHandler? terminationHandler = parseResult.Parser.Configuration.ProcessTerminationTimeout.HasValue + ? new (cts, startedInvocation, parseResult.Parser.Configuration.ProcessTerminationTimeout.Value) + : null; try { - if (processTerminationCompletionSource is null) + if (terminationHandler is null) { return await startedInvocation; } else { - return await (await Task.WhenAny(startedInvocation, processTerminationCompletionSource.Task)); + return await (await Task.WhenAny(startedInvocation, terminationHandler.ProcessTerminationCompletionSource.Task)); } } catch (Exception ex) when (context.Parser.Configuration.ExceptionHandler is not null) @@ -49,11 +42,7 @@ internal static async Task InvokeAsync(ParseResult parseResult, IConsole? c } finally { - if (processTerminationTimeout.HasValue) - { - Console.CancelKeyPress -= OnCancelKeyPress; - AppDomain.CurrentDomain.ProcessExit -= OnProcessExit; - } + terminationHandler?.Dispose(); } static async Task InvokeHandlerWithMiddleware(InvocationContext context, CancellationToken token) @@ -64,29 +53,6 @@ static async Task InvokeHandlerWithMiddleware(InvocationContext context, Ca return GetExitCode(context); } - - // Windows: user presses Ctrl+C - // Unix: the same + kill(SIGINT) - void OnCancelKeyPress(object? sender, ConsoleCancelEventArgs e) - { - e.Cancel = true; - - Cancel(SIGINT_EXIT_CODE); - } - - // Windows: user closes console app windows - // Unix: kill(SIGTERM) - void OnProcessExit(object? sender, EventArgs e) => Cancel(SIGTERM_EXIT_CODE); - - void Cancel(int forcedTerminationExitCode) - { - cts.Cancel(); - - if (!startedInvocation.Wait(processTerminationTimeout.Value)) - { - processTerminationCompletionSource!.SetResult(forcedTerminationExitCode); - } - } } internal static int Invoke(ParseResult parseResult, IConsole? console = null) diff --git a/src/System.CommandLine/Invocation/ProcessTerminationHandler.cs b/src/System.CommandLine/Invocation/ProcessTerminationHandler.cs new file mode 100644 index 0000000000..4b91d9d5dd --- /dev/null +++ b/src/System.CommandLine/Invocation/ProcessTerminationHandler.cs @@ -0,0 +1,93 @@ +using System.Runtime.InteropServices; +using System.Threading; +using System.Threading.Tasks; + +namespace System.CommandLine.Invocation; + +internal sealed class ProcessTerminationHandler : IDisposable +{ + private const int SIGINT_EXIT_CODE = 130; + private const int SIGTERM_EXIT_CODE = 143; + + internal readonly TaskCompletionSource ProcessTerminationCompletionSource; + private readonly CancellationTokenSource _cts; + private readonly Task _startedInvocation; + private readonly TimeSpan _processTerminationTimeout; + private readonly IDisposable? _sigIntRegistration, _sigTermRegistration; + + internal ProcessTerminationHandler( + CancellationTokenSource cts, + Task startedInvocation, + TimeSpan processTerminationTimeout) + { + ProcessTerminationCompletionSource = new (); + _cts = cts; + _startedInvocation = startedInvocation; + _processTerminationTimeout = processTerminationTimeout; + +#if NET7_0_OR_GREATER + if (!OperatingSystem.IsAndroid() + && !OperatingSystem.IsIOS() + && !OperatingSystem.IsTvOS() + && !OperatingSystem.IsBrowser()) + { + _sigIntRegistration = PosixSignalRegistration.Create(PosixSignal.SIGINT, OnPosixSignal); + _sigTermRegistration = PosixSignalRegistration.Create(PosixSignal.SIGTERM, OnPosixSignal); + return; + } +#endif + + Console.CancelKeyPress += OnCancelKeyPress; + AppDomain.CurrentDomain.ProcessExit += OnProcessExit; + } + + public void Dispose() + { + if (_sigIntRegistration is not null) + { + _sigIntRegistration.Dispose(); + _sigTermRegistration!.Dispose(); + } + else + { + Console.CancelKeyPress -= OnCancelKeyPress; + AppDomain.CurrentDomain.ProcessExit -= OnProcessExit; + } + } + +#if NET7_0_OR_GREATER + void OnPosixSignal(PosixSignalContext context) + { + context.Cancel = true; + + Cancel(context.Signal == PosixSignal.SIGINT ? SIGINT_EXIT_CODE : SIGTERM_EXIT_CODE); + } +#endif + + void OnCancelKeyPress(object? sender, ConsoleCancelEventArgs e) + { + e.Cancel = true; + + Cancel(SIGINT_EXIT_CODE); + } + + void OnProcessExit(object? sender, EventArgs e) => Cancel(SIGTERM_EXIT_CODE); + + void Cancel(int forcedTerminationExitCode) + { + try + { + _cts.Cancel(); + } + catch (Exception ex) + { + ProcessTerminationCompletionSource.SetException(ex); + return; + } + + if (!_startedInvocation.Wait(_processTerminationTimeout)) + { + ProcessTerminationCompletionSource.SetResult(forcedTerminationExitCode); + } + } +} \ No newline at end of file From d43d5362988c43d90d108c35b61c84997279650d Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 13 Feb 2023 16:31:16 +0100 Subject: [PATCH 14/19] more improvements --- .../CancelOnProcessTerminationTests.cs | 76 +++++++++---------- .../Invocation/AnonymousCommandHandler.cs | 24 ++---- .../Invocation/InvocationPipeline.cs | 6 +- .../Invocation/ProcessTerminationHandler.cs | 30 ++++---- 4 files changed, 57 insertions(+), 79 deletions(-) diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index 3e3522181f..2c037b8b45 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -26,50 +26,39 @@ public enum Signals SIGTERM = 15 // AppDomain.CurrentDomain.ProcessExit } - [LinuxOnlyTheory] - [InlineData(Signals.SIGINT)] - [InlineData(Signals.SIGTERM)] - public Task CancellableHandler_is_cancelled_on_process_termination_when_no_timeout_is_specified(Signals signal) - => StartKillAndVerify(args => - Program(args, infiniteDelay: false, processTerminationTimeout: null), - signal, - GracefulExitCode); + [Fact] + public async Task CancellableHandler_is_cancelled_on_process_termination() + { + // the feature is supported on Windows, but it's simply harder to send SIGINT to test it properly + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + await StartKillAndVerify(new[] { "--infiniteDelay", "false" }, Signals.SIGINT, GracefulExitCode); + } + } - [LinuxOnlyTheory] - [InlineData(Signals.SIGINT)] - [InlineData(Signals.SIGTERM)] - public Task CancellableHandler_is_cancelled_on_process_termination_when_explicit_timeout_is_specified(Signals signo) - => StartKillAndVerify(args => - Program(args, infiniteDelay: false, processTerminationTimeout: TimeSpan.FromSeconds(1)), - signo, - GracefulExitCode); - - [LinuxOnlyTheory] - [InlineData(Signals.SIGINT, SIGINT_EXIT_CODE)] - [InlineData(Signals.SIGTERM, SIGTERM_EXIT_CODE)] - public Task NonCancellableHandler_is_interrupted_on_process_termination_when_no_timeout_is_specified(Signals signo, int expectedExitCode) - => StartKillAndVerify(args => - Program(args, infiniteDelay: true, processTerminationTimeout: null), - signo, - expectedExitCode); - - [LinuxOnlyTheory] - [InlineData(Signals.SIGINT, SIGINT_EXIT_CODE)] - [InlineData(Signals.SIGTERM, SIGTERM_EXIT_CODE)] - public Task NonCancellableHandler_is_interrupted_on_process_termination_when_explicit_timeout_is_specified(Signals signo, int expectedExitCode) - => StartKillAndVerify(args => - Program(args, infiniteDelay: true, processTerminationTimeout: TimeSpan.FromMilliseconds(100)), - signo, - expectedExitCode); + [Fact] + public async Task NonCancellableHandler_is_interrupted_on_process_termination() + { + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + await StartKillAndVerify(new[] { "--infiniteDelay", "true" }, Signals.SIGTERM, SIGTERM_EXIT_CODE); + } + } - private static Task Program(string[] args, bool infiniteDelay, TimeSpan? processTerminationTimeout) + private static Task Program(string[] args) { - var command = new RootCommand(); + Option infiniteDelayOption = new ("--infiniteDelay"); + RootCommand command = new () + { + infiniteDelayOption + }; command.SetHandler(async (context, cancellationToken) => { context.Console.WriteLine(ChildProcessWaiting); + bool infiniteDelay = context.GetValue(infiniteDelayOption); + try { // Passing CancellationToken.None here is an example of bad pattern @@ -84,14 +73,17 @@ private static Task Program(string[] args, bool infiniteDelay, TimeSpan? pr }); return new CommandLineBuilder(command) - .CancelOnProcessTermination(processTerminationTimeout) + .CancelOnProcessTermination() .Build() - .InvokeAsync(""); + .InvokeAsync(args); } - - private static async Task StartKillAndVerify(Func> childProgram, Signals signo, int expectedExitCode) + + private async Task StartKillAndVerify(string[] args, Signals signal, int expectedExitCode) { - using RemoteExecution program = RemoteExecutor.Execute(childProgram, psi: new ProcessStartInfo { RedirectStandardOutput = true, RedirectStandardInput = true }); + using RemoteExecution program = RemoteExecutor.Execute( + Program, + args, + new ProcessStartInfo { RedirectStandardOutput = true }); Process process = program.Process; @@ -100,7 +92,7 @@ private static async Task StartKillAndVerify(Func> childProg childState.Should().Be(ChildProcessWaiting); // Request termination - kill(process.Id, (int)signo).Should().Be(0); + kill(process.Id, (int)signal).Should().Be(0); // Verify the process terminates timely try diff --git a/src/System.CommandLine/Invocation/AnonymousCommandHandler.cs b/src/System.CommandLine/Invocation/AnonymousCommandHandler.cs index 3d6fdb2679..0a941e3948 100644 --- a/src/System.CommandLine/Invocation/AnonymousCommandHandler.cs +++ b/src/System.CommandLine/Invocation/AnonymousCommandHandler.cs @@ -38,28 +38,14 @@ public async Task InvokeAsync(InvocationContext context, CancellationToken return Invoke(context); } - object returnValue = _asyncHandle!(context, cancellationToken); - - int ret; - - switch (returnValue) + Task handler = _asyncHandle!(context, cancellationToken); + if (handler is Task intReturning) { - case Task exitCodeTask: - ret = await exitCodeTask; - break; - case Task task: - await task; - ret = context.ExitCode; - break; - case int exitCode: - ret = exitCode; - break; - default: - ret = context.ExitCode; - break; + return await intReturning; } - return ret; + await handler; + return context.ExitCode; } } } \ No newline at end of file diff --git a/src/System.CommandLine/Invocation/InvocationPipeline.cs b/src/System.CommandLine/Invocation/InvocationPipeline.cs index 576f666e6c..e98648ed1c 100644 --- a/src/System.CommandLine/Invocation/InvocationPipeline.cs +++ b/src/System.CommandLine/Invocation/InvocationPipeline.cs @@ -32,7 +32,11 @@ internal static async Task InvokeAsync(ParseResult parseResult, IConsole? c } else { - return await (await Task.WhenAny(startedInvocation, terminationHandler.ProcessTerminationCompletionSource.Task)); + // Handlers may not implement cancellation. + // In such cases, when CancelOnProcessTermination is configured and user presses Ctrl+C, + // ProcessTerminationCompletionSource completes first, with the result equal to native exit code for given signal. + Task firstCompletedTask = await Task.WhenAny(startedInvocation, terminationHandler.ProcessTerminationCompletionSource.Task); + return await firstCompletedTask; // return the result or propagate the exception } } catch (Exception ex) when (context.Parser.Configuration.ExceptionHandler is not null) diff --git a/src/System.CommandLine/Invocation/ProcessTerminationHandler.cs b/src/System.CommandLine/Invocation/ProcessTerminationHandler.cs index 4b91d9d5dd..03bb6d4042 100644 --- a/src/System.CommandLine/Invocation/ProcessTerminationHandler.cs +++ b/src/System.CommandLine/Invocation/ProcessTerminationHandler.cs @@ -10,22 +10,22 @@ internal sealed class ProcessTerminationHandler : IDisposable private const int SIGTERM_EXIT_CODE = 143; internal readonly TaskCompletionSource ProcessTerminationCompletionSource; - private readonly CancellationTokenSource _cts; - private readonly Task _startedInvocation; + private readonly CancellationTokenSource _handlerCancellationTokenSource; + private readonly Task _startedHandler; private readonly TimeSpan _processTerminationTimeout; private readonly IDisposable? _sigIntRegistration, _sigTermRegistration; internal ProcessTerminationHandler( - CancellationTokenSource cts, - Task startedInvocation, + CancellationTokenSource handlerCancellationTokenSource, + Task startedHandler, TimeSpan processTerminationTimeout) { ProcessTerminationCompletionSource = new (); - _cts = cts; - _startedInvocation = startedInvocation; + _handlerCancellationTokenSource = handlerCancellationTokenSource; + _startedHandler = startedHandler; _processTerminationTimeout = processTerminationTimeout; -#if NET7_0_OR_GREATER +#if NET7_0_OR_GREATER if (!OperatingSystem.IsAndroid() && !OperatingSystem.IsIOS() && !OperatingSystem.IsTvOS() @@ -75,18 +75,14 @@ void OnCancelKeyPress(object? sender, ConsoleCancelEventArgs e) void Cancel(int forcedTerminationExitCode) { - try - { - _cts.Cancel(); - } - catch (Exception ex) - { - ProcessTerminationCompletionSource.SetException(ex); - return; - } + // request cancellation + _handlerCancellationTokenSource.Cancel(); - if (!_startedInvocation.Wait(_processTerminationTimeout)) + // wait for the configured interval + if (!_startedHandler.Wait(_processTerminationTimeout)) { + // if the handler does not finish within configured time, + // use the completion source to signal forced completion (preserving native exit code) ProcessTerminationCompletionSource.SetResult(forcedTerminationExitCode); } } From 158e469a10fc34c26e0317e6a902c17c60c6171c Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 13 Feb 2023 20:55:53 +0100 Subject: [PATCH 15/19] fix compilation warnings --- .../Invocation/ProcessTerminationHandler.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/System.CommandLine/Invocation/ProcessTerminationHandler.cs b/src/System.CommandLine/Invocation/ProcessTerminationHandler.cs index 03bb6d4042..da83bddad1 100644 --- a/src/System.CommandLine/Invocation/ProcessTerminationHandler.cs +++ b/src/System.CommandLine/Invocation/ProcessTerminationHandler.cs @@ -13,7 +13,9 @@ internal sealed class ProcessTerminationHandler : IDisposable private readonly CancellationTokenSource _handlerCancellationTokenSource; private readonly Task _startedHandler; private readonly TimeSpan _processTerminationTimeout; +#if NET7_0_OR_GREATER private readonly IDisposable? _sigIntRegistration, _sigTermRegistration; +#endif internal ProcessTerminationHandler( CancellationTokenSource handlerCancellationTokenSource, @@ -25,7 +27,7 @@ internal ProcessTerminationHandler( _startedHandler = startedHandler; _processTerminationTimeout = processTerminationTimeout; -#if NET7_0_OR_GREATER +#if NET7_0_OR_GREATER // we prefer the new API as they allow for cancelling SIGTERM if (!OperatingSystem.IsAndroid() && !OperatingSystem.IsIOS() && !OperatingSystem.IsTvOS() @@ -43,16 +45,17 @@ internal ProcessTerminationHandler( public void Dispose() { +#if NET7_0_OR_GREATER if (_sigIntRegistration is not null) { _sigIntRegistration.Dispose(); _sigTermRegistration!.Dispose(); + return; } - else - { - Console.CancelKeyPress -= OnCancelKeyPress; - AppDomain.CurrentDomain.ProcessExit -= OnProcessExit; - } +#endif + + Console.CancelKeyPress -= OnCancelKeyPress; + AppDomain.CurrentDomain.ProcessExit -= OnProcessExit; } #if NET7_0_OR_GREATER From 577765ad0c608fa3b016e592eb5545c7b8eee243 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 13 Feb 2023 21:16:32 +0100 Subject: [PATCH 16/19] RemoteExecutor does not support getting application arguments, Process.WaitForExitAsync is available only on .NET 7+ --- .../CancelOnProcessTerminationTests.cs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs index 2c037b8b45..6e62a9c432 100644 --- a/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/CancelOnProcessTerminationTests.cs @@ -29,8 +29,9 @@ public enum Signals [Fact] public async Task CancellableHandler_is_cancelled_on_process_termination() { - // the feature is supported on Windows, but it's simply harder to send SIGINT to test it properly - if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + // The feature is supported on Windows, but it's simply harder to send SIGINT to test it properly. + // Same for macOS, where RemoteExecutor does not support getting application arguments. + if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { await StartKillAndVerify(new[] { "--infiniteDelay", "false" }, Signals.SIGINT, GracefulExitCode); } @@ -39,7 +40,7 @@ public async Task CancellableHandler_is_cancelled_on_process_termination() [Fact] public async Task NonCancellableHandler_is_interrupted_on_process_termination() { - if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) { await StartKillAndVerify(new[] { "--infiniteDelay", "true" }, Signals.SIGTERM, SIGTERM_EXIT_CODE); } @@ -95,16 +96,9 @@ private async Task StartKillAndVerify(string[] args, Signals signal, int expecte kill(process.Id, (int)signal).Should().Be(0); // Verify the process terminates timely - try - { - using CancellationTokenSource cts = new (TimeSpan.FromSeconds(10)); - await process.WaitForExitAsync(cts.Token); - } - catch (OperationCanceledException) + if (!process.WaitForExit((int)TimeSpan.FromSeconds(10).TotalMilliseconds)) { process.Kill(); - - throw; } // Verify the process exit code From 6114d76535604f8252881b08e74a1031bbee7c17 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 17 Feb 2023 16:53:00 +0100 Subject: [PATCH 17/19] set CancellationToken cancellationToken to default by default (this is what BCL does) --- ...dLine_NamingConventionBinder_api_is_not_changed.approved.txt | 2 +- ...ovalTests.System_CommandLine_api_is_not_changed.approved.txt | 2 +- .../ModelBindingCommandHandler.cs | 2 +- src/System.CommandLine/ICommandHandler.cs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_NamingConventionBinder_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_NamingConventionBinder_api_is_not_changed.approved.txt index 993330ea89..a8c045b3dd 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_NamingConventionBinder_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_NamingConventionBinder_api_is_not_changed.approved.txt @@ -99,7 +99,7 @@ public System.Void BindParameter(System.Reflection.ParameterInfo param, System.CommandLine.Argument argument) public System.Void BindParameter(System.Reflection.ParameterInfo param, System.CommandLine.Option option) public System.Int32 Invoke(System.CommandLine.Invocation.InvocationContext context) - public System.Threading.Tasks.Task InvokeAsync(System.CommandLine.Invocation.InvocationContext context, System.Threading.CancellationToken cancellationToken) + public System.Threading.Tasks.Task InvokeAsync(System.CommandLine.Invocation.InvocationContext context, System.Threading.CancellationToken cancellationToken = null) public class ModelDescriptor public static ModelDescriptor FromType() public static ModelDescriptor FromType(System.Type type) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index e22a11119f..025fa35e1f 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -136,7 +136,7 @@ System.CommandLine public static System.Void SetHandler(this Command command, Func handle, IValueDescriptor symbol1, IValueDescriptor symbol2, IValueDescriptor symbol3, IValueDescriptor symbol4, IValueDescriptor symbol5, IValueDescriptor symbol6, IValueDescriptor symbol7, IValueDescriptor symbol8) public interface ICommandHandler public System.Int32 Invoke(System.CommandLine.Invocation.InvocationContext context) - public System.Threading.Tasks.Task InvokeAsync(System.CommandLine.Invocation.InvocationContext context, System.Threading.CancellationToken cancellationToken) + public System.Threading.Tasks.Task InvokeAsync(System.CommandLine.Invocation.InvocationContext context, System.Threading.CancellationToken cancellationToken = null) public interface IConsole : System.CommandLine.IO.IStandardError, System.CommandLine.IO.IStandardIn, System.CommandLine.IO.IStandardOut public abstract class IdentifierSymbol : Symbol public System.Collections.Generic.IReadOnlyCollection Aliases { get; } diff --git a/src/System.CommandLine.NamingConventionBinder/ModelBindingCommandHandler.cs b/src/System.CommandLine.NamingConventionBinder/ModelBindingCommandHandler.cs index 4628653da0..97bf39bae4 100644 --- a/src/System.CommandLine.NamingConventionBinder/ModelBindingCommandHandler.cs +++ b/src/System.CommandLine.NamingConventionBinder/ModelBindingCommandHandler.cs @@ -57,7 +57,7 @@ internal ModelBindingCommandHandler( /// The current invocation context. /// A token that can be used to cancel the invocation. /// A task whose value can be used to set the process exit code. - public async Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken) + public async Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken = default) { var bindingContext = context.BindingContext; diff --git a/src/System.CommandLine/ICommandHandler.cs b/src/System.CommandLine/ICommandHandler.cs index 4644bca98b..c5d04dde72 100644 --- a/src/System.CommandLine/ICommandHandler.cs +++ b/src/System.CommandLine/ICommandHandler.cs @@ -25,6 +25,6 @@ public interface ICommandHandler /// Provides context for the invocation, including parse results and binding support. /// The token to monitor for cancellation requests. /// A value that can be used as the exit code for the process. - Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken); + Task InvokeAsync(InvocationContext context, CancellationToken cancellationToken = default); } } From 5905a9bc428b5d25a1ec6af831afda53add0fd86 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 17 Feb 2023 19:18:45 +0100 Subject: [PATCH 18/19] extend Middleware with CancellationToken so it can be passed to Hosting --- ...CommandLine_api_is_not_changed.approved.txt | 4 ++-- .../HostingTests.cs | 16 ++++++++-------- .../HostingExtensions.cs | 8 ++++---- .../InvocationLifetime.cs | 3 ++- .../Invocation/InvocationPipelineTests.cs | 12 ++++++------ .../Builder/CommandLineBuilderExtensions.cs | 10 +++++----- .../Invocation/InvocationMiddleware.cs | 5 ++++- .../Invocation/InvocationPipeline.cs | 18 +++++++++--------- 8 files changed, 40 insertions(+), 36 deletions(-) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index 025fa35e1f..3af763db30 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -340,9 +340,9 @@ System.CommandLine.Invocation public T GetValue(Argument argument) public delegate InvocationMiddleware : System.MulticastDelegate, System.ICloneable, System.Runtime.Serialization.ISerializable .ctor(System.Object object, System.IntPtr method) - public System.IAsyncResult BeginInvoke(InvocationContext context, System.Func next, System.AsyncCallback callback, System.Object object) + public System.IAsyncResult BeginInvoke(InvocationContext context, System.Threading.CancellationToken cancellationToken, System.Func next, System.AsyncCallback callback, System.Object object) public System.Threading.Tasks.Task EndInvoke(System.IAsyncResult result) - public System.Threading.Tasks.Task Invoke(InvocationContext context, System.Func next) + public System.Threading.Tasks.Task Invoke(InvocationContext context, System.Threading.CancellationToken cancellationToken, System.Func next) public enum MiddlewareOrder : System.Enum, System.IComparable, System.IConvertible, System.IFormattable Default=0 ErrorReporting=1000 diff --git a/src/System.CommandLine.Hosting.Tests/HostingTests.cs b/src/System.CommandLine.Hosting.Tests/HostingTests.cs index 83b6210606..a1e3d2b362 100644 --- a/src/System.CommandLine.Hosting.Tests/HostingTests.cs +++ b/src/System.CommandLine.Hosting.Tests/HostingTests.cs @@ -300,10 +300,10 @@ public static void GetInvocationContext_returns_same_instance_as_outer_middlewar InvocationContext ctxHosting = null; var parser = new CommandLineBuilder() - .AddMiddleware((context, next) => + .AddMiddleware((context, cancellationToken, next) => { ctxCustom = context; - return next(context); + return next(context, cancellationToken); }) .UseHost(hostBuilder => { @@ -323,10 +323,10 @@ public static void GetInvocationContext_in_ConfigureServices_returns_same_instan InvocationContext ctxConfigureServices = null; var parser = new CommandLineBuilder() - .AddMiddleware((context, next) => + .AddMiddleware((context, cancellationToken, next) => { ctxCustom = context; - return next(context); + return next(context, cancellationToken); }) .UseHost(hostBuilder => { @@ -373,13 +373,13 @@ public static void GetHost_returns_non_null_instance_in_subsequent_middleware() bool hostAsserted = false; var parser = new CommandLineBuilder() .UseHost() - .AddMiddleware((invCtx, next) => + .AddMiddleware((invCtx, cancellationToken, next) => { IHost host = invCtx.GetHost(); host.Should().NotBeNull(); hostAsserted = true; - return next(invCtx); + return next(invCtx, cancellationToken); }) .Build(); @@ -393,13 +393,13 @@ public static void GetHost_returns_null_when_no_host_in_invocation() { bool hostAsserted = false; var parser = new CommandLineBuilder() - .AddMiddleware((invCtx, next) => + .AddMiddleware((invCtx, cancellationToken, next) => { IHost host = invCtx.GetHost(); host.Should().BeNull(); hostAsserted = true; - return next(invCtx); + return next(invCtx, cancellationToken); }) .Build(); diff --git a/src/System.CommandLine.Hosting/HostingExtensions.cs b/src/System.CommandLine.Hosting/HostingExtensions.cs index 70c7969f57..1a4eb4637a 100644 --- a/src/System.CommandLine.Hosting/HostingExtensions.cs +++ b/src/System.CommandLine.Hosting/HostingExtensions.cs @@ -18,7 +18,7 @@ public static class HostingExtensions public static CommandLineBuilder UseHost(this CommandLineBuilder builder, Func hostBuilderFactory, Action configureHost = null) => - builder.AddMiddleware(async (invocation, next) => + builder.AddMiddleware(async (invocation, cancellationToken, next) => { var argsRemaining = invocation.ParseResult.UnmatchedTokens.ToArray(); var hostBuilder = hostBuilderFactory?.Invoke(argsRemaining) @@ -44,11 +44,11 @@ public static CommandLineBuilder UseHost(this CommandLineBuilder builder, invocation.BindingContext.AddService(typeof(IHost), _ => host); - await host.StartAsync(); + await host.StartAsync(cancellationToken); - await next(invocation); + await next(invocation, cancellationToken); - await host.StopAsync(); + await host.StopAsync(cancellationToken); }); public static CommandLineBuilder UseHost(this CommandLineBuilder builder, diff --git a/src/System.CommandLine.Hosting/InvocationLifetime.cs b/src/System.CommandLine.Hosting/InvocationLifetime.cs index 4201436594..7cd7477c43 100644 --- a/src/System.CommandLine.Hosting/InvocationLifetime.cs +++ b/src/System.CommandLine.Hosting/InvocationLifetime.cs @@ -1,7 +1,6 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. -using System.CommandLine.Invocation; using System.Threading; using System.Threading.Tasks; @@ -58,6 +57,8 @@ public Task WaitForStartAsync(CancellationToken cancellationToken) }, this); } + // The token comes from HostingExtensions.UseHost middleware + // and it's the invocation cancellation token. invokeCancelReg = cancellationToken.Register(state => { ((InvocationLifetime)state).OnInvocationCancelled(); diff --git a/src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs b/src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs index 157b69ae1d..c19f899f3f 100644 --- a/src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs +++ b/src/System.CommandLine.Tests/Invocation/InvocationPipelineTests.cs @@ -192,7 +192,7 @@ public async Task ParseResult_can_be_replaced_by_middleware() { command }) - .AddMiddleware(async (context, next) => + .AddMiddleware(async (context, token, next) => { var tokens = context.ParseResult .Tokens @@ -201,7 +201,7 @@ public async Task ParseResult_can_be_replaced_by_middleware() .ToArray(); context.ParseResult = context.Parser.Parse(tokens); - await next(context); + await next(context, token); }) .Build(); @@ -228,7 +228,7 @@ public async Task Invocation_can_be_short_circuited_by_middleware_by_not_calling { command }) - .AddMiddleware(async (_, _) => + .AddMiddleware(async (_, _, _) => { middlewareWasCalled = true; await Task.Yield(); @@ -259,7 +259,7 @@ public void Synchronous_invocation_can_be_short_circuited_by_async_middleware_by { command }) - .AddMiddleware(async (context, next) => + .AddMiddleware(async (context, cancellationToken, next) => { middlewareWasCalled = true; await Task.Yield(); @@ -346,9 +346,9 @@ public async Task Middleware_can_throw_OperationCancelledException() { command }) - .AddMiddleware((context, next) => + .AddMiddleware((context, cancellationToken, next) => { - throw new OperationCanceledException(); + throw new OperationCanceledException(cancellationToken); }) .UseExceptionHandler((ex, ctx) => ctx.ExitCode = ex is OperationCanceledException ? 123 : 456) .Build(); diff --git a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs index 811706ad96..20f09e18d7 100644 --- a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs +++ b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs @@ -95,7 +95,7 @@ public static CommandLineBuilder EnablePosixBundling( public static CommandLineBuilder RegisterWithDotnetSuggest( this CommandLineBuilder builder) { - builder.AddMiddleware(async (context, next) => + builder.AddMiddleware(async (context, cancellationToken, next) => { var feature = new FeatureRegistration("dotnet-suggest-registration"); @@ -115,7 +115,7 @@ await feature.EnsureRegistered(async () => stdOut: value => stdOut.Append(value), stdErr: value => stdOut.Append(value)); - await dotnetSuggestProcess.CompleteAsync(); + await dotnetSuggestProcess.CompleteAsync(cancellationToken); return $@"{dotnetSuggestProcess.StartInfo.FileName} exited with code {dotnetSuggestProcess.ExitCode} OUT: @@ -135,7 +135,7 @@ await feature.EnsureRegistered(async () => } }); - await next(context); + await next(context, cancellationToken); }, MiddlewareOrderInternal.RegisterWithDotnetSuggest); return builder; @@ -339,10 +339,10 @@ public static CommandLineBuilder AddMiddleware( Action onInvoke, MiddlewareOrder order = MiddlewareOrder.Default) { - builder.AddMiddleware(async (context, next) => + builder.AddMiddleware(async (context, cancellationToken, next) => { onInvoke(context); - await next(context); + await next(context, cancellationToken); }, order); return builder; diff --git a/src/System.CommandLine/Invocation/InvocationMiddleware.cs b/src/System.CommandLine/Invocation/InvocationMiddleware.cs index 7ae994427f..f1b3a92ccf 100644 --- a/src/System.CommandLine/Invocation/InvocationMiddleware.cs +++ b/src/System.CommandLine/Invocation/InvocationMiddleware.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using System.Threading; using System.Threading.Tasks; namespace System.CommandLine.Invocation @@ -9,8 +10,10 @@ namespace System.CommandLine.Invocation /// A delegate used for adding command handler invocation middleware. /// /// The context for the current invocation, which will be passed to each middleware and then to the command handler, unless a middleware short circuits it. + /// A token that can be used to cancel the invocation. /// A continuation. Passing the incoming to it will execute the next middleware in the pipeline and, at the end of the pipeline, the command handler. Middleware can short circuit the invocation by not calling this continuation. public delegate Task InvocationMiddleware( InvocationContext context, - Func next); + CancellationToken cancellationToken, + Func next); } diff --git a/src/System.CommandLine/Invocation/InvocationPipeline.cs b/src/System.CommandLine/Invocation/InvocationPipeline.cs index e98648ed1c..621e93f6f4 100644 --- a/src/System.CommandLine/Invocation/InvocationPipeline.cs +++ b/src/System.CommandLine/Invocation/InvocationPipeline.cs @@ -51,9 +51,9 @@ internal static async Task InvokeAsync(ParseResult parseResult, IConsole? c static async Task InvokeHandlerWithMiddleware(InvocationContext context, CancellationToken token) { - InvocationMiddleware invocationChain = BuildInvocationChain(context, token, true); + InvocationMiddleware invocationChain = BuildInvocationChain(context, true); - await invocationChain(context, _ => Task.CompletedTask); + await invocationChain(context, token, (_, _) => Task.CompletedTask); return GetExitCode(context); } @@ -80,20 +80,20 @@ internal static int Invoke(ParseResult parseResult, IConsole? console = null) static int InvokeHandlerWithMiddleware(InvocationContext context) { - InvocationMiddleware invocationChain = BuildInvocationChain(context, CancellationToken.None, false); + InvocationMiddleware invocationChain = BuildInvocationChain(context, false); - invocationChain(context, static _ => Task.CompletedTask).ConfigureAwait(false).GetAwaiter().GetResult(); + invocationChain(context, CancellationToken.None, static (_, _) => Task.CompletedTask).ConfigureAwait(false).GetAwaiter().GetResult(); return GetExitCode(context); } } - private static InvocationMiddleware BuildInvocationChain(InvocationContext context, CancellationToken cancellationToken, bool invokeAsync) + private static InvocationMiddleware BuildInvocationChain(InvocationContext context, bool invokeAsync) { var invocations = new List(context.Parser.Configuration.Middleware.Count + 1); invocations.AddRange(context.Parser.Configuration.Middleware); - invocations.Add(async (invocationContext, _) => + invocations.Add(async (invocationContext, cancellationToken, _) => { if (invocationContext.ParseResult.Handler is { } handler) { @@ -105,9 +105,9 @@ private static InvocationMiddleware BuildInvocationChain(InvocationContext conte return invocations.Aggregate( (first, second) => - (ctx, next) => - first(ctx, - c => second(c, next))); + (ctx, token, next) => + first(ctx, token, + (c, t) => second(c, t, next))); } private static int GetExitCode(InvocationContext context) From 510b4cd9205bfd5118558a00d67613678e106426 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Sat, 18 Feb 2023 20:48:32 +0100 Subject: [PATCH 19/19] Apply suggestions from code review Co-authored-by: Kevin B --- src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs | 2 +- src/System.CommandLine/Invocation/InvocationContext.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs index 20f09e18d7..9d6a3c9821 100644 --- a/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs +++ b/src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs @@ -26,7 +26,7 @@ public static class CommandLineBuilderExtensions /// A command line builder. /// /// Optional timeout for the command to process the exit cancellation. - /// If not passed , a default timeout of 2 seconds is enforced. + /// If not passed, a default timeout of 2 seconds is enforced. /// If positive value is passed - command is forcefully terminated after the timeout with exit code 130 (as if was not called). /// Host enforced timeout for ProcessExit event cannot be extended - default is 2 seconds: https://docs.microsoft.com/en-us/dotnet/api/system.appdomain.processexit?view=net-6.0. /// diff --git a/src/System.CommandLine/Invocation/InvocationContext.cs b/src/System.CommandLine/Invocation/InvocationContext.cs index 49bdfa953e..89c187d307 100644 --- a/src/System.CommandLine/Invocation/InvocationContext.cs +++ b/src/System.CommandLine/Invocation/InvocationContext.cs @@ -35,7 +35,7 @@ public InvocationContext(ParseResult parseResult, IConsole? console = null) /// public IConsole Console { - get =>_console ??= new SystemConsole(); + get => _console ??= new SystemConsole(); set => _console = value; }