Console.Unix: avoid deadlock between LazyInitializer and Console.Out

src/libraries/System.Console/src/System/Console.cs

@@ -22,7 +22,7 @@ public static class Console
         // there's little benefit to having a large buffer.  So we use a smaller buffer size to reduce working set.
         private const int WriteBufferSize = 256;
 
-        private static object InternalSyncObject = new object(); // for synchronizing changing of Console's static fields
+        private static readonly object s_syncObject = new object();
         private static TextReader? s_in;
         private static TextWriter? s_out, s_error;
         private static Encoding? s_inputEncoding;
@@ -33,31 +33,57 @@ public static class Console
         private static ConsoleCancelEventHandler? s_cancelCallbacks;
         private static ConsolePal.ControlCHandlerRegistrar? s_registrar;
 
-        internal static T EnsureInitialized<T>([NotNull] ref T? field, Func<T> initializer) where T : class =>
-            LazyInitializer.EnsureInitialized(ref field, ref InternalSyncObject, initializer);
+        public static TextReader In
+        {
+            get
+            {
+                return Volatile.Read(ref s_in) ?? EnsureInitialized();
 
-        public static TextReader In => EnsureInitialized(ref s_in, () => ConsolePal.GetOrCreateReader());
+                static TextReader EnsureInitialized()
+                {
+                    // EnsureConsoleInitialized may lock Console.Out.
+                    // We musn't call it under s_syncObject lock,
+                    // otherwise there can be a deadlock when another
+                    // thread locks these objects in opposite order.
+                    ConsolePal.EnsureConsoleInitialized();
+
+                    lock (s_syncObject) // Ensures In and InputEncoding are synchronized.
+                    {
+                        return s_in ??= ConsolePal.GetOrCreateReader();
+                    }
+                }
+            }
+        }
 
         public static Encoding InputEncoding
         {
             get
             {
-                return EnsureInitialized(ref s_inputEncoding, () => ConsolePal.InputEncoding);
+                Encoding? encoding = Volatile.Read(ref s_inputEncoding);
+                if (encoding == null)
+                {
+                    lock (s_syncObject)
+                    {
+                        encoding = s_inputEncoding ??= ConsolePal.InputEncoding;
+                    }
+                }
+                return encoding;
             }
             set
             {
                 CheckNonNull(value, nameof(value));
-                lock (InternalSyncObject)
+
+                lock (s_syncObject)
                 {
                     // Set the terminal console encoding.
                     ConsolePal.SetConsoleInputEncoding(value);
 
-                    Volatile.Write(ref s_inputEncoding, (Encoding)value.Clone());
+                    s_inputEncoding = (Encoding)value.Clone();
 
                     // We need to reinitialize 'Console.In' in the next call to s_in
                     // This will discard the current StreamReader, potentially
                     // losing buffered data.
-                    Volatile.Write(ref s_in, null);
+                    s_in = null;
                 }
             }
         }
@@ -66,32 +92,40 @@ public static Encoding OutputEncoding
         {
             get
             {
-                return EnsureInitialized(ref s_outputEncoding, () => ConsolePal.OutputEncoding);
+                Encoding? encoding = Volatile.Read(ref s_outputEncoding);
+                if (encoding == null)
+                {
+                    lock (s_syncObject)
+                    {
+                        encoding = s_outputEncoding ??= ConsolePal.OutputEncoding;
+                    }
+                }
+                return encoding;
             }
             set
             {
                 CheckNonNull(value, nameof(value));
 
-                lock (InternalSyncObject)
+                lock (s_syncObject)
                 {
                     // Set the terminal console encoding.
                     ConsolePal.SetConsoleOutputEncoding(value);
 
                     // Before changing the code page we need to flush the data
                     // if Out hasn't been redirected. Also, have the next call to
                     // s_out reinitialize the console code page.
-                    if (Volatile.Read(ref s_out) != null && !s_isOutTextWriterRedirected)
+                    if (s_out != null && !s_isOutTextWriterRedirected)
                     {
-                        s_out!.Flush();
-                        Volatile.Write(ref s_out, null);
+                        s_out.Flush();
+                        s_out = null;
                     }
-                    if (Volatile.Read(ref s_error) != null && !s_isErrorTextWriterRedirected)
+                    if (s_error != null && !s_isErrorTextWriterRedirected)
                     {
-                        s_error!.Flush();
-                        Volatile.Write(ref s_error, null);
+                        s_error.Flush();
+                        s_error = null;
                     }
 
-                    Volatile.Write(ref s_outputEncoding, (Encoding)value.Clone());
+                    s_outputEncoding = (Encoding)value.Clone();
                 }
             }
         }
