Skip to content

Commit

Permalink
Console.Unix: fix, make CancelKeyPress work when input is redirected. (…
Browse files Browse the repository at this point in the history 
…#52891)

* Console.Unix: fix, make SIGINT work when input is redirected.

* Fix misplaced s_initialized assignment
  • Loading branch information
@tmds
tmds authored May 28, 2021
1 parent cb1f5ad commit ff5b9e0
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 33 deletions.
47 changes: 22 additions & 25 deletions src/libraries/System.Console/src/System/ConsolePal.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -919,26 +919,16 @@ internal static void EnsureConsoleInitialized()
/// <summary>Ensures that the console has been initialized for use.</summary>
private static unsafe void EnsureInitializedCore()
{
// Initialization is only needed when input isn't redirected.
if (Console.IsInputRedirected)
{
s_initialized = true;
return;
}

lock (Console.Out) // ensure that writing the ANSI string and setting initialized to true are done atomically
{
if (!s_initialized)
{
// Do this even when redirected to make CancelKeyPress works.
if (!Interop.Sys.InitializeTerminalAndSignalHandling())
{
throw new Win32Exception();
}

// Register a callback for signals that may invalidate our cached terminal settings.
// This includes: SIGCONT, SIGCHLD, SIGWINCH.
Interop.Sys.SetTerminalInvalidationHandler(&InvalidateTerminalSettings);

// Provide the native lib with the correct code from the terminfo to transition us into
// "application mode". This will both transition it immediately, as well as allow
// the native lib later to handle signals that require re-entering the mode.
Expand All @@ -951,20 +941,27 @@ private static unsafe void EnsureInitializedCore()
}
}

// Load special control character codes used for input processing
var controlCharacterNames = new Interop.Sys.ControlCharacterNames[4]
if (!Console.IsInputRedirected)
{
Interop.Sys.ControlCharacterNames.VERASE,
Interop.Sys.ControlCharacterNames.VEOL,
Interop.Sys.ControlCharacterNames.VEOL2,
Interop.Sys.ControlCharacterNames.VEOF
};
var controlCharacterValues = new byte[controlCharacterNames.Length];
Interop.Sys.GetControlCharacters(controlCharacterNames, controlCharacterValues, controlCharacterNames.Length, out s_posixDisableValue);
s_veraseCharacter = controlCharacterValues[0];
s_veolCharacter = controlCharacterValues[1];
s_veol2Character = controlCharacterValues[2];
s_veofCharacter = controlCharacterValues[3];
// Register a callback for signals that may invalidate our cached terminal settings.
// This includes: SIGCONT, SIGCHLD, SIGWINCH.
Interop.Sys.SetTerminalInvalidationHandler(&InvalidateTerminalSettings);

// Load special control character codes used for input processing
var controlCharacterNames = new Interop.Sys.ControlCharacterNames[4]
{
Interop.Sys.ControlCharacterNames.VERASE,
Interop.Sys.ControlCharacterNames.VEOL,
Interop.Sys.ControlCharacterNames.VEOL2,
Interop.Sys.ControlCharacterNames.VEOF
};
var controlCharacterValues = new byte[controlCharacterNames.Length];
Interop.Sys.GetControlCharacters(controlCharacterNames, controlCharacterValues, controlCharacterNames.Length, out s_posixDisableValue);
s_veraseCharacter = controlCharacterValues[0];
s_veolCharacter = controlCharacterValues[1];
s_veol2Character = controlCharacterValues[2];
s_veofCharacter = controlCharacterValues[3];
}

// Mark us as initialized
s_initialized = true;
Expand Down Expand Up @@ -1451,7 +1448,7 @@ internal sealed class ControlCHandlerRegistrar

internal unsafe void Register()
{
EnsureConsoleInitialized();
Debug.Assert(s_initialized); // by CancelKeyPress add.

Debug.Assert(!_handlerRegistered);
Interop.Sys.RegisterForCtrl(&OnBreakEvent);
Expand Down
22 changes: 14 additions & 8 deletions src/libraries/System.Console/tests/CancelKeyPress.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,30 +12,34 @@

public partial class CancelKeyPressTests
{
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void HandlerInvokedForSigInt()
[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[InlineData(false)]
[InlineData(true)]
public void HandlerInvokedForSigInt(bool redirectStandardInput)
{
// .NET Core respects ignored disposition for SIGINT/SIGQUIT.
if (IsSignalIgnored(SIGINT))
{
return;
}

HandlerInvokedForSignal(SIGINT);
HandlerInvokedForSignal(SIGINT, redirectStandardInput);
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/38998", TestPlatforms.OSX)]
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
[SkipOnMono("Mono doesn't call CancelKeyPress for SIGQUIT.")]
public void HandlerInvokedForSigQuit()
[InlineData(false)]
[InlineData(true)]
public void HandlerInvokedForSigQuit(bool redirectStandardInput)
{
// .NET Core respects ignored disposition for SIGINT/SIGQUIT.
if (IsSignalIgnored(SIGQUIT))
{
return;
}

HandlerInvokedForSignal(SIGQUIT);
HandlerInvokedForSignal(SIGQUIT, redirectStandardInput);
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
Expand Down Expand Up @@ -77,7 +81,7 @@ public void ExitDetectionNotBlockedByHandler()
}).Dispose();
}

private void HandlerInvokedForSignal(int signalOuter)
private void HandlerInvokedForSignal(int signalOuter, bool redirectStandardInput)
{
// On Windows we could use GenerateConsoleCtrlEvent to send a ctrl-C to the process,
// however that'll apply to all processes associated with the same group, which will
Expand All @@ -87,6 +91,8 @@ private void HandlerInvokedForSignal(int signalOuter)

// This test sends a SIGINT back to itself... if run in the xunit process, this would end
// up canceling the rest of xunit's tests. So we run the test itself in a separate process.
RemoteInvokeOptions options = new RemoteInvokeOptions();
options.StartInfo.RedirectStandardInput = redirectStandardInput;
RemoteExecutor.Invoke(signalStr =>
{
var tcs = new TaskCompletionSource<ConsoleSpecialKey>();
Expand All @@ -111,7 +117,7 @@ private void HandlerInvokedForSignal(int signalOuter)
{
Console.CancelKeyPress -= handler;
}
}, signalOuter.ToString()).Dispose();
}, signalOuter.ToString(), options).Dispose();
}

private unsafe static bool IsSignalIgnored(int signal)
Expand Down

3 comments on commit ff5b9e0

@jbaehr
Copy link

@jbaehr jbaehr commented on ff5b9e0 Jul 29, 2021

Choose a reason for hiding this comment

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

@stephentoub are there any planes to backport this (i.e. #52891) to the 5.0 branch? After closing #51221 5.0.7 and 5.0.8 have been release, but unfortunately they do not include the fix. Any chance for 5.0.9?

(sorry for commenting here, but the original issue as well as the PR are locked for comments now. Please let me know when there are preferred alternatives)

@stephentoub
Copy link
Member

Choose a reason for hiding this comment

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

are there any planes to backport this (i.e. #52891) to the 5.0 branch?

cc: @jeffhandley

@jeffhandley
Copy link
Member

Choose a reason for hiding this comment

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

@jbaehr If you have a need for the fix to be backported, could you file a new issue to express that need please? To justify backporting it, we would need to understand how the issue is affecting you against 5.0 and what your timeline would be for updating to 6.0 once it's released. Additionally, we would benefit from getting validation from you that this fix is proven to address the issue for you.

Thanks!

Please sign in to comment.