Support Ctrl+C handling #290

tmds · 2018-11-05T13:12:30Z

An initial attempt at implementing #259.

This adds a Canceled property and Cancel method to the InvocationContext.
This is similar to the RequestAborted and Abort methods on HttpContext.

I'm doing an early PR to get some feedback.

It won't compile on my system, running ./build.sh gives me errors:

/home/tmds/repos/System.CommandLine/artifacts/Debug/obj/System.CommandLine/netstandard2.0/System.CommandLine.AssemblyInfo.cs(10,12): error CS0579: Duplicate 'System.Reflection.AssemblyCompanyAttribute' attribute [/home/tmds/repos/System.CommandLine/src/System.CommandLine/System.CommandLine.csproj]
/home/tmds/repos/System.CommandLine/artifacts/Debug/obj/System.CommandLine/netstandard2.0/System.CommandLine.AssemblyInfo.cs(11,12): error CS0579: Duplicate 'System.Reflection.AssemblyConfigurationAttribute' attribute [/home/tmds/repos/System.CommandLine/src/System.CommandLine/System.CommandLine.csproj]
/home/tmds/repos/System.CommandLine/artifacts/Debug/obj/System.CommandLine/netstandard2.0/System.CommandLine.AssemblyInfo.cs(13,12): error CS0579: Duplicate 'System.Reflection.AssemblyFileVersionAttribute' attribute [/home/tmds/repos/System.CommandLine/src/System.CommandLine/System.CommandLine.csproj]
/home/tmds/repos/System.CommandLine/artifacts/Debug/obj/System.CommandLine/netstandard2.0/System.CommandLine.AssemblyInfo.cs(14,12): error CS0579: Duplicate 'System.Reflection.AssemblyInformationalVersionAttribute' attribute [/home/tmds/repos/System.CommandLine/src/System.CommandLine/System.CommandLine.csproj]
/home/tmds/repos/System.CommandLine/artifacts/Debug/obj/System.CommandLine/netstandard2.0/System.CommandLine.AssemblyInfo.cs(15,12): error CS0579: Duplicate 'System.Reflection.AssemblyProductAttribute' attribute [/home/tmds/repos/System.CommandLine/src/System.CommandLine/System.CommandLine.csproj]
/home/tmds/repos/System.CommandLine/artifacts/Debug/obj/System.CommandLine/netstandard2.0/System.CommandLine.AssemblyInfo.cs(16,12): error CS0579: Duplicate 'System.Reflection.AssemblyTitleAttribute' attribute [/home/tmds/repos/System.CommandLine/src/System.CommandLine/System.CommandLine.csproj]
/home/tmds/repos/System.CommandLine/artifacts/Debug/obj/System.CommandLine/netstandard2.0/System.CommandLine.AssemblyInfo.cs(17,12): error CS0579: Duplicate 'System.Reflection.AssemblyVersionAttribute' attribute [/home/tmds/repos/System.CommandLine/src/System.CommandLine/System.CommandLine.csproj]

tmds · 2018-11-13T09:04:25Z

I reimplemented this using middleware and added a middleware for AssemblyLoadContext.Default.Unloading (SIGTERM) as well. It looks a lot cleaner now.

tmds · 2018-11-13T09:06:09Z

I'll look at adding some tests when we settled on a public API.

jonsequitur · 2018-11-13T14:50:08Z

Tests will be important for helping understand how this is intended to be used and to make sure it's not regressed during refactoring. This is particularly important because we still plan to decompose IConsole.

src/System.CommandLine/IConsole.cs

src/System.CommandLine/Invocation/InvocationExtensions.cs

src/System.CommandLine/Invocation/CommandHandler.cs

src/System.CommandLine/IConsole.cs

src/System.CommandLine.Tests/CancelOnCancelKeyTests.cs

src/System.CommandLine/Invocation/InvocationContext.cs

src/System.CommandLine.Tests/InvocationPipelineTests.cs

src/System.CommandLine/Invocation/InvocationExtensions.cs

…on with UseExceptionHandler