@@ -119,9 +153,37 @@ public static ConsoleKeyInfo ReadKey(bool intercept)
             return ConsolePal.ReadKey(intercept);
         }
 
-        public static TextWriter Out => EnsureInitialized(ref s_out, () => CreateOutputWriter(OpenStandardOutput()));
+        public static TextWriter Out
+        {
+            get
+            {
+                return Volatile.Read(ref s_out) ?? EnsureInitialized();
+
+                static TextWriter EnsureInitialized()
+                {
+                    lock (s_syncObject) // Ensures Out and OutputEncoding are synchronized.
+                    {
+                        return s_out ??= CreateOutputWriter(OpenStandardOutput());
+                    }
+                }
+            }
+        }
+
+        public static TextWriter Error
+        {
+            get
+            {
+                return Volatile.Read(ref s_error) ?? EnsureInitialized();
 
-        public static TextWriter Error => EnsureInitialized(ref s_error, () => CreateOutputWriter(OpenStandardError()));
+                static TextWriter EnsureInitialized()
+                {
+                    lock (s_syncObject) // Ensures Error and OutputEncoding are synchronized.
+                    {
+                        return s_error ??= CreateOutputWriter(OpenStandardError());
+                    }
+                }
+            }
+        }
 
         private static TextWriter CreateOutputWriter(Stream outputStream)
         {
@@ -145,26 +207,44 @@ public static bool IsInputRedirected
         {
             get
             {
-                StrongBox<bool> redirected = EnsureInitialized(ref _isStdInRedirected, () => new StrongBox<bool>(ConsolePal.IsInputRedirectedCore()));
+                StrongBox<bool> redirected = Volatile.Read(ref _isStdInRedirected) ?? EnsureInitialized();
                 return redirected.Value;
+
+                static StrongBox<bool> EnsureInitialized()
+                {
+                    Volatile.Write(ref _isStdInRedirected, new StrongBox<bool>(ConsolePal.IsInputRedirectedCore()));
+                    return _isStdInRedirected!;
+                }
             }
         }
 
         public static bool IsOutputRedirected
         {
             get
             {
-                StrongBox<bool> redirected = EnsureInitialized(ref _isStdOutRedirected, () => new StrongBox<bool>(ConsolePal.IsOutputRedirectedCore()));
+                StrongBox<bool> redirected = Volatile.Read(ref _isStdOutRedirected) ?? EnsureInitialized();
                 return redirected.Value;
+
+                static StrongBox<bool> EnsureInitialized()
+                {
+                    Volatile.Write(ref _isStdOutRedirected, new StrongBox<bool>(ConsolePal.IsOutputRedirectedCore()));
+                    return _isStdOutRedirected!;
+                }
             }
         }
 
         public static bool IsErrorRedirected
         {
             get
             {
-                StrongBox<bool> redirected = EnsureInitialized(ref _isStdErrRedirected, () => new StrongBox<bool>(ConsolePal.IsErrorRedirectedCore()));
+                StrongBox<bool> redirected = Volatile.Read(ref _isStdErrRedirected) ?? EnsureInitialized();
                 return redirected.Value;
+
+                static StrongBox<bool> EnsureInitialized()
+                {
+                    Volatile.Write(ref _isStdErrRedirected, new StrongBox<bool>(ConsolePal.IsErrorRedirectedCore()));
+                    return _isStdErrRedirected!;
+                }
             }
         }
 
@@ -331,7 +411,7 @@ public static event ConsoleCancelEventHandler? CancelKeyPress
         {
             add
             {
-                lock (InternalSyncObject)
+                lock (s_syncObject)
                 {
                     s_cancelCallbacks += value;
 
@@ -345,7 +425,7 @@ public static event ConsoleCancelEventHandler? CancelKeyPress
             }
             remove
             {
-                lock (InternalSyncObject)
+                lock (s_syncObject)
                 {
                     s_cancelCallbacks -= value;
                     if (s_registrar != null && s_cancelCallbacks == null)
@@ -412,33 +492,28 @@ public static void SetIn(TextReader newIn)
         {
             CheckNonNull(newIn, nameof(newIn));
             newIn = SyncTextReader.GetSynchronizedTextReader(newIn);
-            lock (InternalSyncObject)
-            {
-                Volatile.Write(ref s_in, newIn);
-            }
+            Volatile.Write(ref s_in, newIn);
         }
 
         public static void SetOut(TextWriter newOut)
         {
             CheckNonNull(newOut, nameof(newOut));
             newOut = TextWriter.Synchronized(newOut);
-            Volatile.Write(ref s_isOutTextWriterRedirected, true);
-
-            lock (InternalSyncObject)
+            lock (s_syncObject)
             {
-                Volatile.Write(ref s_out, newOut);
+                s_isOutTextWriterRedirected = true;
+                s_out = newOut;
             }
         }
 
         public static void SetError(TextWriter newError)
         {
             CheckNonNull(newError, nameof(newError));
             newError = TextWriter.Synchronized(newError);
-            Volatile.Write(ref s_isErrorTextWriterRedirected, true);
-
-            lock (InternalSyncObject)
+            lock (s_syncObject)
             {
-                Volatile.Write(ref s_error, newError);
+                s_isErrorTextWriterRedirected = true;
+                s_error = newError;
             }
         }
 

src/libraries/System.Console/src/System/ConsolePal.Unix.cs

@@ -73,14 +73,22 @@ private static SyncTextReader StdInReader
         {
             get
             {
-                EnsureInitialized();
-
-                return Console.EnsureInitialized(
-                        ref s_stdInReader,
-                        () => SyncTextReader.GetSynchronizedTextReader(
-                            new StdInReader(
-                                encoding: Console.InputEncoding,
-                                bufferSize: InteractiveBufferSize)));
+                return Volatile.Read(ref s_stdInReader) ?? EnsureInitialized();
+
+                static SyncTextReader EnsureInitialized()
+                {
+                    EnsureConsoleInitialized();
+
+                    SyncTextReader reader = SyncTextReader.GetSynchronizedTextReader(
+                                                new StdInReader(
+                                                    encoding: Console.InputEncoding,
+                                                    bufferSize: InteractiveBufferSize));
+
+                    // Don't overwrite a set reader.
+                    Interlocked.CompareExchange(ref s_stdInReader, reader, null);
+
+                    return s_stdInReader;
+                }
             }
         }
 
@@ -97,8 +105,7 @@ internal static TextReader GetOrCreateReader()
                         encoding: Console.InputEncoding,
                         detectEncodingFromByteOrderMarks: false,
                         bufferSize: Console.ReadBufferSize,
-                        leaveOpen: true)
-                        );
+                        leaveOpen: true));
             }
             else
             {
@@ -143,14 +150,14 @@ public static bool TreatControlCAsInput
                 if (Console.IsInputRedirected)
                     return false;
 
-                EnsureInitialized();
+                EnsureConsoleInitialized();
                 return !Interop.Sys.GetSignalForBreak();
             }
             set
             {
                 if (!Console.IsInputRedirected)
                 {
-                    EnsureInitialized();
+                    EnsureConsoleInitialized();
                     if (!Interop.Sys.SetSignalForBreak(signalForBreak: !value))
                         throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo());
                 }
