From b47305f3b494408c2c6fc712d5873386c9421c1f Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 27 Feb 2019 10:46:26 +0100 Subject: [PATCH 01/14] Console: toggle terminal echo based on presence of interactive child processes .NET applications echo characters during a Console.Read. By default, Unix terminals echo characters as the user is typing them. When a .NET Core application launches an interactive application (e.g. 'vi') that application expects to find the terminal in an echoing state. To make both work, corefx was disabling echo during Console.Read operations, and turning it back on when the read is done. This changes to echoing while there are interactive applications and not echoing when there are none. This means we no longer need to toggle echo off/on for each Console.Read. And the terminal will no longer echo when there is no Console.Read going on. --- ...rop.ConfigureConsoleForInteractiveChild.cs | 15 ++ ...rop.InitializeConsoleAndSignalHandling.cs} | 4 +- .../Interop.ReadStdinUnbuffered.cs | 7 +- .../Interop.RegisterForSigChld.cs | 2 +- src/Native/Unix/System.Native/pal_console.c | 252 +++++++++++------- src/Native/Unix/System.Native/pal_console.h | 10 +- src/Native/Unix/System.Native/pal_signal.c | 64 ++--- src/Native/Unix/System.Native/pal_signal.h | 9 +- src/System.Console/src/System.Console.csproj | 4 +- .../src/System/ConsolePal.Unix.cs | 9 +- .../src/System/IO/StdInReader.cs | 62 ++--- .../src/System.Diagnostics.Process.csproj | 6 + .../src/System/Diagnostics/Process.Unix.cs | 82 ++++-- .../Diagnostics/ProcessWaitState.Unix.cs | 31 ++- 14 files changed, 319 insertions(+), 238 deletions(-) create mode 100644 src/Common/src/Interop/Unix/System.Native/Interop.ConfigureConsoleForInteractiveChild.cs rename src/Common/src/Interop/Unix/System.Native/{Interop.InitializeConsole.cs => Interop.InitializeConsoleAndSignalHandling.cs} (80%) diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.ConfigureConsoleForInteractiveChild.cs b/src/Common/src/Interop/Unix/System.Native/Interop.ConfigureConsoleForInteractiveChild.cs new file mode 100644 index 000000000000..cfbd3b00ef65 --- /dev/null +++ b/src/Common/src/Interop/Unix/System.Native/Interop.ConfigureConsoleForInteractiveChild.cs @@ -0,0 +1,15 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.InteropServices; + +internal static partial class Interop +{ + internal static partial class Sys + { + [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ConfigureConsoleForInteractiveChild")] + internal static extern unsafe void ConfigureConsoleForInteractiveChild(bool enable); + } +} diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.InitializeConsole.cs b/src/Common/src/Interop/Unix/System.Native/Interop.InitializeConsoleAndSignalHandling.cs similarity index 80% rename from src/Common/src/Interop/Unix/System.Native/Interop.InitializeConsole.cs rename to src/Common/src/Interop/Unix/System.Native/Interop.InitializeConsoleAndSignalHandling.cs index aeb1b69bfb9e..780c2b0f3aa7 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.InitializeConsole.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.InitializeConsoleAndSignalHandling.cs @@ -8,8 +8,8 @@ internal static partial class Interop { internal static partial class Sys { - [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_InitializeConsole", SetLastError = true)] - internal static extern bool InitializeConsole(); + [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_InitializeConsoleAndSignalHandling", SetLastError = true)] + internal static extern bool InitializeConsoleAndSignalHandling(); [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_SetKeypadXmit")] internal static extern void SetKeypadXmit(string terminfoString); diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs b/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs index 1b3b0ac7ea51..f8b8cb00c642 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs @@ -12,10 +12,7 @@ internal static partial class Sys [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ReadStdin", SetLastError = true)] internal static extern unsafe int ReadStdin(byte* buffer, int bufferSize); - [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_InitializeConsoleBeforeRead")] - internal static extern unsafe void InitializeConsoleBeforeRead(byte minChars = 1, byte decisecondsTimeout = 0); - - [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_UninitializeConsoleAfterRead")] - internal static extern unsafe void UninitializeConsoleAfterRead(); + [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ConfigureConsoleTimeout")] + internal static extern unsafe void ConfigureConsoleTimeout(byte minChars = 1, byte decisecondsTimeout = 0); } } diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.RegisterForSigChld.cs b/src/Common/src/Interop/Unix/System.Native/Interop.RegisterForSigChld.cs index f0d428d45c5b..9c374cee10e9 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.RegisterForSigChld.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.RegisterForSigChld.cs @@ -11,6 +11,6 @@ internal partial class Sys internal delegate void SigChldCallback(bool reapAll); [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_RegisterForSigChld")] - internal static extern bool RegisterForSigChld(SigChldCallback handler); + internal static extern void RegisterForSigChld(SigChldCallback handler); } } diff --git a/src/Native/Unix/System.Native/pal_console.c b/src/Native/Unix/System.Native/pal_console.c index afbbd8eeb1be..c099bc9dce42 100644 --- a/src/Native/Unix/System.Native/pal_console.c +++ b/src/Native/Unix/System.Native/pal_console.c @@ -17,6 +17,7 @@ #include #include #include +#include int32_t SystemNative_GetWindowSize(WinSize* windowSize) { @@ -44,7 +45,7 @@ int32_t SystemNative_IsATty(intptr_t fd) static char* g_keypadXmit = NULL; // string used to enable application mode, from terminfo -static void WriteKeypadXmit() // used in a signal handler, must be signal-safe +static void WriteKeypadXmit() { // If a terminfo "application mode" keypad_xmit string has been supplied, // write it out to the terminal to enter the mode. @@ -71,92 +72,123 @@ void SystemNative_SetKeypadXmit(const char* terminfoString) WriteKeypadXmit(); } -static bool g_readInProgress = false; // tracks whether a read is currently in progress, such that attributes have been changed -static bool g_signalForBreak = true; // tracks whether the terminal should send signals for breaks, such that attributes have been changed -static bool g_haveInitTermios = false; // whether g_initTermios has been initialized -static struct termios g_initTermios; // the initial attributes captured when Console was initialized -static struct termios g_preReadTermios; // the original attributes captured before a read; valid if g_readInProgress is true -static struct termios g_currTermios; // the current attributes set during a read; valid if g_readInProgress is true +static bool g_signalForBreak = true; // tracks whether the terminal should send signals for breaks, such that attributes have been changed +static bool g_haveInitTermios = false; // whether g_initTermios has been initialized +static bool g_hasInteractiveChildren = false; // tracks whether the application has interactive child processes. +static struct termios g_initTermios; // the initial attributes captured when Console was initialized +static bool g_consoleUninitialized = false; // tracks whether the application is terminating +static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER; -void UninitializeConsole() +static void ApplyTerminalSettings(struct termios* termios, bool signalForBreak, bool hasInteractiveChildren) { - // pal_signal.cpp calls this on SIGQUIT/SIGINT. - // This can happen when SystemNative_InitializeConsole was not called. - - // Put the attributes back to what they were when the console was initially initialized. - // We only do so, however, if we have explicitly modified the termios; doing so always - // can result in problems if the app is in the background, as then attempting to call - // tcsetattr on STDIN_FILENO will suspend the app and prevent its shutdown. We also don't - // want to, for example, just compare g_currTermios with g_initTermios, as we'd then be - // factoring in changes made by other apps or by user code. - if (g_haveInitTermios && // we successfully initialized the console - (g_readInProgress || !g_signalForBreak)) // we modified attributes + if (signalForBreak) + termios->c_lflag |= (uint32_t)ISIG; + else + termios->c_lflag &= (uint32_t)(~ISIG); + + if (hasInteractiveChildren) { - tcsetattr(STDIN_FILENO, TCSANOW, &g_initTermios); - // ignore any failure + assert(g_haveInitTermios); + termios->c_iflag = g_initTermios.c_iflag; + termios->c_lflag = g_initTermios.c_lflag; + } + else + { + termios->c_iflag &= (uint32_t)(~(IXON | IXOFF)); + termios->c_lflag &= (uint32_t)(~(ECHO | ICANON | IEXTEN)); } } -static void IncorporateBreak(struct termios *termios, int32_t signalForBreak) +static bool TcGetAttr(struct termios* termios, bool signalForBreak, bool hasInteractiveChildren) { - assert(termios != NULL); - assert(signalForBreak == 0 || signalForBreak == 1); + bool rv = tcgetattr(STDIN_FILENO, termios) >= 0; + if (rv) + { + ApplyTerminalSettings(termios, signalForBreak, hasInteractiveChildren); + } + return rv; +} - if (signalForBreak) - termios->c_lflag |= (uint32_t)ISIG; - else - termios->c_lflag &= (uint32_t)(~ISIG); +static bool TcSetAttr(struct termios* t) +{ + // This method is called with lock g_lock held. + + if (g_consoleUninitialized) + { + // The application is exiting, we mustn't change terminal settings. + return true; + } + + return tcsetattr(STDIN_FILENO, TCSANOW, t) >= 0; } -// In order to support Console.ReadKey(intercept: true), we need to disable echo and canonical mode. -// We have two main choices: do so for the entire app, or do so only while in the Console.ReadKey(true). -// The former has a huge downside: the terminal is in a non-echo state, so anything else that runs -// in the same terminal won't echo even if it expects to, e.g. using Process.Start to launch an interactive, -// program, or P/Invoking to a native library that reads from stdin rather than using Console. The second -// also has a downside, in that any typing which occurs prior to invoking Console.ReadKey(true) will -// be visible even though it wasn't supposed to be. The downsides of the former approach are so large -// and the cons of the latter minimal and constrained to the one API that we've chosen the second approach. -// Thus, InitializeConsoleBeforeRead is called to set up the state of the console, then a read is done, -// and then UninitializeConsoleAfterRead is called. -void SystemNative_InitializeConsoleBeforeRead(uint8_t minChars, uint8_t decisecondsTimeout) +void UninitializeConsole() { - struct termios newTermios; - if (tcgetattr(STDIN_FILENO, &newTermios) >= 0) + // This method is called on SIGQUIT/SIGINT from the signal dispatching thread + // and on atexit. + + if (pthread_mutex_lock(&g_lock) == 0) { - if (!g_readInProgress) + if (g_consoleUninitialized) + { + return; + } + + if (g_haveInitTermios) { - // Store the original settings, but only if we didn't already. This function - // may be called when the process is resumed after being suspended, and if - // that happens during a read, we'll call this function to reset the attrs. - g_preReadTermios = newTermios; + // TODO: https://github.com/dotnet/cli/issues/6216 + TcSetAttr(&g_initTermios); } - newTermios.c_iflag &= (uint32_t)(~(IXON | IXOFF)); - newTermios.c_lflag &= (uint32_t)(~(ECHO | ICANON | IEXTEN)); - newTermios.c_cc[VMIN] = minChars; - newTermios.c_cc[VTIME] = decisecondsTimeout; - IncorporateBreak(&newTermios, g_signalForBreak); + g_consoleUninitialized = true; - if (tcsetattr(STDIN_FILENO, TCSANOW, &newTermios) >= 0) + pthread_mutex_unlock(&g_lock); + } +} + +void SystemNative_ConfigureConsoleTimeout(uint8_t minChars, uint8_t decisecondsTimeout) +{ + if (pthread_mutex_lock(&g_lock) == 0) + { + struct termios termios; + if (TcGetAttr(&termios, g_signalForBreak, g_hasInteractiveChildren)) { - g_currTermios = newTermios; - g_readInProgress = true; + termios.c_cc[VMIN] = minChars; + termios.c_cc[VTIME] = decisecondsTimeout; + + TcSetAttr(&termios); } + pthread_mutex_unlock(&g_lock); } } -void SystemNative_UninitializeConsoleAfterRead() +void SystemNative_ConfigureConsoleForInteractiveChild(int32_t hasInteractiveChildren) { - if (g_readInProgress) + assert(hasInteractiveChildren == 0 || hasInteractiveChildren == 1); + + if (pthread_mutex_lock(&g_lock) == 0) { - g_readInProgress = false; - - int tmpErrno = errno; // preserve any errors from before uninitializing - IncorporateBreak(&g_preReadTermios, g_signalForBreak); - int ret = tcsetattr(STDIN_FILENO, TCSANOW, &g_preReadTermios); - assert(ret >= 0); // shouldn't fail, but if it does we don't want to fail in release - (void)ret; - errno = tmpErrno; + if (g_hasInteractiveChildren == hasInteractiveChildren) + { + return; + } + + struct termios termios; + if (TcGetAttr(&termios, g_signalForBreak, hasInteractiveChildren)) + { + if (TcSetAttr(&termios)) + { + g_hasInteractiveChildren = hasInteractiveChildren; + } + } + + // Redo "Application mode" when there are no more interactive children. + if (!hasInteractiveChildren) + { + WriteKeypadXmit(); + } + + pthread_mutex_unlock(&g_lock); } } @@ -257,10 +289,9 @@ void SystemNative_GetControlCharacters( int32_t SystemNative_StdinReady() { - SystemNative_InitializeConsoleBeforeRead(1, 0); + // TODO ??: https://github.com/dotnet/corefx/pull/6619 struct pollfd fd = { .fd = STDIN_FILENO, .events = POLLIN }; int rv = poll(&fd, 1, 0) > 0 ? 1 : 0; - SystemNative_UninitializeConsoleAfterRead(); return rv; } @@ -289,57 +320,76 @@ int32_t SystemNative_SetSignalForBreak(int32_t signalForBreak) { assert(signalForBreak == 0 || signalForBreak == 1); - struct termios current; - if (tcgetattr(STDIN_FILENO, ¤t) >= 0) + int rv = 0; + + if (pthread_mutex_lock(&g_lock) == 0) { - IncorporateBreak(¤t, signalForBreak); - if (tcsetattr(STDIN_FILENO, TCSANOW, ¤t) >= 0) + struct termios termios; + if (TcGetAttr(&termios, signalForBreak, g_hasInteractiveChildren)) { - g_signalForBreak = signalForBreak; - return 1; + if (TcSetAttr(&termios)) + { + g_signalForBreak = signalForBreak; + rv = 1; + } } + pthread_mutex_unlock(&g_lock); } - return 0; + return rv; } void ReinitializeConsole() { - // pal_signal.cpp calls this on SIGCONT/SIGCHLD. - // This can happen when SystemNative_InitializeConsole was not called. - // This gets called on a signal handler, we may only use async-signal-safe functions. - - // If the process was suspended while reading, we need to - // re-initialize the console for the read, as the attributes - // previously set were likely overwritten. - if (g_readInProgress) + // Restores the state of the console after being suspended. + // pal_signal.cpp calls this on SIGCONT from the signal handling thread. + + if (pthread_mutex_lock(&g_lock) == 0) { - IncorporateBreak(&g_currTermios, g_signalForBreak); - tcsetattr(STDIN_FILENO, TCSANOW, &g_currTermios); - } + struct termios termios; + if (TcGetAttr(&termios, g_signalForBreak, g_hasInteractiveChildren)) + { + TcSetAttr(&termios); + } - // "Application mode" will also have been reset and needs to be redone. - WriteKeypadXmit(); + // Redo "Application mode" unless there are interactive children. + // In that case, we'll redo it when there are no more interactive children. + if (!g_hasInteractiveChildren) + { + WriteKeypadXmit(); + } + + pthread_mutex_unlock(&g_lock); + } } -int32_t SystemNative_InitializeConsole() +int32_t SystemNative_InitializeConsoleAndSignalHandling() { - if (!InitializeSignalHandling()) - { - return 0; - } + static int32_t initialized = 0; - if (tcgetattr(STDIN_FILENO, &g_initTermios) >= 0) - { - g_haveInitTermios = true; - g_signalForBreak = (g_initTermios.c_lflag & ISIG) != 0; - } - else + // Both the Process and Console class call this method for initialization. + if (pthread_mutex_lock(&g_lock) == 0) { - g_haveInitTermios = false; - g_signalForBreak = true; + if (initialized == 0) + { + initialized = InitializeSignalHandlingCore(); + + if (initialized == 1) + { + g_haveInitTermios = tcgetattr(STDIN_FILENO, &g_initTermios) >= 0; + + if (g_haveInitTermios) + { + struct termios termios = g_initTermios; + ApplyTerminalSettings(&termios, g_signalForBreak, g_hasInteractiveChildren); + TcSetAttr(&termios); + } + + atexit(UninitializeConsole); + } + } + pthread_mutex_unlock(&g_lock); } - atexit(UninitializeConsole); - return 1; + return initialized; } diff --git a/src/Native/Unix/System.Native/pal_console.h b/src/Native/Unix/System.Native/pal_console.h index 83b172a846b9..52946acc0289 100644 --- a/src/Native/Unix/System.Native/pal_console.h +++ b/src/Native/Unix/System.Native/pal_console.h @@ -58,11 +58,11 @@ DLLEXPORT int32_t SystemNative_GetWindowSize(WinSize* windowsSize); DLLEXPORT int32_t SystemNative_IsATty(intptr_t fd); /** - * Initializes the console for use by System.Console. + * Initializes signal handling and terminal for use by System.Console and System.Process. * * Returns 1 on success; otherwise returns 0 and sets errno. */ -DLLEXPORT int32_t SystemNative_InitializeConsole(void); +DLLEXPORT int32_t SystemNative_InitializeConsoleAndSignalHandling(void); /** * Stores the string that can be written to stdout to transition @@ -92,12 +92,12 @@ DLLEXPORT int32_t SystemNative_StdinReady(void); /** * Initializes the terminal in preparation for a read operation. */ -DLLEXPORT void SystemNative_InitializeConsoleBeforeRead(uint8_t minChars, uint8_t decisecondsTimeout); +DLLEXPORT void SystemNative_ConfigureConsoleTimeout(uint8_t minChars, uint8_t decisecondsTimeout); /** - * Restores the terminal's attributes to what they were before InitializeConsoleBeforeRead was called. + * Initializes the terminal for running interactive applications. */ -DLLEXPORT void SystemNative_UninitializeConsoleAfterRead(void); +DLLEXPORT void SystemNative_ConfigureConsoleForInteractiveChild(int32_t enable); /** * Reads the number of bytes specified into the provided buffer from stdin. diff --git a/src/Native/Unix/System.Native/pal_signal.c b/src/Native/Unix/System.Native/pal_signal.c index 549e4b20a533..9390492e0d47 100644 --- a/src/Native/Unix/System.Native/pal_signal.c +++ b/src/Native/Unix/System.Native/pal_signal.c @@ -40,28 +40,15 @@ static struct sigaction* OrigActionFor(int sig) static void SignalHandler(int sig, siginfo_t* siginfo, void* context) { - if (sig == SIGCONT || sig == SIGCHLD) - { - // SIGCONT will be sent when we're resumed after suspension, at which point - // we need to set the terminal back up. Similarly, SIGCHLD will be sent after - // a child process completes, and that child could have left things in a bad state, - // so we similarly need to reinitialize. - ReinitializeConsole(); - } - // Signal handler for signals where we want our background thread to do the real processing. // It simply writes the signal code to a pipe that's read by the thread. - if (sig == SIGQUIT || sig == SIGINT || sig == SIGCHLD) - { - // Write the signal code to the pipe - uint8_t signalCodeByte = (uint8_t)sig; - ssize_t writtenBytes; - while ((writtenBytes = write(g_signalPipe[1], &signalCodeByte, 1)) < 0 && errno == EINTR); + uint8_t signalCodeByte = (uint8_t)sig; + ssize_t writtenBytes; + while ((writtenBytes = write(g_signalPipe[1], &signalCodeByte, 1)) < 0 && errno == EINTR); - if (writtenBytes != 1) - { - abort(); // fatal error - } + if (writtenBytes != 1) + { + abort(); // fatal error } // Delegate to any saved handler we may have @@ -154,6 +141,10 @@ static void* SignalHandlerLoop(void* arg) callback(reapAll ? 1 : 0); } } + else if (signalCode == SIGCONT) + { + ReinitializeConsole(); + } else { assert_msg(false, "invalid signalCode", (int)signalCode); @@ -192,13 +183,8 @@ void SystemNative_RestoreAndHandleCtrl(CtrlCode ctrlCode) kill(getpid(), signalCode); } -uint32_t SystemNative_RegisterForSigChld(SigChldCallback callback) +void SystemNative_RegisterForSigChld(SigChldCallback callback) { - if (!InitializeSignalHandling()) - { - return 0; - } - assert(callback != NULL); assert(g_sigChldCallback == NULL); @@ -207,8 +193,6 @@ uint32_t SystemNative_RegisterForSigChld(SigChldCallback callback) g_sigChldCallback = callback; } pthread_mutex_unlock(&lock); - - return 1; } static void InstallSignalHandler(int sig, bool skipWhenSigIgn) @@ -236,7 +220,7 @@ static void InstallSignalHandler(int sig, bool skipWhenSigIgn) assert(rv == 0); } -static bool InitializeSignalHandlingCore() +int32_t InitializeSignalHandlingCore() { // Create a pipe we'll use to communicate with our worker // thread. We can't do anything interesting in the signal handler, @@ -244,7 +228,7 @@ static bool InitializeSignalHandlingCore() // the handling work. if (SystemNative_Pipe(g_signalPipe, PAL_O_CLOEXEC) != 0) { - return false; + return 0; } assert(g_signalPipe[0] >= 0); assert(g_signalPipe[1] >= 0); @@ -255,7 +239,7 @@ static bool InitializeSignalHandlingCore() { CloseSignalHandlingPipe(); errno = ENOMEM; - return false; + return 0; } *readFdPtr = g_signalPipe[0]; @@ -267,7 +251,7 @@ static bool InitializeSignalHandlingCore() free(readFdPtr); CloseSignalHandlingPipe(); errno = err; - return false; + return 0; } // Finally, register our signal handlers @@ -278,21 +262,5 @@ static bool InitializeSignalHandlingCore() InstallSignalHandler(SIGCONT, /* skipWhenSigIgn */ false); InstallSignalHandler(SIGCHLD, /* skipWhenSigIgn */ false); - return true; -} - -uint32_t InitializeSignalHandling() -{ - static bool initialized = false; - - pthread_mutex_lock(&lock); - { - if (!initialized) - { - initialized = InitializeSignalHandlingCore(); - } - } - pthread_mutex_unlock(&lock); - - return initialized ? 1 : 0; + return 1; } diff --git a/src/Native/Unix/System.Native/pal_signal.h b/src/Native/Unix/System.Native/pal_signal.h index 5f8cb7affa5d..98d0c95b508e 100644 --- a/src/Native/Unix/System.Native/pal_signal.h +++ b/src/Native/Unix/System.Native/pal_signal.h @@ -8,11 +8,11 @@ #include "pal_types.h" /** - * Initializes the signal handling for use by System.Console and System.Process. + * Initializes the signal handling, called by InitializeConsoleAndSignalHandling. * * Returns 1 on success; otherwise returns 0 and sets errno. */ -uint32_t InitializeSignalHandling(void); +int32_t InitializeSignalHandlingCore(void); /** * Hooks up the specified callback for notifications when SIGINT or SIGQUIT is received. @@ -41,12 +41,9 @@ typedef void (*SigChldCallback)(int reapAll); /** * Hooks up the specified callback for notifications when SIGCHLD is received. * - * Not thread safe. Caller must provide its owns synchronization to ensure RegisterForSigChld - * is not called concurrently with itself. - * * Should only be called when a callback is not currently registered. */ -DLLEXPORT uint32_t SystemNative_RegisterForSigChld(SigChldCallback callback); +DLLEXPORT void SystemNative_RegisterForSigChld(SigChldCallback callback); /** * Remove our handler and reissue the signal to be picked up by the previously registered handler. diff --git a/src/System.Console/src/System.Console.csproj b/src/System.Console/src/System.Console.csproj index 841fd3d791c2..ed8f60774827 100644 --- a/src/System.Console/src/System.Console.csproj +++ b/src/System.Console/src/System.Console.csproj @@ -256,8 +256,8 @@ Common\Interop\Unix\Interop.GetWindowWidth.cs - - Common\Interop\Unix\Interop.InitializeConsole.cs + + Common\Interop\Unix\Interop.InitializeConsoleAndSignalHandling.cs Common\Interop\Unix\Interop.ReadStdinUnbuffered.cs diff --git a/src/System.Console/src/System/ConsolePal.Unix.cs b/src/System.Console/src/System/ConsolePal.Unix.cs index 527b36409f1c..e931dc981b87 100644 --- a/src/System.Console/src/System/ConsolePal.Unix.cs +++ b/src/System.Console/src/System/ConsolePal.Unix.cs @@ -380,7 +380,7 @@ private static unsafe void GetCursorPosition(out int left, out int top) // involved in reading/writing, such as when accessing a remote system. We also extend // the timeout on the very first request to 15 seconds, to account for potential latency // before we know if we will receive a response. - Interop.Sys.InitializeConsoleBeforeRead(minChars: (byte)(s_everReceivedCursorPositionResponse ? 1 : 0), decisecondsTimeout: (byte)(s_firstCursorPositionRequest ? 100 : 10)); + Interop.Sys.ConfigureConsoleTimeout(minChars: (byte)(s_everReceivedCursorPositionResponse ? 1 : 0), decisecondsTimeout: (byte)(s_firstCursorPositionRequest ? 100 : 10)); try { // Write out the cursor position report request. @@ -453,7 +453,7 @@ private static unsafe void GetCursorPosition(out int left, out int top) } finally { - Interop.Sys.UninitializeConsoleAfterRead(); + Interop.Sys.ConfigureConsoleTimeout(minChars: 1, decisecondsTimeout: 0); s_firstCursorPositionRequest = false; } @@ -809,9 +809,8 @@ private static void EnsureInitializedCore() { if (!s_initialized) { - // Ensure the console is configured appropriately. This will start - // signal handlers, etc. - if (!Interop.Sys.InitializeConsole()) + // Setup signal handling and configure the terminal. + if (!Interop.Sys.InitializeConsoleAndSignalHandling()) { throw new Win32Exception(); } diff --git a/src/System.Console/src/System/IO/StdInReader.cs b/src/System.Console/src/System/IO/StdInReader.cs index eda2d9a500de..ecd270c43995 100644 --- a/src/System.Console/src/System/IO/StdInReader.cs +++ b/src/System.Console/src/System/IO/StdInReader.cs @@ -87,8 +87,6 @@ private string ReadLine(bool consumeKeys) Debug.Assert(_tmpKeys.Count == 0); string readLineStr = null; - // Disable echo and buffering. These will be disabled for the duration of the line read. - Interop.Sys.InitializeConsoleBeforeRead(); try { // Read key-by-key until we've read a line. @@ -174,8 +172,6 @@ private string ReadLine(bool consumeKeys) } finally { - Interop.Sys.UninitializeConsoleAfterRead(); - // If we're not consuming the read input, make the keys available for a future read while (_tmpKeys.Count > 0) { @@ -362,43 +358,35 @@ public unsafe ConsoleKeyInfo ReadKey(out bool previouslyProcessed) } previouslyProcessed = false; - Interop.Sys.InitializeConsoleBeforeRead(); - try - { - ConsoleKey key; - char ch; - bool isAlt, isCtrl, isShift; + ConsoleKey key; + char ch; + bool isAlt, isCtrl, isShift; - if (IsUnprocessedBufferEmpty()) + if (IsUnprocessedBufferEmpty()) + { + // Read in bytes + byte* bufPtr = stackalloc byte[BytesToBeRead]; + int result = ReadStdin(bufPtr, BytesToBeRead); + if (result > 0) { - // Read in bytes - byte* bufPtr = stackalloc byte[BytesToBeRead]; - int result = ReadStdin(bufPtr, BytesToBeRead); - if (result > 0) - { - // Append them - AppendExtraBuffer(bufPtr, result); - } - else - { - // Could be empty if EOL entered on its own. Pick one of the EOL characters we have, - // or just use 0 if none are available. - return new ConsoleKeyInfo((char) - (ConsolePal.s_veolCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veolCharacter : - ConsolePal.s_veol2Character != ConsolePal.s_posixDisableValue ? ConsolePal.s_veol2Character : - ConsolePal.s_veofCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veofCharacter : - 0), - default(ConsoleKey), false, false, false); - } + // Append them + AppendExtraBuffer(bufPtr, result); + } + else + { + // Could be empty if EOL entered on its own. Pick one of the EOL characters we have, + // or just use 0 if none are available. + return new ConsoleKeyInfo((char) + (ConsolePal.s_veolCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veolCharacter : + ConsolePal.s_veol2Character != ConsolePal.s_posixDisableValue ? ConsolePal.s_veol2Character : + ConsolePal.s_veofCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veofCharacter : + 0), + default(ConsoleKey), false, false, false); } - - MapBufferToConsoleKey(out key, out ch, out isShift, out isAlt, out isCtrl); - return new ConsoleKeyInfo(ch, key, isShift, isAlt, isCtrl); - } - finally - { - Interop.Sys.UninitializeConsoleAfterRead(); } + + MapBufferToConsoleKey(out key, out ch, out isShift, out isAlt, out isCtrl); + return new ConsoleKeyInfo(ch, key, isShift, isAlt, isCtrl); } /// Gets whether there's input waiting on stdin. diff --git a/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj b/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj index 101813958778..79f8e9d258f6 100644 --- a/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj +++ b/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj @@ -311,6 +311,9 @@ Common\Interop\Unix\Interop.SysConf.cs + + Common\Interop\Unix\Interop.ConfigureConsoleForInteractiveChild.cs + Common\Interop\Unix\Interop.ForkAndExecProcess.cs @@ -329,6 +332,9 @@ Common\Interop\Unix\Interop.GetSid.cs + + Common\Interop\Unix\Interop.InitializeConsoleAndSignalHandling.cs + Common\Interop\Unix\Interop.Kill.cs diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index c50a6ebd1438..570036e806da 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -19,10 +19,11 @@ public partial class Process : IDisposable { private static readonly UTF8Encoding s_utf8NoBom = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false); - private static volatile bool s_sigchildHandlerRegistered = false; - private static readonly object s_sigchildGate = new object(); + private static volatile bool s_initialized = false; + private static readonly object s_initializedGate = new object(); private static readonly Interop.Sys.SigChldCallback s_sigChildHandler = OnSigChild; private static readonly ReaderWriterLockSlim s_processStartLock = new ReaderWriterLockSlim(); + private static int s_interactiveChildProcessCount; /// /// Puts a Process component in state to interact with operating system processes that run in a @@ -344,7 +345,7 @@ private SafeProcessHandle GetProcessHandle() /// The start info with which to start the process. private bool StartCore(ProcessStartInfo startInfo) { - EnsureSigChildHandler(); + EnsureInitialized(); string filename; string[] argv; @@ -369,6 +370,13 @@ private bool StartCore(ProcessStartInfo startInfo) (userId, groupId) = GetUserAndGroupIds(startInfo); } + // .NET applications don't echo characters unless there is a Console.Read operation. + // Unix applications expect the terminal to be in an echoing state by default. + // To support processes that interact with the terminal (e.g. 'vi'), we need to configure the + // terminal to echo. We keep this configuration as long as there are interactive applications. + // Based on ProcessStartInfo we determine if the child may be an interactive application. + bool isInteractiveChild = !startInfo.RedirectStandardInput && !startInfo.RedirectStandardOutput; + if (startInfo.UseShellExecute) { string verb = startInfo.Verb; @@ -392,7 +400,7 @@ private bool StartCore(ProcessStartInfo startInfo) isExecuting = ForkAndExecProcess(filename, argv, envp, cwd, startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, setCredentials, userId, groupId, - out stdinFd, out stdoutFd, out stderrFd, + out stdinFd, out stdoutFd, out stderrFd, isInteractiveChild, throwOnNoExec: false); // return false instead of throwing on ENOEXEC } @@ -405,7 +413,7 @@ private bool StartCore(ProcessStartInfo startInfo) ForkAndExecProcess(filename, argv, envp, cwd, startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, setCredentials, userId, groupId, - out stdinFd, out stdoutFd, out stderrFd); + out stdinFd, out stdoutFd, out stderrFd, isInteractiveChild); } } else @@ -420,7 +428,7 @@ private bool StartCore(ProcessStartInfo startInfo) ForkAndExecProcess(filename, argv, envp, cwd, startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, setCredentials, userId, groupId, - out stdinFd, out stdoutFd, out stderrFd); + out stdinFd, out stdoutFd, out stderrFd, isInteractiveChild); } // Configure the parent's ends of the redirection streams. @@ -455,7 +463,7 @@ private bool ForkAndExecProcess( bool redirectStdin, bool redirectStdout, bool redirectStderr, bool setCredentials, uint userId, uint groupId, out int stdinFd, out int stdoutFd, out int stderrFd, - bool throwOnNoExec = true) + bool isInteractiveChild, bool throwOnNoExec = true) { if (string.IsNullOrEmpty(filename)) { @@ -467,6 +475,11 @@ private bool ForkAndExecProcess( s_processStartLock.EnterReadLock(); try { + if (isInteractiveChild) + { + ConfigureConsoleForInteractiveChildren(+1); + } + int childPid; // Invoke the shim fork/execve routine. It will create pipes for all requested @@ -485,7 +498,7 @@ private bool ForkAndExecProcess( { // Ensure we'll reap this process. // note: SetProcessId will set this if we don't set it first. - _waitStateHolder = new ProcessWaitState.Holder(childPid, isNewChild: true); + _waitStateHolder = new ProcessWaitState.Holder(childPid, isNewChild: true, isInteractiveChild); // Store the child's information into this Process object. Debug.Assert(childPid >= 0); @@ -508,6 +521,14 @@ private bool ForkAndExecProcess( finally { s_processStartLock.ExitReadLock(); + + // We failed to launch an interactive child. + if (_waitStateHolder == null && isInteractiveChild) + { + s_processStartLock.EnterWriteLock(); + ConfigureConsoleForInteractiveChildren(-1); + s_processStartLock.ExitWriteLock(); + } } } @@ -940,24 +961,27 @@ private static unsafe bool TryGetPasswd(string name, byte* buf, int bufLen, out private bool WaitForInputIdleCore(int milliseconds) => throw new InvalidOperationException(SR.InputIdleUnkownError); - private static void EnsureSigChildHandler() + private static void EnsureInitialized() { - if (s_sigchildHandlerRegistered) + if (s_initialized) { return; } - lock (s_sigchildGate) + lock (s_initializedGate) { - if (!s_sigchildHandlerRegistered) + if (!s_initialized) { - // Ensure signal handling is setup and register our callback. - if (!Interop.Sys.RegisterForSigChld(s_sigChildHandler)) + // Setup signal handling and configure the terminal. + if (!Interop.Sys.InitializeConsoleAndSignalHandling()) { throw new Win32Exception(); } - s_sigchildHandlerRegistered = true; + // Register our callback. + Interop.Sys.RegisterForSigChld(s_sigChildHandler); + + s_initialized = true; } } } @@ -968,12 +992,38 @@ private static void OnSigChild(bool reapAll) s_processStartLock.EnterWriteLock(); try { - ProcessWaitState.CheckChildren(reapAll); + ProcessWaitState.CheckChildren(reapAll, out int reapedInteractiveChildren); + + if (reapedInteractiveChildren > 0) + { + ConfigureConsoleForInteractiveChildren(-reapedInteractiveChildren); + } } finally { s_processStartLock.ExitWriteLock(); } } + + private static void ConfigureConsoleForInteractiveChildren(int increment) + { + Debug.Assert(increment != 0); + + int interactiveChildrenRemaining = Interlocked.Add(ref s_interactiveChildProcessCount, increment); + if (increment > 0) + { + Debug.Assert(s_processStartLock.IsReadLockHeld); + // This will noop if the console is already configured for an interactive child. + Interop.Sys.ConfigureConsoleForInteractiveChild(true); + } + else + { + Debug.Assert(s_processStartLock.IsWriteLockHeld); + if (interactiveChildrenRemaining == 0) + { + Interop.Sys.ConfigureConsoleForInteractiveChild(false); + } + } + } } } diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs index 2aeec4bc0725..e8f163d80b1e 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs @@ -55,9 +55,9 @@ internal sealed class Holder : IDisposable { internal ProcessWaitState _state; - internal Holder(int processId, bool isNewChild = false) + internal Holder(int processId, bool isNewChild = false, bool isInteractiveChild = false) { - _state = ProcessWaitState.AddRef(processId, isNewChild); + _state = ProcessWaitState.AddRef(processId, isNewChild, isInteractiveChild); } ~Holder() @@ -99,7 +99,7 @@ public void Dispose() /// /// The process ID for which we need wait state. /// The wait state object. - internal static ProcessWaitState AddRef(int processId, bool isNewChild) + internal static ProcessWaitState AddRef(int processId, bool isNewChild, bool isInteractiveChild) { lock (s_childProcessWaitStates) { @@ -109,7 +109,7 @@ internal static ProcessWaitState AddRef(int processId, bool isNewChild) // When the PID is recycled for a new child, we remove the old child. s_childProcessWaitStates.Remove(processId); - pws = new ProcessWaitState(processId, isChild: true); + pws = new ProcessWaitState(processId, isChild: true, isInteractiveChild); s_childProcessWaitStates.Add(processId, pws); pws._outstandingRefCount++; // For Holder pws._outstandingRefCount++; // Decremented in CheckChildren @@ -143,7 +143,7 @@ internal static ProcessWaitState AddRef(int processId, bool isNewChild) } if (pws == null) { - pws = new ProcessWaitState(processId, isChild: false, exitTime); + pws = new ProcessWaitState(processId, isChild: false, isInteractiveChild: false, exitTime); s_processWaitStates.Add(processId, pws); } pws._outstandingRefCount++; @@ -196,6 +196,8 @@ internal void ReleaseRef() private readonly int _processId; /// Associated process is a child process. private readonly bool _isChild; + /// Associated process is an interactive child process. + private readonly bool _isInteractiveChild; /// If a wait operation is in progress, the Task that represents it; otherwise, null. private Task _waitInProgress; @@ -216,11 +218,12 @@ internal void ReleaseRef() /// Initialize the wait state object. /// The associated process' ID. - private ProcessWaitState(int processId, bool isChild, DateTime exitTime = default) + private ProcessWaitState(int processId, bool isChild, bool isInteractiveChild, DateTime exitTime = default) { Debug.Assert(processId >= 0); _processId = processId; _isChild = isChild; + _isInteractiveChild = isInteractiveChild; _exitTime = exitTime; } @@ -545,7 +548,7 @@ internal bool WaitForExit(int millisecondsTimeout) }); } - private bool TryReapChild() + private bool TryReapChild(ref int interactiveChildrenReaped) { lock (_gate) { @@ -563,6 +566,12 @@ private bool TryReapChild() _exitCode = exitCode; SetExited(); + + if (_isInteractiveChild) + { + interactiveChildrenReaped++; + } + return true; } else if (waitResult == 0) @@ -579,8 +588,10 @@ private bool TryReapChild() } } - internal static void CheckChildren(bool reapAll) + internal static void CheckChildren(bool reapAll, out int interactiveChildrenReaped) { + interactiveChildrenReaped = 0; + // This is called on SIGCHLD from a native thread. // A lock in Process ensures no new processes are spawned while we are checking. lock (s_childProcessWaitStates) @@ -598,7 +609,7 @@ internal static void CheckChildren(bool reapAll) if (s_childProcessWaitStates.TryGetValue(pid, out ProcessWaitState pws)) { // Known Process. - if (pws.TryReapChild()) + if (pws.TryReapChild(ref interactiveChildrenReaped)) { pws.ReleaseRef(); } @@ -631,7 +642,7 @@ internal static void CheckChildren(bool reapAll) foreach (KeyValuePair kv in s_childProcessWaitStates) { ProcessWaitState pws = kv.Value; - if (pws.TryReapChild()) + if (pws.TryReapChild(ref interactiveChildrenReaped)) { if (firstToRemove == null) { From 47b3e2898da2778607e98174cd2669d5311c36f3 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 27 Feb 2019 17:22:06 +0100 Subject: [PATCH 02/14] Make some tcsetattr operations non-blocking --- src/Native/Unix/System.Native/pal_console.c | 62 ++++++++++++++++----- 1 file changed, 49 insertions(+), 13 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_console.c b/src/Native/Unix/System.Native/pal_console.c index c099bc9dce42..d9dafbc8011d 100644 --- a/src/Native/Unix/System.Native/pal_console.c +++ b/src/Native/Unix/System.Native/pal_console.c @@ -18,6 +18,7 @@ #include #include #include +#include int32_t SystemNative_GetWindowSize(WinSize* windowSize) { @@ -78,6 +79,7 @@ static bool g_hasInteractiveChildren = false; // tracks whether the application static struct termios g_initTermios; // the initial attributes captured when Console was initialized static bool g_consoleUninitialized = false; // tracks whether the application is terminating static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER; +static volatile bool g_receivedSigTtou = false; static void ApplyTerminalSettings(struct termios* termios, bool signalForBreak, bool hasInteractiveChildren) { @@ -109,17 +111,54 @@ static bool TcGetAttr(struct termios* termios, bool signalForBreak, bool hasInte return rv; } -static bool TcSetAttr(struct termios* t) +static void ttou_handler(int signo) { - // This method is called with lock g_lock held. + (void)signo; + g_receivedSigTtou = true; +} +static bool TcSetAttr(struct termios* t, bool blockIfBackground) +{ if (g_consoleUninitialized) { // The application is exiting, we mustn't change terminal settings. return true; } - return tcsetattr(STDIN_FILENO, TCSANOW, t) >= 0; + if (!blockIfBackground) + { + // When the process is running in background, changing terminal settings + // will stop it (default SIGTTOU action). + // We change SIGTTOU to to get EINTR instead of blocking. + struct sigaction action; + memset(&action, 0, sizeof(action)); + action.sa_handler = ttou_handler; + int rvSigaction = sigaction(SIGTTOU, &action, NULL); + assert(rvSigaction == 0); + + g_receivedSigTtou = false; + } + + bool rv = tcsetattr(STDIN_FILENO, TCSANOW, t) >= 0; + + if (!blockIfBackground) + { + if (!rv && errno == EINTR && g_receivedSigTtou) + { + // Operation failed because we are background + // pretend it went fine. + rv = true; + } + + // Restore default SIGTTOU handler. + struct sigaction action; + memset(&action, 0, sizeof(action)); + action.sa_handler = SIG_DFL; + int rvSigaction = sigaction(SIGTTOU, &action, NULL); + assert(rvSigaction == 0); + } + + return rv; } void UninitializeConsole() @@ -136,8 +175,7 @@ void UninitializeConsole() if (g_haveInitTermios) { - // TODO: https://github.com/dotnet/cli/issues/6216 - TcSetAttr(&g_initTermios); + TcSetAttr(&g_initTermios, /* blockIfBackground */ false); } g_consoleUninitialized = true; @@ -156,7 +194,7 @@ void SystemNative_ConfigureConsoleTimeout(uint8_t minChars, uint8_t decisecondsT termios.c_cc[VMIN] = minChars; termios.c_cc[VTIME] = decisecondsTimeout; - TcSetAttr(&termios); + TcSetAttr(&termios, /* blockIfBackground */ true); } pthread_mutex_unlock(&g_lock); } @@ -176,11 +214,9 @@ void SystemNative_ConfigureConsoleForInteractiveChild(int32_t hasInteractiveChil struct termios termios; if (TcGetAttr(&termios, g_signalForBreak, hasInteractiveChildren)) { - if (TcSetAttr(&termios)) - { - g_hasInteractiveChildren = hasInteractiveChildren; - } + TcSetAttr(&termios, /* blockIfBackground */ false); } + g_hasInteractiveChildren = hasInteractiveChildren; // Redo "Application mode" when there are no more interactive children. if (!hasInteractiveChildren) @@ -327,7 +363,7 @@ int32_t SystemNative_SetSignalForBreak(int32_t signalForBreak) struct termios termios; if (TcGetAttr(&termios, signalForBreak, g_hasInteractiveChildren)) { - if (TcSetAttr(&termios)) + if (TcSetAttr(&termios, /* blockIfBackground */ false)) { g_signalForBreak = signalForBreak; rv = 1; @@ -349,7 +385,7 @@ void ReinitializeConsole() struct termios termios; if (TcGetAttr(&termios, g_signalForBreak, g_hasInteractiveChildren)) { - TcSetAttr(&termios); + TcSetAttr(&termios, /* blockIfBackground */ false); } // Redo "Application mode" unless there are interactive children. @@ -382,7 +418,7 @@ int32_t SystemNative_InitializeConsoleAndSignalHandling() { struct termios termios = g_initTermios; ApplyTerminalSettings(&termios, g_signalForBreak, g_hasInteractiveChildren); - TcSetAttr(&termios); + TcSetAttr(&termios, /* blockIfBackground */ false); } atexit(UninitializeConsole); From 5e1f70cb96416f53a96a42487314bfefb07c1225 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Wed, 27 Feb 2019 17:34:34 +0100 Subject: [PATCH 03/14] Fix order between changing console settings and SetExited --- .../src/System/Diagnostics/Process.Unix.cs | 9 ++------- .../Diagnostics/ProcessWaitState.Unix.cs | 18 +++++++----------- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index 570036e806da..22039b5a82ae 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -992,12 +992,7 @@ private static void OnSigChild(bool reapAll) s_processStartLock.EnterWriteLock(); try { - ProcessWaitState.CheckChildren(reapAll, out int reapedInteractiveChildren); - - if (reapedInteractiveChildren > 0) - { - ConfigureConsoleForInteractiveChildren(-reapedInteractiveChildren); - } + ProcessWaitState.CheckChildren(reapAll); } finally { @@ -1005,7 +1000,7 @@ private static void OnSigChild(bool reapAll) } } - private static void ConfigureConsoleForInteractiveChildren(int increment) + internal static void ConfigureConsoleForInteractiveChildren(int increment) { Debug.Assert(increment != 0); diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs index e8f163d80b1e..83bd90171c3b 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs @@ -548,7 +548,7 @@ internal bool WaitForExit(int millisecondsTimeout) }); } - private bool TryReapChild(ref int interactiveChildrenReaped) + private bool TryReapChild() { lock (_gate) { @@ -565,12 +565,10 @@ private bool TryReapChild(ref int interactiveChildrenReaped) { _exitCode = exitCode; - SetExited(); + // Update console settings before calling SetExited. + Process.ConfigureConsoleForInteractiveChildren(-1); - if (_isInteractiveChild) - { - interactiveChildrenReaped++; - } + SetExited(); return true; } @@ -588,10 +586,8 @@ private bool TryReapChild(ref int interactiveChildrenReaped) } } - internal static void CheckChildren(bool reapAll, out int interactiveChildrenReaped) + internal static void CheckChildren(bool reapAll) { - interactiveChildrenReaped = 0; - // This is called on SIGCHLD from a native thread. // A lock in Process ensures no new processes are spawned while we are checking. lock (s_childProcessWaitStates) @@ -609,7 +605,7 @@ internal static void CheckChildren(bool reapAll, out int interactiveChildrenReap if (s_childProcessWaitStates.TryGetValue(pid, out ProcessWaitState pws)) { // Known Process. - if (pws.TryReapChild(ref interactiveChildrenReaped)) + if (pws.TryReapChild()) { pws.ReleaseRef(); } @@ -642,7 +638,7 @@ internal static void CheckChildren(bool reapAll, out int interactiveChildrenReap foreach (KeyValuePair kv in s_childProcessWaitStates) { ProcessWaitState pws = kv.Value; - if (pws.TryReapChild(ref interactiveChildrenReaped)) + if (pws.TryReapChild()) { if (firstToRemove == null) { From d0685cb4fe3ec089fa2c098ce02562a2c9efd391 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 1 Mar 2019 11:33:02 +0100 Subject: [PATCH 04/14] Fix unbalanced lock/unlocks --- src/Native/Unix/System.Native/pal_console.c | 40 ++++++++++----------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_console.c b/src/Native/Unix/System.Native/pal_console.c index d9dafbc8011d..c5badd988672 100644 --- a/src/Native/Unix/System.Native/pal_console.c +++ b/src/Native/Unix/System.Native/pal_console.c @@ -168,18 +168,16 @@ void UninitializeConsole() if (pthread_mutex_lock(&g_lock) == 0) { - if (g_consoleUninitialized) + if (!g_consoleUninitialized) { - return; - } + if (g_haveInitTermios) + { + TcSetAttr(&g_initTermios, /* blockIfBackground */ false); + } - if (g_haveInitTermios) - { - TcSetAttr(&g_initTermios, /* blockIfBackground */ false); + g_consoleUninitialized = true; } - g_consoleUninitialized = true; - pthread_mutex_unlock(&g_lock); } } @@ -206,22 +204,20 @@ void SystemNative_ConfigureConsoleForInteractiveChild(int32_t hasInteractiveChil if (pthread_mutex_lock(&g_lock) == 0) { - if (g_hasInteractiveChildren == hasInteractiveChildren) - { - return; - } - - struct termios termios; - if (TcGetAttr(&termios, g_signalForBreak, hasInteractiveChildren)) + if (g_hasInteractiveChildren != hasInteractiveChildren) { - TcSetAttr(&termios, /* blockIfBackground */ false); - } - g_hasInteractiveChildren = hasInteractiveChildren; + struct termios termios; + if (TcGetAttr(&termios, g_signalForBreak, hasInteractiveChildren)) + { + TcSetAttr(&termios, /* blockIfBackground */ false); + } + g_hasInteractiveChildren = hasInteractiveChildren; - // Redo "Application mode" when there are no more interactive children. - if (!hasInteractiveChildren) - { - WriteKeypadXmit(); + // Redo "Application mode" when there are no more interactive children. + if (!hasInteractiveChildren) + { + WriteKeypadXmit(); + } } pthread_mutex_unlock(&g_lock); From 435df018e573bfe3e5b28b6bc05430c5a58d4447 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 1 Mar 2019 11:57:51 +0100 Subject: [PATCH 05/14] Cache notty --- src/Native/Unix/System.Native/pal_console.c | 41 ++++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_console.c b/src/Native/Unix/System.Native/pal_console.c index c5badd988672..c7577efa238a 100644 --- a/src/Native/Unix/System.Native/pal_console.c +++ b/src/Native/Unix/System.Native/pal_console.c @@ -80,6 +80,7 @@ static struct termios g_initTermios; // the initial attributes captured static bool g_consoleUninitialized = false; // tracks whether the application is terminating static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER; static volatile bool g_receivedSigTtou = false; +static bool g_noTty = false; // cache we are not a tty static void ApplyTerminalSettings(struct termios* termios, bool signalForBreak, bool hasInteractiveChildren) { @@ -103,7 +104,19 @@ static void ApplyTerminalSettings(struct termios* termios, bool signalForBreak, static bool TcGetAttr(struct termios* termios, bool signalForBreak, bool hasInteractiveChildren) { + if (g_noTty) + { + errno = ENOTTY; + return false; + } + bool rv = tcgetattr(STDIN_FILENO, termios) >= 0; + + if (!rv && errno == ENOTTY) + { + g_noTty = true; + } + if (rv) { ApplyTerminalSettings(termios, signalForBreak, hasInteractiveChildren); @@ -117,8 +130,24 @@ static void ttou_handler(int signo) g_receivedSigTtou = true; } +static void InstallTTOUHandler(void (*handler)(int)) +{ + struct sigaction action; + memset(&action, 0, sizeof(action)); + action.sa_handler = handler; + int rvSigaction = sigaction(SIGTTOU, &action, NULL); + assert(rvSigaction == 0); + (void)rvSigaction; +} + static bool TcSetAttr(struct termios* t, bool blockIfBackground) { + if (g_noTty) + { + errno = ENOTTY; + return false; + } + if (g_consoleUninitialized) { // The application is exiting, we mustn't change terminal settings. @@ -130,11 +159,7 @@ static bool TcSetAttr(struct termios* t, bool blockIfBackground) // When the process is running in background, changing terminal settings // will stop it (default SIGTTOU action). // We change SIGTTOU to to get EINTR instead of blocking. - struct sigaction action; - memset(&action, 0, sizeof(action)); - action.sa_handler = ttou_handler; - int rvSigaction = sigaction(SIGTTOU, &action, NULL); - assert(rvSigaction == 0); + InstallTTOUHandler(ttou_handler); g_receivedSigTtou = false; } @@ -151,11 +176,7 @@ static bool TcSetAttr(struct termios* t, bool blockIfBackground) } // Restore default SIGTTOU handler. - struct sigaction action; - memset(&action, 0, sizeof(action)); - action.sa_handler = SIG_DFL; - int rvSigaction = sigaction(SIGTTOU, &action, NULL); - assert(rvSigaction == 0); + InstallTTOUHandler(SIG_DFL); } return rv; From a86b9f09e0d1fc10491d62564ff5a70735d6ac56 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 4 Mar 2019 15:35:38 +0100 Subject: [PATCH 06/14] Prefer configuring the terminal for Console over child processes --- ...terop.ConfigureTerminalForChildProcess.cs} | 4 +- ...op.InitializeTerminalAndSignalHandling.cs} | 4 +- .../Interop.ReadStdinUnbuffered.cs | 4 +- src/Native/Unix/System.Native/pal_console.c | 237 ++++++++++-------- src/Native/Unix/System.Native/pal_console.h | 10 +- src/Native/Unix/System.Native/pal_signal.h | 2 +- src/System.Console/src/System.Console.csproj | 4 +- .../src/System/ConsolePal.Unix.cs | 6 +- .../src/System/IO/StdInReader.cs | 53 ++-- .../src/System.Diagnostics.Process.csproj | 8 +- .../src/System/Diagnostics/Process.Unix.cs | 40 ++- .../Diagnostics/ProcessWaitState.Unix.cs | 20 +- 12 files changed, 212 insertions(+), 180 deletions(-) rename src/Common/src/Interop/Unix/System.Native/{Interop.ConfigureConsoleForInteractiveChild.cs => Interop.ConfigureTerminalForChildProcess.cs} (74%) rename src/Common/src/Interop/Unix/System.Native/{Interop.InitializeConsoleAndSignalHandling.cs => Interop.InitializeTerminalAndSignalHandling.cs} (79%) diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.ConfigureConsoleForInteractiveChild.cs b/src/Common/src/Interop/Unix/System.Native/Interop.ConfigureTerminalForChildProcess.cs similarity index 74% rename from src/Common/src/Interop/Unix/System.Native/Interop.ConfigureConsoleForInteractiveChild.cs rename to src/Common/src/Interop/Unix/System.Native/Interop.ConfigureTerminalForChildProcess.cs index cfbd3b00ef65..bfe6f08ec5c3 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.ConfigureConsoleForInteractiveChild.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.ConfigureTerminalForChildProcess.cs @@ -9,7 +9,7 @@ internal static partial class Interop { internal static partial class Sys { - [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ConfigureConsoleForInteractiveChild")] - internal static extern unsafe void ConfigureConsoleForInteractiveChild(bool enable); + [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ConfigureTerminalForChildProcess")] + internal static extern unsafe void ConfigureTerminalForChildProcess(bool enable); } } diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.InitializeConsoleAndSignalHandling.cs b/src/Common/src/Interop/Unix/System.Native/Interop.InitializeTerminalAndSignalHandling.cs similarity index 79% rename from src/Common/src/Interop/Unix/System.Native/Interop.InitializeConsoleAndSignalHandling.cs rename to src/Common/src/Interop/Unix/System.Native/Interop.InitializeTerminalAndSignalHandling.cs index 780c2b0f3aa7..c6ca6982a6d0 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.InitializeConsoleAndSignalHandling.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.InitializeTerminalAndSignalHandling.cs @@ -8,8 +8,8 @@ internal static partial class Interop { internal static partial class Sys { - [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_InitializeConsoleAndSignalHandling", SetLastError = true)] - internal static extern bool InitializeConsoleAndSignalHandling(); + [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_InitializeTerminalAndSignalHandling", SetLastError = true)] + internal static extern bool InitializeTerminalAndSignalHandling(); [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_SetKeypadXmit")] internal static extern void SetKeypadXmit(string terminfoString); diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs b/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs index f8b8cb00c642..9fd3cb0ef4cb 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs @@ -12,7 +12,7 @@ internal static partial class Sys [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ReadStdin", SetLastError = true)] internal static extern unsafe int ReadStdin(byte* buffer, int bufferSize); - [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ConfigureConsoleTimeout")] - internal static extern unsafe void ConfigureConsoleTimeout(byte minChars = 1, byte decisecondsTimeout = 0); + [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ConfigureTerminalForConsole")] + internal static extern unsafe void ConfigureTerminalForConsole(bool reading, byte minChars = 1, byte decisecondsTimeout = 0); } } diff --git a/src/Native/Unix/System.Native/pal_console.c b/src/Native/Unix/System.Native/pal_console.c index c7577efa238a..5fc9ee0613cf 100644 --- a/src/Native/Unix/System.Native/pal_console.c +++ b/src/Native/Unix/System.Native/pal_console.c @@ -73,56 +73,29 @@ void SystemNative_SetKeypadXmit(const char* terminfoString) WriteKeypadXmit(); } -static bool g_signalForBreak = true; // tracks whether the terminal should send signals for breaks, such that attributes have been changed -static bool g_haveInitTermios = false; // whether g_initTermios has been initialized -static bool g_hasInteractiveChildren = false; // tracks whether the application has interactive child processes. -static struct termios g_initTermios; // the initial attributes captured when Console was initialized -static bool g_consoleUninitialized = false; // tracks whether the application is terminating static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER; -static volatile bool g_receivedSigTtou = false; -static bool g_noTty = false; // cache we are not a tty -static void ApplyTerminalSettings(struct termios* termios, bool signalForBreak, bool hasInteractiveChildren) -{ - if (signalForBreak) - termios->c_lflag |= (uint32_t)ISIG; - else - termios->c_lflag &= (uint32_t)(~ISIG); +static bool g_signalForBreak = true; // tracks whether the terminal should send signals for breaks, such that attributes have been changed - if (hasInteractiveChildren) - { - assert(g_haveInitTermios); - termios->c_iflag = g_initTermios.c_iflag; - termios->c_lflag = g_initTermios.c_lflag; - } - else - { - termios->c_iflag &= (uint32_t)(~(IXON | IXOFF)); - termios->c_lflag &= (uint32_t)(~(ECHO | ICANON | IEXTEN)); - } -} +static bool g_haveInitTermios = false; // tracks whether g_initTermios has been initialized +static struct termios g_initTermios; // the initial attributes captured -static bool TcGetAttr(struct termios* termios, bool signalForBreak, bool hasInteractiveChildren) -{ - if (g_noTty) - { - errno = ENOTTY; - return false; - } +static bool g_hasCurrentTermios = false; // tracks whether g_currentTermios is valid +static struct termios g_currentTermios; // the latest attributes set - bool rv = tcgetattr(STDIN_FILENO, termios) >= 0; +// The terminal can be used by the .NET application via the Console class. +// It may also be used by child processes that is started via the Process class. +// The terminal needs to be configured different depending on the user. +// ConfigureTerminalForXXX are called to change the configuration. +// When it is ambiguous whether we should configure for Console/a child Process, +// we prefer configuring for the Console. +static bool g_reading = false; // tracks whether the application is performing a Console.Read operation +static bool g_childUsesTerminal = false; // tracks whether a child process is using the terminal +static bool g_consoleUninitialized = false; // tracks whether the application is terminating - if (!rv && errno == ENOTTY) - { - g_noTty = true; - } +static bool g_noTty = false; // cache we are not a tty - if (rv) - { - ApplyTerminalSettings(termios, signalForBreak, hasInteractiveChildren); - } - return rv; -} +static volatile bool g_receivedSigTtou = false; static void ttou_handler(int signo) { @@ -140,14 +113,8 @@ static void InstallTTOUHandler(void (*handler)(int)) (void)rvSigaction; } -static bool TcSetAttr(struct termios* t, bool blockIfBackground) +static bool TcSetAttr(struct termios* termios, bool blockIfBackground) { - if (g_noTty) - { - errno = ENOTTY; - return false; - } - if (g_consoleUninitialized) { // The application is exiting, we mustn't change terminal settings. @@ -164,7 +131,7 @@ static bool TcSetAttr(struct termios* t, bool blockIfBackground) g_receivedSigTtou = false; } - bool rv = tcsetattr(STDIN_FILENO, TCSANOW, t) >= 0; + bool rv = tcsetattr(STDIN_FILENO, TCSANOW, termios) >= 0; if (!blockIfBackground) { @@ -179,11 +146,61 @@ static bool TcSetAttr(struct termios* t, bool blockIfBackground) InstallTTOUHandler(SIG_DFL); } + if (rv) + { + g_hasCurrentTermios = true; + g_currentTermios = *termios; + } + return rv; } +static bool ConfigureTerminal(bool signalForBreak, bool forChild, uint8_t minChars, uint8_t decisecondsTimeout, bool blockIfBackground) +{ + if (g_noTty) + { + errno = ENOTTY; + return false; + } + + g_childUsesTerminal = forChild; + + assert(g_haveInitTermios); + struct termios termios = g_initTermios; + + if (signalForBreak) + termios.c_lflag |= (uint32_t)ISIG; + else + termios.c_lflag &= (uint32_t)(~ISIG); + + if (!forChild) + { + termios.c_iflag &= (uint32_t)(~(IXON | IXOFF)); + termios.c_lflag &= (uint32_t)(~(ECHO | ICANON | IEXTEN)); + } + + termios.c_cc[VMIN] = minChars; + termios.c_cc[VTIME] = decisecondsTimeout; + + // Check if the settings have changed. + if (g_hasCurrentTermios) + { + if (g_currentTermios.c_lflag == termios.c_lflag && + g_currentTermios.c_iflag == termios.c_iflag && + g_currentTermios.c_cc[VMIN] == termios.c_cc[VMIN] && + g_currentTermios.c_cc[VMIN] == termios.c_cc[VMIN]) + { + return true; + } + } + + return TcSetAttr(&termios, blockIfBackground); +} + void UninitializeConsole() { + assert(g_haveInitTermios); + // This method is called on SIGQUIT/SIGINT from the signal dispatching thread // and on atexit. @@ -191,10 +208,7 @@ void UninitializeConsole() { if (!g_consoleUninitialized) { - if (g_haveInitTermios) - { - TcSetAttr(&g_initTermios, /* blockIfBackground */ false); - } + TcSetAttr(&g_initTermios, /* blockIfBackground */ false); g_consoleUninitialized = true; } @@ -203,42 +217,42 @@ void UninitializeConsole() } } -void SystemNative_ConfigureConsoleTimeout(uint8_t minChars, uint8_t decisecondsTimeout) +void SystemNative_ConfigureTerminalForConsole(int32_t reading, uint8_t minChars, uint8_t decisecondsTimeout) { if (pthread_mutex_lock(&g_lock) == 0) { - struct termios termios; - if (TcGetAttr(&termios, g_signalForBreak, g_hasInteractiveChildren)) - { - termios.c_cc[VMIN] = minChars; - termios.c_cc[VTIME] = decisecondsTimeout; + g_reading = reading; + + ConfigureTerminal(g_signalForBreak, /* forChild */ false, minChars, decisecondsTimeout, /* blockIfBackground */ true); - TcSetAttr(&termios, /* blockIfBackground */ true); - } pthread_mutex_unlock(&g_lock); } } -void SystemNative_ConfigureConsoleForInteractiveChild(int32_t hasInteractiveChildren) +void SystemNative_ConfigureTerminalForChildProcess(int32_t childUsesTerminal) { - assert(hasInteractiveChildren == 0 || hasInteractiveChildren == 1); + assert(childUsesTerminal == 0 || childUsesTerminal == 1); if (pthread_mutex_lock(&g_lock) == 0) { - if (g_hasInteractiveChildren != hasInteractiveChildren) + // If the application is performing a read, assume the child process won't use the terminal. + if (g_reading) { - struct termios termios; - if (TcGetAttr(&termios, g_signalForBreak, hasInteractiveChildren)) - { - TcSetAttr(&termios, /* blockIfBackground */ false); - } - g_hasInteractiveChildren = hasInteractiveChildren; + return; + } - // Redo "Application mode" when there are no more interactive children. - if (!hasInteractiveChildren) - { - WriteKeypadXmit(); - } + // If no more children are using the terminal, invalidate our cached termios. + if (!childUsesTerminal) + { + g_hasCurrentTermios = false; + } + + ConfigureTerminal(g_signalForBreak, /* forChild */ childUsesTerminal, /* minChars */ 1, /* decisecondsTimeout */ 0, /* blockIfBackground */ false); + + // Redo "Application mode" when there are no more interactive children. + if (!childUsesTerminal) + { + WriteKeypadXmit(); } pthread_mutex_unlock(&g_lock); @@ -342,9 +356,10 @@ void SystemNative_GetControlCharacters( int32_t SystemNative_StdinReady() { - // TODO ??: https://github.com/dotnet/corefx/pull/6619 + SystemNative_ConfigureTerminalForConsole(1, 1, 0); struct pollfd fd = { .fd = STDIN_FILENO, .events = POLLIN }; int rv = poll(&fd, 1, 0) > 0 ? 1 : 0; + SystemNative_ConfigureTerminalForConsole(0, 1, 0); return rv; } @@ -377,15 +392,12 @@ int32_t SystemNative_SetSignalForBreak(int32_t signalForBreak) if (pthread_mutex_lock(&g_lock) == 0) { - struct termios termios; - if (TcGetAttr(&termios, signalForBreak, g_hasInteractiveChildren)) + if (ConfigureTerminal(signalForBreak, /* forChild */ false, /* minChars */ 1, /* decisecondsTimeout */ 0, /* blockIfBackground */ true)) { - if (TcSetAttr(&termios, /* blockIfBackground */ false)) - { - g_signalForBreak = signalForBreak; - rv = 1; - } + g_signalForBreak = signalForBreak; + rv = 1; } + pthread_mutex_unlock(&g_lock); } @@ -399,16 +411,13 @@ void ReinitializeConsole() if (pthread_mutex_lock(&g_lock) == 0) { - struct termios termios; - if (TcGetAttr(&termios, g_signalForBreak, g_hasInteractiveChildren)) + if (!g_childUsesTerminal) { - TcSetAttr(&termios, /* blockIfBackground */ false); - } + if (g_hasCurrentTermios) + { + TcSetAttr(&g_currentTermios, /* blockIfBackground */ false); + } - // Redo "Application mode" unless there are interactive children. - // In that case, we'll redo it when there are no more interactive children. - if (!g_hasInteractiveChildren) - { WriteKeypadXmit(); } @@ -416,7 +425,32 @@ void ReinitializeConsole() } } -int32_t SystemNative_InitializeConsoleAndSignalHandling() +static bool InitializeTerminalCore() +{ + g_haveInitTermios = tcgetattr(STDIN_FILENO, &g_initTermios) >= 0; + + if (!g_haveInitTermios && errno == ENOTTY) + { + g_noTty = true; + } + + if (g_haveInitTermios) + { + g_hasCurrentTermios = true; + g_currentTermios = g_initTermios; + g_signalForBreak = g_initTermios.c_lflag & (uint32_t)ISIG; + + atexit(UninitializeConsole); + } + else + { + g_signalForBreak = true; + } + + return g_haveInitTermios || g_noTty; +} + +int32_t SystemNative_InitializeTerminalAndSignalHandling() { static int32_t initialized = 0; @@ -425,20 +459,9 @@ int32_t SystemNative_InitializeConsoleAndSignalHandling() { if (initialized == 0) { - initialized = InitializeSignalHandlingCore(); - - if (initialized == 1) + if (InitializeTerminalCore()) { - g_haveInitTermios = tcgetattr(STDIN_FILENO, &g_initTermios) >= 0; - - if (g_haveInitTermios) - { - struct termios termios = g_initTermios; - ApplyTerminalSettings(&termios, g_signalForBreak, g_hasInteractiveChildren); - TcSetAttr(&termios, /* blockIfBackground */ false); - } - - atexit(UninitializeConsole); + initialized = InitializeSignalHandlingCore(); } } pthread_mutex_unlock(&g_lock); diff --git a/src/Native/Unix/System.Native/pal_console.h b/src/Native/Unix/System.Native/pal_console.h index 52946acc0289..d0e22f2e6943 100644 --- a/src/Native/Unix/System.Native/pal_console.h +++ b/src/Native/Unix/System.Native/pal_console.h @@ -62,7 +62,7 @@ DLLEXPORT int32_t SystemNative_IsATty(intptr_t fd); * * Returns 1 on success; otherwise returns 0 and sets errno. */ -DLLEXPORT int32_t SystemNative_InitializeConsoleAndSignalHandling(void); +DLLEXPORT int32_t SystemNative_InitializeTerminalAndSignalHandling(void); /** * Stores the string that can be written to stdout to transition @@ -90,14 +90,14 @@ DLLEXPORT void SystemNative_GetControlCharacters( DLLEXPORT int32_t SystemNative_StdinReady(void); /** - * Initializes the terminal in preparation for a read operation. + * Configures the terminal for Console operations. */ -DLLEXPORT void SystemNative_ConfigureConsoleTimeout(uint8_t minChars, uint8_t decisecondsTimeout); +DLLEXPORT void SystemNative_ConfigureTerminalForConsole(int32_t reading, uint8_t minChars, uint8_t decisecondsTimeout); /** - * Initializes the terminal for running interactive applications. + * Configures the terminal for Process operations. */ -DLLEXPORT void SystemNative_ConfigureConsoleForInteractiveChild(int32_t enable); +DLLEXPORT void SystemNative_ConfigureTerminalForChildProcess(int32_t enable); /** * Reads the number of bytes specified into the provided buffer from stdin. diff --git a/src/Native/Unix/System.Native/pal_signal.h b/src/Native/Unix/System.Native/pal_signal.h index 98d0c95b508e..24183d280d33 100644 --- a/src/Native/Unix/System.Native/pal_signal.h +++ b/src/Native/Unix/System.Native/pal_signal.h @@ -8,7 +8,7 @@ #include "pal_types.h" /** - * Initializes the signal handling, called by InitializeConsoleAndSignalHandling. + * Initializes the signal handling, called by InitializeTerminalAndSignalHandling. * * Returns 1 on success; otherwise returns 0 and sets errno. */ diff --git a/src/System.Console/src/System.Console.csproj b/src/System.Console/src/System.Console.csproj index ed8f60774827..fbfab19de7e3 100644 --- a/src/System.Console/src/System.Console.csproj +++ b/src/System.Console/src/System.Console.csproj @@ -256,8 +256,8 @@ Common\Interop\Unix\Interop.GetWindowWidth.cs - - Common\Interop\Unix\Interop.InitializeConsoleAndSignalHandling.cs + + Common\Interop\Unix\Interop.InitializeTerminalAndSignalHandling.cs Common\Interop\Unix\Interop.ReadStdinUnbuffered.cs diff --git a/src/System.Console/src/System/ConsolePal.Unix.cs b/src/System.Console/src/System/ConsolePal.Unix.cs index e931dc981b87..3d777e68c391 100644 --- a/src/System.Console/src/System/ConsolePal.Unix.cs +++ b/src/System.Console/src/System/ConsolePal.Unix.cs @@ -380,7 +380,7 @@ private static unsafe void GetCursorPosition(out int left, out int top) // involved in reading/writing, such as when accessing a remote system. We also extend // the timeout on the very first request to 15 seconds, to account for potential latency // before we know if we will receive a response. - Interop.Sys.ConfigureConsoleTimeout(minChars: (byte)(s_everReceivedCursorPositionResponse ? 1 : 0), decisecondsTimeout: (byte)(s_firstCursorPositionRequest ? 100 : 10)); + Interop.Sys.ConfigureTerminalForConsole(reading: true, minChars: (byte)(s_everReceivedCursorPositionResponse ? 1 : 0), decisecondsTimeout: (byte)(s_firstCursorPositionRequest ? 100 : 10)); try { // Write out the cursor position report request. @@ -453,7 +453,7 @@ private static unsafe void GetCursorPosition(out int left, out int top) } finally { - Interop.Sys.ConfigureConsoleTimeout(minChars: 1, decisecondsTimeout: 0); + Interop.Sys.ConfigureTerminalForConsole(reading: false); s_firstCursorPositionRequest = false; } @@ -810,7 +810,7 @@ private static void EnsureInitializedCore() if (!s_initialized) { // Setup signal handling and configure the terminal. - if (!Interop.Sys.InitializeConsoleAndSignalHandling()) + if (!Interop.Sys.InitializeTerminalAndSignalHandling()) { throw new Win32Exception(); } diff --git a/src/System.Console/src/System/IO/StdInReader.cs b/src/System.Console/src/System/IO/StdInReader.cs index ecd270c43995..d5693e4780dc 100644 --- a/src/System.Console/src/System/IO/StdInReader.cs +++ b/src/System.Console/src/System/IO/StdInReader.cs @@ -87,6 +87,7 @@ private string ReadLine(bool consumeKeys) Debug.Assert(_tmpKeys.Count == 0); string readLineStr = null; + Interop.Sys.ConfigureTerminalForConsole(reading: true); try { // Read key-by-key until we've read a line. @@ -172,6 +173,8 @@ private string ReadLine(bool consumeKeys) } finally { + Interop.Sys.ConfigureTerminalForConsole(reading: false); + // If we're not consuming the read input, make the keys available for a future read while (_tmpKeys.Count > 0) { @@ -362,31 +365,39 @@ public unsafe ConsoleKeyInfo ReadKey(out bool previouslyProcessed) char ch; bool isAlt, isCtrl, isShift; - if (IsUnprocessedBufferEmpty()) + Interop.Sys.ConfigureTerminalForConsole(reading: true); + try { - // Read in bytes - byte* bufPtr = stackalloc byte[BytesToBeRead]; - int result = ReadStdin(bufPtr, BytesToBeRead); - if (result > 0) - { - // Append them - AppendExtraBuffer(bufPtr, result); - } - else + if (IsUnprocessedBufferEmpty()) { - // Could be empty if EOL entered on its own. Pick one of the EOL characters we have, - // or just use 0 if none are available. - return new ConsoleKeyInfo((char) - (ConsolePal.s_veolCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veolCharacter : - ConsolePal.s_veol2Character != ConsolePal.s_posixDisableValue ? ConsolePal.s_veol2Character : - ConsolePal.s_veofCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veofCharacter : - 0), - default(ConsoleKey), false, false, false); + // Read in bytes + byte* bufPtr = stackalloc byte[BytesToBeRead]; + int result = ReadStdin(bufPtr, BytesToBeRead); + if (result > 0) + { + // Append them + AppendExtraBuffer(bufPtr, result); + } + else + { + // Could be empty if EOL entered on its own. Pick one of the EOL characters we have, + // or just use 0 if none are available. + return new ConsoleKeyInfo((char) + (ConsolePal.s_veolCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veolCharacter : + ConsolePal.s_veol2Character != ConsolePal.s_posixDisableValue ? ConsolePal.s_veol2Character : + ConsolePal.s_veofCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veofCharacter : + 0), + default(ConsoleKey), false, false, false); + } } - } - MapBufferToConsoleKey(out key, out ch, out isShift, out isAlt, out isCtrl); - return new ConsoleKeyInfo(ch, key, isShift, isAlt, isCtrl); + MapBufferToConsoleKey(out key, out ch, out isShift, out isAlt, out isCtrl); + return new ConsoleKeyInfo(ch, key, isShift, isAlt, isCtrl); + } + finally + { + Interop.Sys.ConfigureTerminalForConsole(reading: false); + } } /// Gets whether there's input waiting on stdin. diff --git a/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj b/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj index 79f8e9d258f6..dd2f64de620e 100644 --- a/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj +++ b/src/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj @@ -311,8 +311,8 @@ Common\Interop\Unix\Interop.SysConf.cs - - Common\Interop\Unix\Interop.ConfigureConsoleForInteractiveChild.cs + + Common\Interop\Unix\Interop.ConfigureTerminalForChildProcess.cs Common\Interop\Unix\Interop.ForkAndExecProcess.cs @@ -332,8 +332,8 @@ Common\Interop\Unix\Interop.GetSid.cs - - Common\Interop\Unix\Interop.InitializeConsoleAndSignalHandling.cs + + Common\Interop\Unix\Interop.InitializeTerminalAndSignalHandling.cs Common\Interop\Unix\Interop.Kill.cs diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index 22039b5a82ae..6ef3f4b95e5b 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -23,7 +23,7 @@ public partial class Process : IDisposable private static readonly object s_initializedGate = new object(); private static readonly Interop.Sys.SigChldCallback s_sigChildHandler = OnSigChild; private static readonly ReaderWriterLockSlim s_processStartLock = new ReaderWriterLockSlim(); - private static int s_interactiveChildProcessCount; + private static int s_childrenUsingTerminalCount; /// /// Puts a Process component in state to interact with operating system processes that run in a @@ -373,9 +373,8 @@ private bool StartCore(ProcessStartInfo startInfo) // .NET applications don't echo characters unless there is a Console.Read operation. // Unix applications expect the terminal to be in an echoing state by default. // To support processes that interact with the terminal (e.g. 'vi'), we need to configure the - // terminal to echo. We keep this configuration as long as there are interactive applications. - // Based on ProcessStartInfo we determine if the child may be an interactive application. - bool isInteractiveChild = !startInfo.RedirectStandardInput && !startInfo.RedirectStandardOutput; + // terminal to echo. We keep this configuration as long as there are children possibly using the terminal. + bool usesTerminal = !startInfo.RedirectStandardInput && !startInfo.RedirectStandardOutput; if (startInfo.UseShellExecute) { @@ -400,7 +399,7 @@ private bool StartCore(ProcessStartInfo startInfo) isExecuting = ForkAndExecProcess(filename, argv, envp, cwd, startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, setCredentials, userId, groupId, - out stdinFd, out stdoutFd, out stderrFd, isInteractiveChild, + out stdinFd, out stdoutFd, out stderrFd, usesTerminal, throwOnNoExec: false); // return false instead of throwing on ENOEXEC } @@ -413,7 +412,7 @@ private bool StartCore(ProcessStartInfo startInfo) ForkAndExecProcess(filename, argv, envp, cwd, startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, setCredentials, userId, groupId, - out stdinFd, out stdoutFd, out stderrFd, isInteractiveChild); + out stdinFd, out stdoutFd, out stderrFd, usesTerminal); } } else @@ -428,7 +427,7 @@ private bool StartCore(ProcessStartInfo startInfo) ForkAndExecProcess(filename, argv, envp, cwd, startInfo.RedirectStandardInput, startInfo.RedirectStandardOutput, startInfo.RedirectStandardError, setCredentials, userId, groupId, - out stdinFd, out stdoutFd, out stderrFd, isInteractiveChild); + out stdinFd, out stdoutFd, out stderrFd, usesTerminal); } // Configure the parent's ends of the redirection streams. @@ -463,7 +462,7 @@ private bool ForkAndExecProcess( bool redirectStdin, bool redirectStdout, bool redirectStderr, bool setCredentials, uint userId, uint groupId, out int stdinFd, out int stdoutFd, out int stderrFd, - bool isInteractiveChild, bool throwOnNoExec = true) + bool usesTerminal, bool throwOnNoExec = true) { if (string.IsNullOrEmpty(filename)) { @@ -475,9 +474,9 @@ private bool ForkAndExecProcess( s_processStartLock.EnterReadLock(); try { - if (isInteractiveChild) + if (usesTerminal) { - ConfigureConsoleForInteractiveChildren(+1); + ConfigureTerminalForChildProcesses(+1); } int childPid; @@ -498,7 +497,7 @@ private bool ForkAndExecProcess( { // Ensure we'll reap this process. // note: SetProcessId will set this if we don't set it first. - _waitStateHolder = new ProcessWaitState.Holder(childPid, isNewChild: true, isInteractiveChild); + _waitStateHolder = new ProcessWaitState.Holder(childPid, isNewChild: true, usesTerminal); // Store the child's information into this Process object. Debug.Assert(childPid >= 0); @@ -522,11 +521,11 @@ private bool ForkAndExecProcess( { s_processStartLock.ExitReadLock(); - // We failed to launch an interactive child. - if (_waitStateHolder == null && isInteractiveChild) + // We failed to launch a child that could use the terminal. + if (_waitStateHolder == null && usesTerminal) { s_processStartLock.EnterWriteLock(); - ConfigureConsoleForInteractiveChildren(-1); + ConfigureTerminalForChildProcesses(-1); s_processStartLock.ExitWriteLock(); } } @@ -973,7 +972,7 @@ private static void EnsureInitialized() if (!s_initialized) { // Setup signal handling and configure the terminal. - if (!Interop.Sys.InitializeConsoleAndSignalHandling()) + if (!Interop.Sys.InitializeTerminalAndSignalHandling()) { throw new Win32Exception(); } @@ -1000,23 +999,22 @@ private static void OnSigChild(bool reapAll) } } - internal static void ConfigureConsoleForInteractiveChildren(int increment) + internal static void ConfigureTerminalForChildProcesses(int increment) { Debug.Assert(increment != 0); - int interactiveChildrenRemaining = Interlocked.Add(ref s_interactiveChildProcessCount, increment); + int childrenUsingTerminalRemaining = Interlocked.Add(ref s_childrenUsingTerminalCount, increment); if (increment > 0) { Debug.Assert(s_processStartLock.IsReadLockHeld); - // This will noop if the console is already configured for an interactive child. - Interop.Sys.ConfigureConsoleForInteractiveChild(true); + Interop.Sys.ConfigureTerminalForChildProcess(true); } else { Debug.Assert(s_processStartLock.IsWriteLockHeld); - if (interactiveChildrenRemaining == 0) + if (childrenUsingTerminalRemaining == 0) { - Interop.Sys.ConfigureConsoleForInteractiveChild(false); + Interop.Sys.ConfigureTerminalForChildProcess(false); } } } diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs index 83bd90171c3b..1cbc557a27ca 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs @@ -55,9 +55,9 @@ internal sealed class Holder : IDisposable { internal ProcessWaitState _state; - internal Holder(int processId, bool isNewChild = false, bool isInteractiveChild = false) + internal Holder(int processId, bool isNewChild = false, bool usesTerminal = false) { - _state = ProcessWaitState.AddRef(processId, isNewChild, isInteractiveChild); + _state = ProcessWaitState.AddRef(processId, isNewChild, usesTerminal); } ~Holder() @@ -99,7 +99,7 @@ public void Dispose() /// /// The process ID for which we need wait state. /// The wait state object. - internal static ProcessWaitState AddRef(int processId, bool isNewChild, bool isInteractiveChild) + internal static ProcessWaitState AddRef(int processId, bool isNewChild, bool usesTerminal) { lock (s_childProcessWaitStates) { @@ -109,7 +109,7 @@ internal static ProcessWaitState AddRef(int processId, bool isNewChild, bool isI // When the PID is recycled for a new child, we remove the old child. s_childProcessWaitStates.Remove(processId); - pws = new ProcessWaitState(processId, isChild: true, isInteractiveChild); + pws = new ProcessWaitState(processId, isChild: true, usesTerminal); s_childProcessWaitStates.Add(processId, pws); pws._outstandingRefCount++; // For Holder pws._outstandingRefCount++; // Decremented in CheckChildren @@ -143,7 +143,7 @@ internal static ProcessWaitState AddRef(int processId, bool isNewChild, bool isI } if (pws == null) { - pws = new ProcessWaitState(processId, isChild: false, isInteractiveChild: false, exitTime); + pws = new ProcessWaitState(processId, isChild: false, usesTerminal: false, exitTime); s_processWaitStates.Add(processId, pws); } pws._outstandingRefCount++; @@ -196,8 +196,8 @@ internal void ReleaseRef() private readonly int _processId; /// Associated process is a child process. private readonly bool _isChild; - /// Associated process is an interactive child process. - private readonly bool _isInteractiveChild; + /// Associated process is a child that can use the terminal. + private readonly bool _usesTerminal; /// If a wait operation is in progress, the Task that represents it; otherwise, null. private Task _waitInProgress; @@ -218,12 +218,12 @@ internal void ReleaseRef() /// Initialize the wait state object. /// The associated process' ID. - private ProcessWaitState(int processId, bool isChild, bool isInteractiveChild, DateTime exitTime = default) + private ProcessWaitState(int processId, bool isChild, bool usesTerminal, DateTime exitTime = default) { Debug.Assert(processId >= 0); _processId = processId; _isChild = isChild; - _isInteractiveChild = isInteractiveChild; + _usesTerminal = usesTerminal; _exitTime = exitTime; } @@ -566,7 +566,7 @@ private bool TryReapChild() _exitCode = exitCode; // Update console settings before calling SetExited. - Process.ConfigureConsoleForInteractiveChildren(-1); + Process.ConfigureTerminalForChildProcesses(-1); SetExited(); From 40483a93cd8ca3ca82caa28c2b05be5b3523f745 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 4 Mar 2019 15:51:59 +0100 Subject: [PATCH 07/14] Update comments --- .../Interop.ConfigureTerminalForChildProcess.cs | 2 +- src/Native/Unix/System.Native/pal_console.c | 2 +- src/Native/Unix/System.Native/pal_console.h | 6 +++--- src/System.Console/src/System/ConsolePal.Unix.cs | 1 - .../src/System/Diagnostics/Process.Unix.cs | 5 ++--- 5 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.ConfigureTerminalForChildProcess.cs b/src/Common/src/Interop/Unix/System.Native/Interop.ConfigureTerminalForChildProcess.cs index bfe6f08ec5c3..75c116d4b8c1 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.ConfigureTerminalForChildProcess.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.ConfigureTerminalForChildProcess.cs @@ -10,6 +10,6 @@ internal static partial class Interop internal static partial class Sys { [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ConfigureTerminalForChildProcess")] - internal static extern unsafe void ConfigureTerminalForChildProcess(bool enable); + internal static extern unsafe void ConfigureTerminalForChildProcess(bool childUsesTerminal); } } diff --git a/src/Native/Unix/System.Native/pal_console.c b/src/Native/Unix/System.Native/pal_console.c index 5fc9ee0613cf..203699396dcf 100644 --- a/src/Native/Unix/System.Native/pal_console.c +++ b/src/Native/Unix/System.Native/pal_console.c @@ -249,7 +249,7 @@ void SystemNative_ConfigureTerminalForChildProcess(int32_t childUsesTerminal) ConfigureTerminal(g_signalForBreak, /* forChild */ childUsesTerminal, /* minChars */ 1, /* decisecondsTimeout */ 0, /* blockIfBackground */ false); - // Redo "Application mode" when there are no more interactive children. + // Redo "Application mode" when there are no more children using the terminal. if (!childUsesTerminal) { WriteKeypadXmit(); diff --git a/src/Native/Unix/System.Native/pal_console.h b/src/Native/Unix/System.Native/pal_console.h index d0e22f2e6943..d165f48d8617 100644 --- a/src/Native/Unix/System.Native/pal_console.h +++ b/src/Native/Unix/System.Native/pal_console.h @@ -58,7 +58,7 @@ DLLEXPORT int32_t SystemNative_GetWindowSize(WinSize* windowsSize); DLLEXPORT int32_t SystemNative_IsATty(intptr_t fd); /** - * Initializes signal handling and terminal for use by System.Console and System.Process. + * Initializes signal handling and terminal for use by System.Console and System.Diagnostics.Process. * * Returns 1 on success; otherwise returns 0 and sets errno. */ @@ -90,12 +90,12 @@ DLLEXPORT void SystemNative_GetControlCharacters( DLLEXPORT int32_t SystemNative_StdinReady(void); /** - * Configures the terminal for Console operations. + * Configures the terminal for System.Console. */ DLLEXPORT void SystemNative_ConfigureTerminalForConsole(int32_t reading, uint8_t minChars, uint8_t decisecondsTimeout); /** - * Configures the terminal for Process operations. + * Configures the terminal for child processes. */ DLLEXPORT void SystemNative_ConfigureTerminalForChildProcess(int32_t enable); diff --git a/src/System.Console/src/System/ConsolePal.Unix.cs b/src/System.Console/src/System/ConsolePal.Unix.cs index 3d777e68c391..19b18a37b4ee 100644 --- a/src/System.Console/src/System/ConsolePal.Unix.cs +++ b/src/System.Console/src/System/ConsolePal.Unix.cs @@ -809,7 +809,6 @@ private static void EnsureInitializedCore() { if (!s_initialized) { - // Setup signal handling and configure the terminal. if (!Interop.Sys.InitializeTerminalAndSignalHandling()) { throw new Win32Exception(); diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index caaf9e7b1f88..ba82475632ca 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -978,7 +978,6 @@ private static void EnsureInitialized() { if (!s_initialized) { - // Setup signal handling and configure the terminal. if (!Interop.Sys.InitializeTerminalAndSignalHandling()) { throw new Win32Exception(); @@ -1014,14 +1013,14 @@ internal static void ConfigureTerminalForChildProcesses(int increment) if (increment > 0) { Debug.Assert(s_processStartLock.IsReadLockHeld); - Interop.Sys.ConfigureTerminalForChildProcess(true); + Interop.Sys.ConfigureTerminalForChildProcess(childUsesTerminal: true); } else { Debug.Assert(s_processStartLock.IsWriteLockHeld); if (childrenUsingTerminalRemaining == 0) { - Interop.Sys.ConfigureTerminalForChildProcess(false); + Interop.Sys.ConfigureTerminalForChildProcess(childUsesTerminal: false); } } } From 9d3a3e1f7158000b4856dca565e8f96fce44f638 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 4 Mar 2019 16:04:14 +0100 Subject: [PATCH 08/14] Rename some 'console' to 'terminal' --- src/Native/Unix/System.Native/pal_console.c | 16 ++++++++-------- src/Native/Unix/System.Native/pal_console.h | 4 ++-- src/Native/Unix/System.Native/pal_signal.c | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_console.c b/src/Native/Unix/System.Native/pal_console.c index 203699396dcf..3140e4d5415b 100644 --- a/src/Native/Unix/System.Native/pal_console.c +++ b/src/Native/Unix/System.Native/pal_console.c @@ -91,7 +91,7 @@ static struct termios g_currentTermios; // the latest attributes set // we prefer configuring for the Console. static bool g_reading = false; // tracks whether the application is performing a Console.Read operation static bool g_childUsesTerminal = false; // tracks whether a child process is using the terminal -static bool g_consoleUninitialized = false; // tracks whether the application is terminating +static bool g_terminalUninitialized = false; // tracks whether the application is terminating static bool g_noTty = false; // cache we are not a tty @@ -115,7 +115,7 @@ static void InstallTTOUHandler(void (*handler)(int)) static bool TcSetAttr(struct termios* termios, bool blockIfBackground) { - if (g_consoleUninitialized) + if (g_terminalUninitialized) { // The application is exiting, we mustn't change terminal settings. return true; @@ -197,7 +197,7 @@ static bool ConfigureTerminal(bool signalForBreak, bool forChild, uint8_t minCha return TcSetAttr(&termios, blockIfBackground); } -void UninitializeConsole() +void UninitializeTerminal() { assert(g_haveInitTermios); @@ -206,11 +206,11 @@ void UninitializeConsole() if (pthread_mutex_lock(&g_lock) == 0) { - if (!g_consoleUninitialized) + if (!g_terminalUninitialized) { TcSetAttr(&g_initTermios, /* blockIfBackground */ false); - g_consoleUninitialized = true; + g_terminalUninitialized = true; } pthread_mutex_unlock(&g_lock); @@ -404,9 +404,9 @@ int32_t SystemNative_SetSignalForBreak(int32_t signalForBreak) return rv; } -void ReinitializeConsole() +void ReinitializeTerminal() { - // Restores the state of the console after being suspended. + // Restores the state of the terminal after being suspended. // pal_signal.cpp calls this on SIGCONT from the signal handling thread. if (pthread_mutex_lock(&g_lock) == 0) @@ -440,7 +440,7 @@ static bool InitializeTerminalCore() g_currentTermios = g_initTermios; g_signalForBreak = g_initTermios.c_lflag & (uint32_t)ISIG; - atexit(UninitializeConsole); + atexit(UninitializeTerminal); } else { diff --git a/src/Native/Unix/System.Native/pal_console.h b/src/Native/Unix/System.Native/pal_console.h index d165f48d8617..7c8d0de77a53 100644 --- a/src/Native/Unix/System.Native/pal_console.h +++ b/src/Native/Unix/System.Native/pal_console.h @@ -129,9 +129,9 @@ typedef void (*CtrlCallback)(CtrlCode signalCode); /** * Called by pal_signal.cpp to reinitialize the console on SIGCONT/SIGCHLD. */ -void ReinitializeConsole(void); +void ReinitializeTerminal(void); /** * Called by pal_signal.cpp to uninitialize the console on SIGINT/SIGQUIT. */ -void UninitializeConsole(void); +void UninitializeTerminal(void); diff --git a/src/Native/Unix/System.Native/pal_signal.c b/src/Native/Unix/System.Native/pal_signal.c index 9390492e0d47..ab38a7343f93 100644 --- a/src/Native/Unix/System.Native/pal_signal.c +++ b/src/Native/Unix/System.Native/pal_signal.c @@ -143,7 +143,7 @@ static void* SignalHandlerLoop(void* arg) } else if (signalCode == SIGCONT) { - ReinitializeConsole(); + ReinitializeTerminal(); } else { @@ -178,7 +178,7 @@ void SystemNative_UnregisterForCtrl() void SystemNative_RestoreAndHandleCtrl(CtrlCode ctrlCode) { int signalCode = ctrlCode == Break ? SIGQUIT : SIGINT; - UninitializeConsole(); + UninitializeTerminal(); sigaction(signalCode, OrigActionFor(signalCode), NULL); kill(getpid(), signalCode); } From e34e71a2f39e836f11acc31aa38943e061fb4fc7 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 4 Mar 2019 17:18:52 +0100 Subject: [PATCH 09/14] Only decrement when child is using terminal --- .../src/System/Diagnostics/ProcessWaitState.Unix.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs index 1cbc557a27ca..4b90e1930389 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs @@ -565,8 +565,11 @@ private bool TryReapChild() { _exitCode = exitCode; - // Update console settings before calling SetExited. - Process.ConfigureTerminalForChildProcesses(-1); + if (_usesTerminal) + { + // Update terminal settings before calling SetExited. + Process.ConfigureTerminalForChildProcesses(-1); + } SetExited(); From 0aa3562ec303b96aa810e73d7d4306ec5cd69a7a Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 5 Mar 2019 14:14:46 +0100 Subject: [PATCH 10/14] Rename ConfigureTerminalForConsole back to In/UninitializeConsoleBefore/AfterRead --- .../Interop.ReadStdinUnbuffered.cs | 7 +++++-- src/Native/Unix/System.Native/pal_console.c | 18 ++++++++++++++---- src/Native/Unix/System.Native/pal_console.h | 9 +++++++-- .../src/System/ConsolePal.Unix.cs | 4 ++-- .../src/System/IO/StdInReader.cs | 14 +++++++------- 5 files changed, 35 insertions(+), 17 deletions(-) diff --git a/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs b/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs index 9fd3cb0ef4cb..1b3b0ac7ea51 100644 --- a/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs +++ b/src/Common/src/Interop/Unix/System.Native/Interop.ReadStdinUnbuffered.cs @@ -12,7 +12,10 @@ internal static partial class Sys [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ReadStdin", SetLastError = true)] internal static extern unsafe int ReadStdin(byte* buffer, int bufferSize); - [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ConfigureTerminalForConsole")] - internal static extern unsafe void ConfigureTerminalForConsole(bool reading, byte minChars = 1, byte decisecondsTimeout = 0); + [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_InitializeConsoleBeforeRead")] + internal static extern unsafe void InitializeConsoleBeforeRead(byte minChars = 1, byte decisecondsTimeout = 0); + + [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_UninitializeConsoleAfterRead")] + internal static extern unsafe void UninitializeConsoleAfterRead(); } } diff --git a/src/Native/Unix/System.Native/pal_console.c b/src/Native/Unix/System.Native/pal_console.c index 3140e4d5415b..0b8c3fd84e39 100644 --- a/src/Native/Unix/System.Native/pal_console.c +++ b/src/Native/Unix/System.Native/pal_console.c @@ -217,11 +217,11 @@ void UninitializeTerminal() } } -void SystemNative_ConfigureTerminalForConsole(int32_t reading, uint8_t minChars, uint8_t decisecondsTimeout) +void SystemNative_InitializeConsoleBeforeRead(uint8_t minChars, uint8_t decisecondsTimeout) { if (pthread_mutex_lock(&g_lock) == 0) { - g_reading = reading; + g_reading = true; ConfigureTerminal(g_signalForBreak, /* forChild */ false, minChars, decisecondsTimeout, /* blockIfBackground */ true); @@ -229,6 +229,16 @@ void SystemNative_ConfigureTerminalForConsole(int32_t reading, uint8_t minChars, } } +void SystemNative_UninitializeConsoleAfterRead() +{ + if (pthread_mutex_lock(&g_lock) == 0) + { + g_reading = false; + + pthread_mutex_unlock(&g_lock); + } +} + void SystemNative_ConfigureTerminalForChildProcess(int32_t childUsesTerminal) { assert(childUsesTerminal == 0 || childUsesTerminal == 1); @@ -356,10 +366,10 @@ void SystemNative_GetControlCharacters( int32_t SystemNative_StdinReady() { - SystemNative_ConfigureTerminalForConsole(1, 1, 0); + SystemNative_InitializeConsoleBeforeRead(1, 0); struct pollfd fd = { .fd = STDIN_FILENO, .events = POLLIN }; int rv = poll(&fd, 1, 0) > 0 ? 1 : 0; - SystemNative_ConfigureTerminalForConsole(0, 1, 0); + SystemNative_UninitializeConsoleAfterRead(); return rv; } diff --git a/src/Native/Unix/System.Native/pal_console.h b/src/Native/Unix/System.Native/pal_console.h index 7c8d0de77a53..f5b59c04ca5e 100644 --- a/src/Native/Unix/System.Native/pal_console.h +++ b/src/Native/Unix/System.Native/pal_console.h @@ -90,9 +90,14 @@ DLLEXPORT void SystemNative_GetControlCharacters( DLLEXPORT int32_t SystemNative_StdinReady(void); /** - * Configures the terminal for System.Console. + * Configures the terminal for System.Console Read. */ -DLLEXPORT void SystemNative_ConfigureTerminalForConsole(int32_t reading, uint8_t minChars, uint8_t decisecondsTimeout); +DLLEXPORT void SystemNative_InitializeConsoleBeforeRead(uint8_t minChars, uint8_t decisecondsTimeout); + +/** + * Configures the terminal after System.Console Read. + */ +DLLEXPORT void SystemNative_UninitializeConsoleAfterRead(void); /** * Configures the terminal for child processes. diff --git a/src/System.Console/src/System/ConsolePal.Unix.cs b/src/System.Console/src/System/ConsolePal.Unix.cs index 19b18a37b4ee..652e902702eb 100644 --- a/src/System.Console/src/System/ConsolePal.Unix.cs +++ b/src/System.Console/src/System/ConsolePal.Unix.cs @@ -380,7 +380,7 @@ private static unsafe void GetCursorPosition(out int left, out int top) // involved in reading/writing, such as when accessing a remote system. We also extend // the timeout on the very first request to 15 seconds, to account for potential latency // before we know if we will receive a response. - Interop.Sys.ConfigureTerminalForConsole(reading: true, minChars: (byte)(s_everReceivedCursorPositionResponse ? 1 : 0), decisecondsTimeout: (byte)(s_firstCursorPositionRequest ? 100 : 10)); + Interop.Sys.InitializeConsoleBeforeRead(minChars: (byte)(s_everReceivedCursorPositionResponse ? 1 : 0), decisecondsTimeout: (byte)(s_firstCursorPositionRequest ? 100 : 10)); try { // Write out the cursor position report request. @@ -453,7 +453,7 @@ private static unsafe void GetCursorPosition(out int left, out int top) } finally { - Interop.Sys.ConfigureTerminalForConsole(reading: false); + Interop.Sys.UninitializeConsoleAfterRead(); s_firstCursorPositionRequest = false; } diff --git a/src/System.Console/src/System/IO/StdInReader.cs b/src/System.Console/src/System/IO/StdInReader.cs index d5693e4780dc..2431ecefd902 100644 --- a/src/System.Console/src/System/IO/StdInReader.cs +++ b/src/System.Console/src/System/IO/StdInReader.cs @@ -87,7 +87,7 @@ private string ReadLine(bool consumeKeys) Debug.Assert(_tmpKeys.Count == 0); string readLineStr = null; - Interop.Sys.ConfigureTerminalForConsole(reading: true); + Interop.Sys.InitializeConsoleBeforeRead(); try { // Read key-by-key until we've read a line. @@ -173,7 +173,7 @@ private string ReadLine(bool consumeKeys) } finally { - Interop.Sys.ConfigureTerminalForConsole(reading: false); + Interop.Sys.UninitializeConsoleAfterRead(); // If we're not consuming the read input, make the keys available for a future read while (_tmpKeys.Count > 0) @@ -361,11 +361,11 @@ public unsafe ConsoleKeyInfo ReadKey(out bool previouslyProcessed) } previouslyProcessed = false; + Interop.Sys.InitializeConsoleBeforeRead(); ConsoleKey key; char ch; bool isAlt, isCtrl, isShift; - Interop.Sys.ConfigureTerminalForConsole(reading: true); try { if (IsUnprocessedBufferEmpty()) @@ -384,9 +384,9 @@ public unsafe ConsoleKeyInfo ReadKey(out bool previouslyProcessed) // or just use 0 if none are available. return new ConsoleKeyInfo((char) (ConsolePal.s_veolCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veolCharacter : - ConsolePal.s_veol2Character != ConsolePal.s_posixDisableValue ? ConsolePal.s_veol2Character : - ConsolePal.s_veofCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veofCharacter : - 0), + ConsolePal.s_veol2Character != ConsolePal.s_posixDisableValue ? ConsolePal.s_veol2Character : + ConsolePal.s_veofCharacter != ConsolePal.s_posixDisableValue ? ConsolePal.s_veofCharacter : + 0), default(ConsoleKey), false, false, false); } } @@ -396,7 +396,7 @@ public unsafe ConsoleKeyInfo ReadKey(out bool previouslyProcessed) } finally { - Interop.Sys.ConfigureTerminalForConsole(reading: false); + Interop.Sys.UninitializeConsoleAfterRead(); } } From 64ab6811663c5b4c64a8191a5bee9e91f909a4ed Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Thu, 7 Mar 2019 11:02:34 +0100 Subject: [PATCH 11/14] Move some code to original place --- src/System.Console/src/System/IO/StdInReader.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/System.Console/src/System/IO/StdInReader.cs b/src/System.Console/src/System/IO/StdInReader.cs index 2431ecefd902..bc8bee2d700c 100644 --- a/src/System.Console/src/System/IO/StdInReader.cs +++ b/src/System.Console/src/System/IO/StdInReader.cs @@ -362,12 +362,12 @@ public unsafe ConsoleKeyInfo ReadKey(out bool previouslyProcessed) previouslyProcessed = false; Interop.Sys.InitializeConsoleBeforeRead(); - ConsoleKey key; - char ch; - bool isAlt, isCtrl, isShift; - try { + ConsoleKey key; + char ch; + bool isAlt, isCtrl, isShift; + if (IsUnprocessedBufferEmpty()) { // Read in bytes From 8065fe74055436cb8b52b25822ab5dd95a3d9b6a Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 11 Mar 2019 13:32:00 +0100 Subject: [PATCH 12/14] Add some extra comments --- src/Native/Unix/System.Native/pal_console.c | 3 ++- .../src/System/Diagnostics/Process.Unix.cs | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Native/Unix/System.Native/pal_console.c b/src/Native/Unix/System.Native/pal_console.c index 0b8c3fd84e39..86fc26449676 100644 --- a/src/Native/Unix/System.Native/pal_console.c +++ b/src/Native/Unix/System.Native/pal_console.c @@ -73,7 +73,7 @@ void SystemNative_SetKeypadXmit(const char* terminfoString) WriteKeypadXmit(); } -static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER; +static pthread_mutex_t g_lock = PTHREAD_MUTEX_INITIALIZER; // prevents races when initializing and changing the terminal. static bool g_signalForBreak = true; // tracks whether the terminal should send signals for breaks, such that attributes have been changed @@ -146,6 +146,7 @@ static bool TcSetAttr(struct termios* termios, bool blockIfBackground) InstallTTOUHandler(SIG_DFL); } + // On success, update the cached value. if (rv) { g_hasCurrentTermios = true; diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index ba82475632ca..0dddd1854965 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -1005,6 +1005,10 @@ private static void OnSigChild(bool reapAll) } } + /// + /// This method is called when the number of child processes that are using the terminal changes. + /// It updates the terminal configuration if necessary. + /// internal static void ConfigureTerminalForChildProcesses(int increment) { Debug.Assert(increment != 0); @@ -1013,13 +1017,17 @@ internal static void ConfigureTerminalForChildProcesses(int increment) if (increment > 0) { Debug.Assert(s_processStartLock.IsReadLockHeld); + + // At least one child is using the terminal. Interop.Sys.ConfigureTerminalForChildProcess(childUsesTerminal: true); } else { Debug.Assert(s_processStartLock.IsWriteLockHeld); + if (childrenUsingTerminalRemaining == 0) { + // No more children are using the terminal. Interop.Sys.ConfigureTerminalForChildProcess(childUsesTerminal: false); } } From 7341b167ec7d9bf33f66daee8fc4258c971f68dd Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 12 Mar 2019 08:30:15 +0100 Subject: [PATCH 13/14] PR feedback --- src/Native/Unix/System.Native/pal_console.c | 17 +++++++++++------ .../src/System/Diagnostics/Process.Unix.cs | 5 +++-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/Native/Unix/System.Native/pal_console.c b/src/Native/Unix/System.Native/pal_console.c index 86fc26449676..95fffe16e396 100644 --- a/src/Native/Unix/System.Native/pal_console.c +++ b/src/Native/Unix/System.Native/pal_console.c @@ -84,8 +84,8 @@ static bool g_hasCurrentTermios = false; // tracks whether g_currentTermios static struct termios g_currentTermios; // the latest attributes set // The terminal can be used by the .NET application via the Console class. -// It may also be used by child processes that is started via the Process class. -// The terminal needs to be configured different depending on the user. +// It may also be used by child processes that are started via the Process class. +// The terminal needs to be configured differently depending on the user. // ConfigureTerminalForXXX are called to change the configuration. // When it is ambiguous whether we should configure for Console/a child Process, // we prefer configuring for the Console. @@ -103,11 +103,12 @@ static void ttou_handler(int signo) g_receivedSigTtou = true; } -static void InstallTTOUHandler(void (*handler)(int)) +static void InstallTTOUHandler(void (*handler)(int), int flags) { struct sigaction action; memset(&action, 0, sizeof(action)); action.sa_handler = handler; + action.sa_flags = flags; int rvSigaction = sigaction(SIGTTOU, &action, NULL); assert(rvSigaction == 0); (void)rvSigaction; @@ -125,8 +126,12 @@ static bool TcSetAttr(struct termios* termios, bool blockIfBackground) { // When the process is running in background, changing terminal settings // will stop it (default SIGTTOU action). - // We change SIGTTOU to to get EINTR instead of blocking. - InstallTTOUHandler(ttou_handler); + // We change SIGTTOU's disposition to get EINTR instead. + // This thread may be used to run a signal handler, which may write to + // stdout. We set SA_RESETHAND to avoid that handler's write infinitly looping + // on EINTR when the process is running in background and the terminal + // configured with TOSTOP. + InstallTTOUHandler(ttou_handler, (int)SA_RESETHAND); g_receivedSigTtou = false; } @@ -143,7 +148,7 @@ static bool TcSetAttr(struct termios* termios, bool blockIfBackground) } // Restore default SIGTTOU handler. - InstallTTOUHandler(SIG_DFL); + InstallTTOUHandler(SIG_DFL, 0); } // On success, update the cached value. diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index 0dddd1854965..95707804feb7 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -375,6 +375,7 @@ private bool StartCore(ProcessStartInfo startInfo) // Unix applications expect the terminal to be in an echoing state by default. // To support processes that interact with the terminal (e.g. 'vi'), we need to configure the // terminal to echo. We keep this configuration as long as there are children possibly using the terminal. + // We consider the child to be interactively using the terminal when both stdin and stdout are connected. bool usesTerminal = !startInfo.RedirectStandardInput && !startInfo.RedirectStandardOutput; if (startInfo.UseShellExecute) @@ -477,7 +478,7 @@ private bool ForkAndExecProcess( { if (usesTerminal) { - ConfigureTerminalForChildProcesses(+1); + ConfigureTerminalForChildProcesses(1); } int childPid; @@ -522,9 +523,9 @@ private bool ForkAndExecProcess( { s_processStartLock.ExitReadLock(); - // We failed to launch a child that could use the terminal. if (_waitStateHolder == null && usesTerminal) { + // We failed to launch a child that could use the terminal. s_processStartLock.EnterWriteLock(); ConfigureTerminalForChildProcesses(-1); s_processStartLock.ExitWriteLock(); From fb72e6359a55098377728bbfe51584c2955b5487 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 12 Mar 2019 09:23:59 +0100 Subject: [PATCH 14/14] typo --- src/Native/Unix/System.Native/pal_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Native/Unix/System.Native/pal_console.c b/src/Native/Unix/System.Native/pal_console.c index 95fffe16e396..0c1250b5ab1b 100644 --- a/src/Native/Unix/System.Native/pal_console.c +++ b/src/Native/Unix/System.Native/pal_console.c @@ -128,7 +128,7 @@ static bool TcSetAttr(struct termios* termios, bool blockIfBackground) // will stop it (default SIGTTOU action). // We change SIGTTOU's disposition to get EINTR instead. // This thread may be used to run a signal handler, which may write to - // stdout. We set SA_RESETHAND to avoid that handler's write infinitly looping + // stdout. We set SA_RESETHAND to avoid that handler's write loops infinitly // on EINTR when the process is running in background and the terminal // configured with TOSTOP. InstallTTOUHandler(ttou_handler, (int)SA_RESETHAND);