dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.75k stars 1.07k forks source link

Regression - ctrl-c handling commit from old dotnet/cli repo not merged into this repo #33417

Open paul-hammant opened 1 year ago

paul-hammant commented 1 year ago

https://github.com/dotnet/cli/commit/69abb9cafd1a924081236fe88321d88e5b7b7c0d is the commit from the past.

Command.cs no longer has the HandleKeyPress logic

image

I had a solution with 30 projects. My NuGet.Config wasn't listing all the feeds needed, so when I kicked off dotnet test in the command line, it would issue errors on being unable to source those DLLs. No matter, I can hit ctrl-c to stop that after I see it is going to not get DLLs.

For .NET 6, that works as expected - early exit of the dotnet test command. For .NET 7, if stops on the current .csproj project, but then immediately goes to the next project. I have to super mash the the ctrl-c key to get the outcome I want, and even then I've barely saved any time cos it wanted to keep going.

KalleOlaviNiemitalo commented 1 year ago

The Console.CancelKeyPress event handling was moved to to class ProcessReaper in https://github.com/dotnet/cli/pull/10720 commit d1c28f998b0b756fdd9ac35bb47491f89324b3c1 on 2019-02-06. Anyway, the HandleCancelKeyPress method does not attempt to terminate the dotnet process or any child processes; it just sets Cancel = true in the event args so that the Ctrl+C won't automatically terminate the dotnet process. I think the intention is that the Ctrl+C terminates only child processes, and the dotnet process then notices that its child processes have terminated and exits.

If there has been a regression between .NET 6 and .NET 7, it might be caused by some changes in System.CommandLine, which was used already in .NET SDK 6.0.100. System.CommandLine can add a Console.CancelKeyPress handler that marks the event as handled (sets Cancel = true), signals a CancellationTokenSource, and waits until the command-line action ends or the ProcessTerminationTimeout expires (default 2 seconds, and .NET SDK does not change it). If dotnet test runs MSBuild in-process and MSBuild adds its own Console.CancelKeyPress handler, then I think this latter handler won't be called before ProcessTerminationTimeout expires, so MSBuild won't know that it needs to stop.

If you set DOTNET_CLI_RUN_MSBUILD_OUTOFPROC=1 in the environment, does Ctrl+C work better then? That should cause dotnet to run MSBuild as a child process, so that MSBuild gets the Console.CancelKeyPress event immediately even if an event handler in the dotnet process is blocking. If that helps, I think System.CommandLine should be changed not to wait in the Console.CancelKeyPress event handler.

Cc: @adamsitnik and @jonsequitur, because of cancellation discussion in https://github.com/dotnet/command-line-api/pull/2205.

paul-hammant commented 1 year ago

Super thorough reply, Kalle.

marcpopMSFT commented 1 year ago

I think msbuild does a graceful cancellation rather than an immediate end of process cancellation. I wonder if there is documentation we have on this to outline this behavior. Ultimately, I think the SDK just passes along the request to cancel to MSBuild.

CC @rainersigwald @donJoseLuis

paul-hammant commented 1 year ago

Would it help if I made a cloneable .sln and a bunch .csproj's to show the ctrl-c only affecting the current proj and the solution-level dispatcher still recursing into other projects?

marcpopMSFT commented 1 year ago

CC @dotnet/kitten for clarification on msbuild behavior.

rainersigwald commented 1 year ago

Would it help if I made a cloneable .sln and a bunch .csproj's to show the ctrl-c only affecting the current proj and the solution-level dispatcher still recursing into other projects?

Yes, I think so--I don't see this on my attempt.

marcpopMSFT commented 1 year ago

Seems likely to be the linked bug. Not closing so MSBuild can confirm when they investigate that bug.

paul-hammant commented 1 year ago

I can reproduce in my workplace with dotnet restore alone. Key is a real nuget-feed URL that the user in question is not currently authenticated against.

I had thought that an Edge/Chrome interactive popup to SSO log in to the feed was automatic, but it seems not for me, hence I can experience the ctrl-c not handled thing of this issue.