@@ -917,7 +924,7 @@ public static bool TryGetSpecialConsoleKey(char[] givenChars, int startIndex, in
         internal static byte s_veofCharacter;
 
         /// <summary>Ensures that the console has been initialized for use.</summary>
-        private static void EnsureInitialized()
+        internal static void EnsureConsoleInitialized()
         {
             if (!s_initialized)
             {
@@ -928,6 +935,13 @@ private static void EnsureInitialized()
         /// <summary>Ensures that the console has been initialized for use.</summary>
         private static void EnsureInitializedCore()
         {
+            // Initialization is only needed when input isn't redirected.
+            if (Console.IsInputRedirected)
+            {
+                s_initialized = true;
+                return;
+            }
+
             lock (Console.Out) // ensure that writing the ANSI string and setting initialized to true are done atomically
             {
                 if (!s_initialized)
@@ -1460,7 +1474,7 @@ internal sealed class ControlCHandlerRegistrar
 
             internal void Register()
             {
-                EnsureInitialized();
+                EnsureConsoleInitialized();
 
                 Debug.Assert(!_handlerRegistered);
                 Interop.Sys.RegisterForCtrl(c => OnBreakEvent(c));

src/libraries/System.Console/src/System/ConsolePal.Windows.cs

@@ -14,6 +14,10 @@ internal static class ConsolePal
     {
         private static IntPtr InvalidHandleValue => new IntPtr(-1);
 
+        /// <summary>Ensures that the console has been initialized for use.</summary>
+        internal static void EnsureConsoleInitialized()
+        { }
+
         private static bool IsWindows7()
         {
             // Version lies for all apps from the OS kick in starting with Windows 8 (6.2). They can

src/libraries/System.Console/src/System/ConsolePal.iOS.cs

@@ -26,6 +26,9 @@ public override unsafe void Write(byte[] buffer, int offset, int count)
 
     internal static class ConsolePal
     {
+        internal static void EnsureConsoleInitialized()
+        { }
+
         public static Stream OpenStandardInput() => throw new PlatformNotSupportedException();
 
         public static Stream OpenStandardOutput() => new NSLogStream();