Fix thread-safety of ConsoleWriter and ConsoleReader

Fixes #123
2025-10-25 15:19:17 +00:00 · 2022-01-30 18:57:26 +02:00
parent 896482821c
commit 0f3abb9db4
3 changed files with 322 additions and 6 deletions
--- a/CliFx/Infrastructure/ConsoleReader.cs
+++ b/CliFx/Infrastructure/ConsoleReader.cs
@@ -1,11 +1,16 @@
-using System.IO;
+using System.Diagnostics.CodeAnalysis;
+using System.IO;
+using System.Runtime.CompilerServices;
 using System.Text;
+using System.Threading.Tasks;

 namespace CliFx.Infrastructure;

 /// <summary>
 /// Implements a <see cref="TextReader"/> for reading characters from a console stream.
 /// </summary>
+// Both the underlying stream AND the stream reader must be synchronized!
+// https://github.com/Tyrrrz/CliFx/issues/123
 public partial class ConsoleReader : StreamReader
 {
    /// <summary>
@@ -29,6 +34,77 @@ public partial class ConsoleReader : StreamReader
        : this(console, stream, System.Console.InputEncoding)
    {
    }
+
+    // The following overrides are required to establish thread-safe behavior
+    // in methods deriving from StreamReader.
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override int Peek() => base.Peek();
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override int Read() => base.Read();
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override int Read(char[] buffer, int index, int count) =>
+        base.Read(buffer, index, count);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override int ReadBlock(char[] buffer, int index, int count)
+    {
+        return base.ReadBlock(buffer, index, count);
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override string? ReadLine() => base.ReadLine();
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override string ReadToEnd() => base.ReadToEnd();
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task<int> ReadAsync(char[] buffer, int index, int count)
+    {
+        // Must be non-async to work with locks
+        return Task.FromResult(Read(buffer, index, count));
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task<int> ReadBlockAsync(char[] buffer, int index, int count)
+    {
+        // Must be non-async to work with locks
+        return Task.FromResult(ReadBlock(buffer, index, count));
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task<string?> ReadLineAsync()
+    {
+        // Must be non-async to work with locks
+        return Task.FromResult(ReadLine());
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task<string> ReadToEndAsync()
+    {
+        // Must be non-async to work with locks
+        return Task.FromResult(ReadToEnd());
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Close() => base.Close();
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    protected override void Dispose(bool disposing) => base.Dispose(disposing);
 }

 public partial class ConsoleReader
--- a/CliFx/Infrastructure/ConsoleWriter.cs
+++ b/CliFx/Infrastructure/ConsoleWriter.cs
@@ -1,5 +1,8 @@
-using System.IO;
+using System.Diagnostics.CodeAnalysis;
+using System.IO;
+using System.Runtime.CompilerServices;
 using System.Text;
+using System.Threading.Tasks;
 using CliFx.Utils;

 namespace CliFx.Infrastructure;
@@ -7,6 +10,8 @@ namespace CliFx.Infrastructure;
 /// <summary>
 /// Implements a <see cref="TextWriter"/> for writing characters to a console stream.
 /// </summary>
+// Both the underlying stream AND the stream writer must be synchronized!
+// https://github.com/Tyrrrz/CliFx/issues/123
 public partial class ConsoleWriter : StreamWriter
 {
    /// <summary>
@@ -30,6 +35,239 @@ public partial class ConsoleWriter : StreamWriter
        : this(console, stream, System.Console.OutputEncoding)
    {
    }
+
+    // The following overrides are required to establish thread-safe behavior
+    // in methods deriving from StreamWriter.
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(char[] buffer, int index, int count) => base.Write(buffer, index, count);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(char[] buffer) => base.Write(buffer);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(char value) => base.Write(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(string? value) => base.Write(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(string format, object? arg0) => base.Write(format, arg0);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(string format, object? arg0, object? arg1) =>
+        base.Write(format, arg0, arg1);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(string format, object? arg0, object? arg1, object? arg2) =>
+        base.Write(format, arg0, arg1, arg2);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(string format, params object?[] arg) => base.Write(format, arg);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(bool value) => base.Write(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(int value) => base.Write(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(long value) => base.Write(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(uint value) => base.Write(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(ulong value) => base.Write(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(float value) => base.Write(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(double value) => base.Write(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(decimal value) => base.Write(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Write(object? value) => base.Write(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task WriteAsync(char[] buffer, int index, int count)
+    {
+        // Must be non-async to work with locks
+        Write(buffer, index, count);
+        return Task.CompletedTask;
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task WriteAsync(char value)
+    {
+        // Must be non-async to work with locks
+        Write(value);
+        return Task.CompletedTask;
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task WriteAsync(string? value)
+    {
+        // Must be non-async to work with locks
+        Write(value);
+        return Task.CompletedTask;
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine() => base.WriteLine();
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(char[] buffer, int index, int count) =>
+        base.WriteLine(buffer, index, count);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(char[] buffer) => base.WriteLine(buffer);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(char value) => base.WriteLine(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(string? value) => base.WriteLine(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(string format, object? arg0) => base.WriteLine(format, arg0);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(string format, object? arg0, object? arg1) =>
+        base.WriteLine(format, arg0, arg1);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(string format, object? arg0, object? arg1, object? arg2) =>
+        base.WriteLine(format, arg0, arg1, arg2);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(string format, params object?[] arg) =>
+        base.WriteLine(format, arg);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(bool value) => base.WriteLine(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(int value) => base.WriteLine(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(long value) => base.WriteLine(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(uint value) => base.WriteLine(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(ulong value) => base.WriteLine(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(float value) => base.WriteLine(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(double value) => base.WriteLine(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(decimal value) => base.WriteLine(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void WriteLine(object? value) => base.WriteLine(value);
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task WriteLineAsync()
+    {
+        // Must be non-async to work with locks
+        WriteLine();
+        return Task.CompletedTask;
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task WriteLineAsync(char value)
+    {
+        // Must be non-async to work with locks
+        WriteLine(value);
+        return Task.CompletedTask;
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task WriteLineAsync(char[] buffer, int index, int count)
+    {
+        // Must be non-async to work with locks
+        WriteLine(buffer, index, count);
+        return Task.CompletedTask;
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task WriteLineAsync(string? value)
+    {
+        // Must be non-async to work with locks
+        WriteLine(value);
+        return Task.CompletedTask;
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Flush() => base.Flush();
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override Task FlushAsync()
+    {
+        // Must be non-async to work with locks
+        Flush();
+        return Task.CompletedTask;
+    }
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    public override void Close() => base.Close();
+
+    /// <inheritdoc />
+    [ExcludeFromCodeCoverage, MethodImpl(MethodImplOptions.Synchronized)]
+    protected override void Dispose(bool disposing) => base.Dispose(disposing);
 }

 public partial class ConsoleWriter
--- a/CliFx/Utils/NoPreambleEncoding.cs
+++ b/CliFx/Utils/NoPreambleEncoding.cs
@@ -8,6 +8,8 @@ namespace CliFx.Utils;
 // https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/Common/src/System/Text/ConsoleEncoding.cs
 // Also see:
 // https://source.dot.net/#System.Console/ConsoleEncoding.cs,5eedd083a4a4f4a2
+// Majority of overrides are just proxy calls to avoid potentially more expensive base behavior.
+// The important part is the GetPreamble() method that has been overriden to return an empty array.
 internal class NoPreambleEncoding : Encoding
 {
    private readonly Encoding _underlyingEncoding;
@@ -72,20 +74,20 @@ internal class NoPreambleEncoding : Encoding
    public override int GetBytes(char[] chars, int charIndex, int charCount, byte[] bytes, int byteIndex) =>
        _underlyingEncoding.GetBytes(chars, charIndex, charCount, bytes, byteIndex);

-    [ExcludeFromCodeCoverage]
-    public override byte[] GetBytes(char[] chars) => _underlyingEncoding.GetBytes(chars);
-
    [ExcludeFromCodeCoverage]
    public override byte[] GetBytes(char[] chars, int index, int count) =>
        _underlyingEncoding.GetBytes(chars, index, count);

    [ExcludeFromCodeCoverage]
-    public override byte[] GetBytes(string s) => _underlyingEncoding.GetBytes(s);
+    public override byte[] GetBytes(char[] chars) => _underlyingEncoding.GetBytes(chars);

    [ExcludeFromCodeCoverage]
    public override int GetBytes(string s, int charIndex, int charCount, byte[] bytes, int byteIndex) =>
        _underlyingEncoding.GetBytes(s, charIndex, charCount, bytes, byteIndex);

+    [ExcludeFromCodeCoverage]
+    public override byte[] GetBytes(string s) => _underlyingEncoding.GetBytes(s);
+
    [ExcludeFromCodeCoverage]
    public override int GetCharCount(byte[] bytes, int index, int count) =>
        _underlyingEncoding.GetCharCount(bytes, index, count);