Skip to content

Commit

Permalink
Console.Unix: avoid deadlock between LazyInitializer and Console.Out (#…
Browse files Browse the repository at this point in the history 
…34297)

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

* Add assert for InternalSyncObject

* Console: use Interlocked for lazy initialization

* Cleanup unused 'using'

* Limit locking to event handlers

* Fix AllowNull -> NotNull

* Localize LazyInitializer.EnsureInitialized logic

* Remove lazy initialize delegates

* Use static local functions for initializing

* Remove unused EnsureInitializedDisposableCore

* Remove unused arg from EnsureInitializedStdInReader

* Also EnsureInitialize when not redirected

* Dispose Console Streams with their Reader/Writer

* Fix s_out -> s_error

* leaveOpen streams, fix Encoding set race

* Fix races between Encoding en initializers of In/Out/Error

* Remove EnsureInitialized

* Cleanup

* Rename static local initializer functions to EnsureInitialized

* Use lock in Input/OutputEncoding

* Rename InternalSyncObject to s_syncObject

* Cleanup space

* Fix _isStdErrRedirected -> _isStdInRedirected

* Remove space

* Improve comments

* Fix potential CancelKeyPress deadlock

* Apply suggestions from code review

Co-Authored-By: Stephen Toub <stoub@microsoft.com>

* Remove unnecessary null forgiving

* Add comment about why we're not Disposing StdInReader

* Add Volatile.Writes for Volatile.Read fields

* More Volatile.Writes

* Add back lock to SetIn

Co-authored-by: Stephen Toub <stoub@microsoft.com>
  • Loading branch information
@tmds @stephentoub
tmds and stephentoub authored Apr 13, 2020
1 parent 2bcc336 commit 59a24e5
Show file tree
Hide file tree
Showing 4 changed files with 171 additions and 40 deletions.
160 changes: 134 additions & 26 deletions src/libraries/System.Console/src/System/Console.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// 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.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Runtime.CompilerServices;
Expand All @@ -22,7 +23,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;
Expand All @@ -33,21 +34,52 @@ 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()
{
// Must be placed outside s_syncObject lock. See Out getter.
ConsolePal.EnsureConsoleInitialized();

lock (s_syncObject) // Ensures In and InputEncoding are synchronized.
{
if (s_in == null)
{
Volatile.Write(ref s_in, ConsolePal.GetOrCreateReader());
}
return s_in;
}
}
}
}

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)
{
if (s_inputEncoding == null)
{
Volatile.Write(ref s_inputEncoding, ConsolePal.InputEncoding);
}
encoding = s_inputEncoding;
}
}
return encoding;
}
set
{
CheckNonNull(value, nameof(value));
lock (InternalSyncObject)

lock (s_syncObject)
{
// Set the terminal console encoding.
ConsolePal.SetConsoleInputEncoding(value);
Expand All @@ -66,28 +98,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)
{
if (s_outputEncoding == null)
{
Volatile.Write(ref s_outputEncoding, ConsolePal.OutputEncoding);
}
encoding = s_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();
s_out.Flush();
Volatile.Write(ref s_out, null);
}
if (Volatile.Read(ref s_error) != null && !s_isErrorTextWriterRedirected)
if (s_error != null && !s_isErrorTextWriterRedirected)
{
s_error!.Flush();
s_error.Flush();
Volatile.Write(ref s_error, null);
}

Expand Down Expand Up @@ -119,9 +163,54 @@ 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
{
// Console.Out shouldn't be locked while holding a lock on s_syncObject.
// Otherwise there can be a deadlock when another thread locks these
// objects in opposite order.
//
// Some functionality requires the console to be initialized.
// On Linux, this initialization requires a lock on Console.Out.
// The EnsureConsoleInitialized call must be placed outside the s_syncObject lock.
Debug.Assert(!Monitor.IsEntered(s_syncObject));

return Volatile.Read(ref s_out) ?? EnsureInitialized();

static TextWriter EnsureInitialized()
{
lock (s_syncObject) // Ensures Out and OutputEncoding are synchronized.
{
if (s_out == null)
{
Volatile.Write(ref s_out, CreateOutputWriter(OpenStandardOutput()));
}
return s_out;
}
}
}
}

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.
{
if (s_error == null)
{
Volatile.Write(ref s_error, CreateOutputWriter(OpenStandardError()));
}
return s_error;
}
}
}
}

private static TextWriter CreateOutputWriter(Stream outputStream)
{
Expand All @@ -145,26 +234,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;
}
}
}

Expand Down Expand Up @@ -331,7 +438,10 @@ public static event ConsoleCancelEventHandler? CancelKeyPress
{
add
{
lock (InternalSyncObject)
// Must be placed outside s_syncObject lock. See Out getter.
ConsolePal.EnsureConsoleInitialized();

lock (s_syncObject)
{
s_cancelCallbacks += value;

Expand All @@ -345,7 +455,7 @@ public static event ConsoleCancelEventHandler? CancelKeyPress
}
remove
{
lock (InternalSyncObject)
lock (s_syncObject)
{
s_cancelCallbacks -= value;
if (s_registrar != null && s_cancelCallbacks == null)
Expand Down Expand Up @@ -412,7 +522,7 @@ public static void SetIn(TextReader newIn)
{
CheckNonNull(newIn, nameof(newIn));
newIn = SyncTextReader.GetSynchronizedTextReader(newIn);
lock (InternalSyncObject)
lock (s_syncObject)
{
Volatile.Write(ref s_in, newIn);
}
Expand All @@ -422,10 +532,9 @@ 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)
{
s_isOutTextWriterRedirected = true;
Volatile.Write(ref s_out, newOut);
}
}
Expand All @@ -434,10 +543,9 @@ 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)
{
s_isErrorTextWriterRedirected = true;
Volatile.Write(ref s_error, newError);
}
}
Expand Down
44 changes: 30 additions & 14 deletions src/libraries/System.Console/src/System/ConsolePal.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,24 @@ 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.
// The reader doesn't own resources, so we don't need to dispose
// when it was already set.
Interlocked.CompareExchange(ref s_stdInReader, reader, null);

return s_stdInReader;
}
}
}

Expand All @@ -97,8 +107,7 @@ internal static TextReader GetOrCreateReader()
encoding: Console.InputEncoding,
detectEncodingFromByteOrderMarks: false,
bufferSize: Console.ReadBufferSize,
leaveOpen: true)
);
leaveOpen: true));
}
else
{
Expand Down Expand Up @@ -143,14 +152,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());
}
Expand Down Expand Up @@ -917,7 +926,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)
{
Expand All @@ -928,6 +937,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)
Expand Down Expand Up @@ -1460,7 +1476,7 @@ internal sealed class ControlCHandlerRegistrar

internal void Register()
{
EnsureInitialized();
EnsureConsoleInitialized();

Debug.Assert(!_handlerRegistered);
Interop.Sys.RegisterForCtrl(c => OnBreakEvent(c));
Expand Down
4 changes: 4 additions & 0 deletions src/libraries/System.Console/src/System/ConsolePal.Windows.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Console/src/System/ConsolePal.iOS.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 59a24e5

Please sign in to comment.