src/System.CommandLine/Invocation/InvocationExtensions.cs

…ncelled

…ndler. The user should catch this exception.

src/System.CommandLine/Invocation/MethodBindingCommandHandler.cs

src/System.CommandLine/Invocation/InvocationContext.cs

src/System.CommandLine.Tests/CancelOnProcessTerminationTests.cs

tmds · 2018-11-28T13:55:32Z

The lock in InvocationContext was trying to solve a race condition that occurs when AddCancellationHandling is called after Cancel. In that case, there was both a non-cancellable (isCancelling returned false) and a cancelled invocation (invocation with cancelled CancellationToken) happening.
This race is now fixed. Calling AddCancellationHandling is the point where the invocation becomes cancellable.

src/System.CommandLine.Tests/CancelOnProcessTerminationTests.cs

src/System.CommandLine/Invocation/InvocationContext.cs

… AddCancellationHandling method

jonsequitur · 2018-11-29T17:53:47Z

src/System.CommandLine.Tests/RemoteExecution.cs

+            }
+        }
+
+        private sealed class RemoteExecutionException : XunitException


I don't think the base exception type needs to be Xunit-specific.

tmds · 2018-12-04T16:00:55Z

Ready to merge!

tmds · 2018-12-04T16:19:47Z

What do you want to do with wiki_feature_description.md? Shall I remove it from the PR?

jonsequitur · 2018-12-04T16:36:39Z

If you'd like to remove it from the PR and push it to the wiki, feel free. I'll figure out where to link to it.

tmds · 2018-12-05T07:52:19Z

I've copied it here: https://github.com/dotnet/System.CommandLine/wiki/Process-termination-handling
You can merge the PR now.

tmds · 2018-12-05T16:07:46Z

🎉

Support Ctrl+C handling

2d7843d

tmds requested a review from jonsequitur November 5, 2018 13:12

tmds added 5 commits November 13, 2018 09:22

Reimplement using middleware

235bf17

Add CancelOnUnload

bf6528f

Undo changes to InvocationPipeline

1076155

Undo changes to InvocationContext

0da0b47

Update MiddlewareOrder

5180d09

jonsequitur reviewed Nov 13, 2018

View reviewed changes

src/System.CommandLine/IConsole.cs Outdated Show resolved Hide resolved

Keboo reviewed Nov 13, 2018

View reviewed changes

src/System.CommandLine/IConsole.cs Outdated Show resolved Hide resolved

src/System.CommandLine/Invocation/InvocationExtensions.cs Outdated Show resolved Hide resolved

src/System.CommandLine/Invocation/InvocationExtensions.cs Outdated Show resolved Hide resolved

Add CancelOnCancelKeyTests

3866b9e

tmds commented Nov 13, 2018

View reviewed changes

src/System.CommandLine/Invocation/CommandHandler.cs Show resolved Hide resolved

tmds commented Nov 13, 2018

View reviewed changes

src/System.CommandLine/IConsole.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Nov 13, 2018

View reviewed changes

src/System.CommandLine.Tests/CancelOnCancelKeyTests.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Nov 14, 2018

View reviewed changes

src/System.CommandLine/Invocation/InvocationContext.cs Outdated Show resolved Hide resolved

Make InvocationContext aware if cancellation is enabled

b7930c7

jonsequitur reviewed Nov 14, 2018

View reviewed changes

src/System.CommandLine.Tests/InvocationPipelineTests.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Nov 14, 2018

View reviewed changes

src/System.CommandLine/Invocation/InvocationExtensions.cs Outdated Show resolved Hide resolved

tmds added 5 commits November 15, 2018 09:17

Merge CancelOn* methods into UseConsoleLifetime and improve interacti…

7374992

…on with UseExceptionHandler

Merge remote-tracking branch 'upstream/master' into cancel_key

d53b5ec

Unwrap InvokeAsync TargetInvocationException

1fcab8b

Bind CancellationToken

256fb39

Undo changes to SystemConsole

48a9af8

tmds commented Nov 15, 2018

View reviewed changes

src/System.CommandLine/Invocation/InvocationExtensions.cs Outdated Show resolved Hide resolved

tmds added 2 commits November 15, 2018 10:05

UseExceptionHandler: also print a message when the invocation gets ca…

952e314

…ncelled

Undo special handling of OperationCanceledException in UseExceptionHa…

d903c2f

…ndler. The user should catch this exception.

jonsequitur reviewed Nov 15, 2018

View reviewed changes

src/System.CommandLine/Invocation/MethodBindingCommandHandler.cs Outdated Show resolved Hide resolved

Add Linux test for UseConsoleLifetime

e5e8c32

tmds commented Nov 27, 2018

View reviewed changes

src/System.CommandLine/Invocation/InvocationContext.cs Outdated Show resolved Hide resolved

tmds commented Nov 27, 2018

View reviewed changes

src/System.CommandLine/Invocation/InvocationContext.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Nov 27, 2018

View reviewed changes

src/System.CommandLine/Invocation/InvocationContext.cs Outdated Show resolved Hide resolved

tmds commented Nov 27, 2018

View reviewed changes

src/System.CommandLine/Invocation/InvocationContext.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Nov 27, 2018

View reviewed changes

src/System.CommandLine.Tests/CancelOnProcessTerminationTests.cs Show resolved Hide resolved

tmds added 4 commits November 28, 2018 11:09

Reword wiki feature description

963c30d

Move child process timeouts to the test process

87d57ec

Simplify tests

c5fe41b

Fix race between producers and consumers of cancellation

2757674

tmds mentioned this pull request Nov 28, 2018

Add Windows tests for CancelOnProcessTermination #328

Open

jonsequitur reviewed Nov 28, 2018

View reviewed changes

src/System.CommandLine.Tests/CancelOnProcessTerminationTests.cs Outdated Show resolved Hide resolved

tmds commented Nov 28, 2018

View reviewed changes

src/System.CommandLine/Invocation/InvocationContext.cs Show resolved Hide resolved

tmds added 5 commits November 29, 2018 09:21

Merge remote-tracking branch 'upstream/master' into cancel_key

1f6208d

Tests: help compiler pick a CommandHandler.Create overload

e0115d0

InvocationContext: ensure order of CancellationHandlerAdded event and…

840f267

… AddCancellationHandling method

Use FluentAssertion style

610ca21

Fix typo

6aa5bd1

jonsequitur reviewed Nov 29, 2018

View reviewed changes

Make RemoteExceptionException directly derive from Exception

269bb1e

jonsequitur approved these changes Nov 30, 2018

View reviewed changes

UseDefaults: include CancelOnProcessTermination

a03dcdb

Remove wiki feature description

6116ef9

jonsequitur merged commit aa631e3 into dotnet:master Dec 5, 2018

tmds mentioned this pull request Apr 6, 2019

suggestion: support Ctrl+C handling natemcmaster/CommandLineUtils#111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Ctrl+C handling #290

Support Ctrl+C handling #290

tmds commented Nov 5, 2018

tmds commented Nov 13, 2018

tmds commented Nov 13, 2018

jonsequitur commented Nov 13, 2018

tmds commented Nov 28, 2018

jonsequitur Nov 29, 2018

tmds commented Dec 4, 2018

tmds commented Dec 4, 2018

jonsequitur commented Dec 4, 2018

tmds commented Dec 5, 2018

tmds commented Dec 5, 2018

Support Ctrl+C handling #290

Support Ctrl+C handling #290

Conversation

tmds commented Nov 5, 2018

tmds commented Nov 13, 2018

tmds commented Nov 13, 2018

jonsequitur commented Nov 13, 2018

tmds commented Nov 28, 2018

jonsequitur Nov 29, 2018

Choose a reason for hiding this comment

tmds commented Dec 4, 2018

tmds commented Dec 4, 2018

jonsequitur commented Dec 4, 2018

tmds commented Dec 5, 2018

tmds commented Dec 5, 2018