From 6863917fde7969cf555ed059166a36afb438b67e Mon Sep 17 00:00:00 2001 From: Tig Date: Thu, 30 Apr 2026 09:16:35 -0600 Subject: [PATCH 1/6] Adds concurrency test for OutputBufferImpl race condition Adds AddStr_And_ClearContents_Concurrent_DoesNotThrow test that reproduces the race condition in OutputBufferImpl where ClearContents replaces the Contents reference while AddStr is concurrently iterating. See #5130. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../OutputBufferImplConcurrencyTests.cs | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs diff --git a/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs b/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs new file mode 100644 index 0000000000..9c3e2663fc --- /dev/null +++ b/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs @@ -0,0 +1,97 @@ +using System.Collections.Concurrent; + +namespace DriverTests.Output; + +/// +/// Proves the race condition in where +/// replaces the Contents +/// reference while is concurrently iterating. +/// See: https://github.com/gui-cs/Terminal.Gui/issues/5130 +/// +public class OutputBufferImplConcurrencyTests +{ + /// + /// Concurrently calls and + /// from separate threads. + /// With the current broken lock (Contents) pattern this reproduces + /// crashes (AccessViolationException, NullReferenceException, or + /// IndexOutOfRangeException) within a few hundred iterations. + /// + [Fact] + public void AddStr_And_ClearContents_Concurrent_DoesNotThrow () + { + // Arrange + OutputBufferImpl buffer = new (); + buffer.SetSize (80, 25); + + ConcurrentBag exceptions = []; + const int ITERATIONS = 5_000; + CancellationTokenSource cts = new (); + + // Writer thread — continuously writes strings + Thread writer = new (() => + { + try + { + for (var i = 0; i < ITERATIONS && !cts.IsCancellationRequested; i++) + { + try + { + buffer.Move (0, i % 25); + buffer.AddStr (new string ('A', 80)); + } + catch (Exception ex) when (ex is not OutOfMemoryException) + { + exceptions.Add (ex); + + break; + } + } + } + catch (Exception ex) + { + exceptions.Add (ex); + } + }) { IsBackground = true }; + + // Resizer/clearer thread — continuously replaces Contents + Thread clearer = new (() => + { + try + { + for (var i = 0; i < ITERATIONS && !cts.IsCancellationRequested; i++) + { + try + { + // Alternate sizes to maximize chance of IndexOutOfRange + buffer.SetSize (i % 2 == 0 ? 80 : 40, i % 2 == 0 ? 25 : 10); + } + catch (Exception ex) when (ex is not OutOfMemoryException) + { + exceptions.Add (ex); + + break; + } + } + } + catch (Exception ex) + { + exceptions.Add (ex); + } + }) { IsBackground = true }; + + // Act + writer.Start (); + clearer.Start (); + + writer.Join (TimeSpan.FromSeconds (10)); + clearer.Join (TimeSpan.FromSeconds (10)); + cts.Cancel (); + + // Assert — with the bug present, we expect exceptions to have been collected. + // Once fixed this test should pass with zero exceptions. + Assert.True (exceptions.IsEmpty, + $"Caught {exceptions.Count} exception(s) during concurrent access. " + + $"First: {exceptions.FirstOrDefault ()?.GetType ().Name}: {exceptions.FirstOrDefault ()?.Message}"); + } +} From 0d30d10b1d34936fad7207cc53eaba757ef4ca2e Mon Sep 17 00:00:00 2001 From: Tig Date: Thu, 30 Apr 2026 09:50:02 -0600 Subject: [PATCH 2/6] Adds concurrency tests, benchmark, and plan for #5130 - Expanded OutputBufferImplConcurrencyTests with 4 tests covering: AddStr+ClearContents, FillRect+ClearContents, Move+AddStr+ClearContents, and a three-way concurrent test - Added OutputBufferBenchmark for perf baseline (AddStr, FillRect, ClearContents, SetSize, TypicalDrawCycle) - Added implementation plan in plans/ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../OutputBuffer/OutputBufferBenchmark.cs | 102 ++++++++ .../OutputBufferImplConcurrencyTests.cs | 209 +++++++++++++-- plans/5130-output-buffer-race-condition.md | 240 ++++++++++++++++++ 3 files changed, 531 insertions(+), 20 deletions(-) create mode 100644 Tests/Benchmarks/ConsoleDrivers/OutputBuffer/OutputBufferBenchmark.cs create mode 100644 plans/5130-output-buffer-race-condition.md diff --git a/Tests/Benchmarks/ConsoleDrivers/OutputBuffer/OutputBufferBenchmark.cs b/Tests/Benchmarks/ConsoleDrivers/OutputBuffer/OutputBufferBenchmark.cs new file mode 100644 index 0000000000..c57c3a111b --- /dev/null +++ b/Tests/Benchmarks/ConsoleDrivers/OutputBuffer/OutputBufferBenchmark.cs @@ -0,0 +1,102 @@ +using System.Drawing; +using System.Text; +using BenchmarkDotNet.Attributes; +using Terminal.Gui.Drivers; +using Attribute = Terminal.Gui.Drawing.Attribute; +using Color = Terminal.Gui.Drawing.Color; + +namespace Terminal.Gui.Benchmarks.ConsoleDrivers.OutputBuffer; + +/// +/// Benchmarks for operations. +/// Measures the performance of AddStr, FillRect, ClearContents, and SetSize +/// to establish a baseline before and after the _contentsLock fix (#5130). +/// +[MemoryDiagnoser] +[BenchmarkCategory ("OutputBuffer")] +public class OutputBufferBenchmark +{ + private OutputBufferImpl _buffer = null!; + + [Params (80, 200)] + public int Cols { get; set; } + + [Params (25, 50)] + public int Rows { get; set; } + + [GlobalSetup] + public void Setup () + { + _buffer = new OutputBufferImpl (); + _buffer.SetSize (Cols, Rows); + _buffer.CurrentAttribute = new Attribute (Color.White, Color.Black); + } + + /// + /// Baseline: Write a full row of text using AddStr. + /// + [Benchmark (Baseline = true)] + public void AddStr_FullRow () + { + _buffer.Move (0, 0); + _buffer.AddStr (new string ('A', Cols)); + } + + /// + /// Write text to every row in the buffer. + /// + [Benchmark] + public void AddStr_AllRows () + { + string line = new ('A', Cols); + + for (var row = 0; row < Rows; row++) + { + _buffer.Move (0, row); + _buffer.AddStr (line); + } + } + + /// + /// Fill the entire screen using FillRect(Rectangle, Rune). + /// + [Benchmark] + public void FillRect_FullScreen () + { + _buffer.FillRect (new Rectangle (0, 0, Cols, Rows), new Rune (' ')); + } + + /// + /// ClearContents resets the entire buffer. + /// + [Benchmark] + public void ClearContents () + { + _buffer.ClearContents (); + } + + /// + /// SetSize triggers ClearContents internally. + /// + [Benchmark] + public void SetSize () + { + _buffer.SetSize (Cols, Rows); + } + + /// + /// Simulates a typical draw cycle: clear, then fill every row. + /// + [Benchmark] + public void TypicalDrawCycle () + { + _buffer.ClearContents (); + string line = new ('X', Cols); + + for (var row = 0; row < Rows; row++) + { + _buffer.Move (0, row); + _buffer.AddStr (line); + } + } +} diff --git a/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs b/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs index 9c3e2663fc..ac86b7310d 100644 --- a/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs +++ b/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs @@ -1,34 +1,33 @@ using System.Collections.Concurrent; +using System.Text; namespace DriverTests.Output; /// -/// Proves the race condition in where +/// Proves the race conditions in where /// replaces the Contents -/// reference while is concurrently iterating. +/// reference while other methods are concurrently operating on it. /// See: https://github.com/gui-cs/Terminal.Gui/issues/5130 /// public class OutputBufferImplConcurrencyTests { + // Copilot + + private const int ITERATIONS = 5_000; + /// - /// Concurrently calls and - /// from separate threads. - /// With the current broken lock (Contents) pattern this reproduces - /// crashes (AccessViolationException, NullReferenceException, or - /// IndexOutOfRangeException) within a few hundred iterations. + /// Runs a writer action and a clearer action concurrently, collecting any exceptions. + /// Returns the collected exceptions for assertion. /// - [Fact] - public void AddStr_And_ClearContents_Concurrent_DoesNotThrow () + private static ConcurrentBag RunConcurrent (Action writerAction, + Action clearerAction) { - // Arrange OutputBufferImpl buffer = new (); buffer.SetSize (80, 25); ConcurrentBag exceptions = []; - const int ITERATIONS = 5_000; CancellationTokenSource cts = new (); - // Writer thread — continuously writes strings Thread writer = new (() => { try @@ -37,8 +36,7 @@ public void AddStr_And_ClearContents_Concurrent_DoesNotThrow () { try { - buffer.Move (0, i % 25); - buffer.AddStr (new string ('A', 80)); + writerAction (buffer, i, cts.Token); } catch (Exception ex) when (ex is not OutOfMemoryException) { @@ -54,7 +52,6 @@ public void AddStr_And_ClearContents_Concurrent_DoesNotThrow () } }) { IsBackground = true }; - // Resizer/clearer thread — continuously replaces Contents Thread clearer = new (() => { try @@ -63,8 +60,7 @@ public void AddStr_And_ClearContents_Concurrent_DoesNotThrow () { try { - // Alternate sizes to maximize chance of IndexOutOfRange - buffer.SetSize (i % 2 == 0 ? 80 : 40, i % 2 == 0 ? 25 : 10); + clearerAction (buffer, i, cts.Token); } catch (Exception ex) when (ex is not OutOfMemoryException) { @@ -80,7 +76,6 @@ public void AddStr_And_ClearContents_Concurrent_DoesNotThrow () } }) { IsBackground = true }; - // Act writer.Start (); clearer.Start (); @@ -88,10 +83,184 @@ public void AddStr_And_ClearContents_Concurrent_DoesNotThrow () clearer.Join (TimeSpan.FromSeconds (10)); cts.Cancel (); - // Assert — with the bug present, we expect exceptions to have been collected. - // Once fixed this test should pass with zero exceptions. + return exceptions; + } + + private static void AssertNoExceptions (ConcurrentBag exceptions) + { Assert.True (exceptions.IsEmpty, $"Caught {exceptions.Count} exception(s) during concurrent access. " + $"First: {exceptions.FirstOrDefault ()?.GetType ().Name}: {exceptions.FirstOrDefault ()?.Message}"); } + + /// + /// Concurrently calls and + /// from separate threads. + /// With the current broken lock (Contents) pattern this reproduces + /// crashes (AccessViolationException, NullReferenceException, or + /// IndexOutOfRangeException) within a few hundred iterations. + /// + [Fact] + public void AddStr_And_ClearContents_Concurrent_DoesNotThrow () + { + ConcurrentBag exceptions = RunConcurrent ( + (buffer, i, _) => + { + buffer.Move (0, i % 25); + buffer.AddStr (new string ('A', 80)); + }, + (buffer, i, _) => + { + // Alternate sizes to maximize chance of IndexOutOfRange + buffer.SetSize (i % 2 == 0 ? 80 : 40, i % 2 == 0 ? 25 : 10); + }); + + AssertNoExceptions (exceptions); + } + + /// + /// Concurrently calls and + /// from separate threads. + /// The FillRect(Rectangle, Rune) overload has the same broken lock (Contents!) pattern. + /// + [Fact] + public void FillRect_And_ClearContents_Concurrent_DoesNotThrow () + { + ConcurrentBag exceptions = RunConcurrent ( + (buffer, i, _) => + { + // Fill a region that fits within the smallest size (40x10) + buffer.FillRect (new Rectangle (0, 0, 30, 8), new Rune ('B')); + }, + (buffer, i, _) => + { + buffer.SetSize (i % 2 == 0 ? 80 : 40, i % 2 == 0 ? 25 : 10); + }); + + AssertNoExceptions (exceptions); + } + + /// + /// Concurrently calls , , + /// and from separate threads to verify that + /// interleaved Move + AddStr doesn't corrupt state when Contents is being replaced. + /// + [Fact] + public void Move_AddStr_And_ClearContents_Concurrent_DoesNotThrow () + { + ConcurrentBag exceptions = RunConcurrent ( + (buffer, i, _) => + { + // Rapidly move around and write short strings + buffer.Move (i % 40, i % 10); + buffer.AddStr ("Hello"); + buffer.Move ((i + 20) % 40, (i + 5) % 10); + buffer.AddStr ("World!"); + }, + (buffer, i, _) => + { + buffer.SetSize (i % 2 == 0 ? 80 : 40, i % 2 == 0 ? 25 : 10); + }); + + AssertNoExceptions (exceptions); + } + + /// + /// Runs and + /// concurrently with to exercise all three write paths + /// simultaneously. + /// + [Fact] + public void AddStr_FillRect_And_ClearContents_ThreeWay_Concurrent_DoesNotThrow () + { + OutputBufferImpl buffer = new (); + buffer.SetSize (80, 25); + + ConcurrentBag exceptions = []; + CancellationTokenSource cts = new (); + + Thread addStrThread = new (() => + { + try + { + for (var i = 0; i < ITERATIONS && !cts.IsCancellationRequested; i++) + { + try + { + buffer.Move (0, i % 25); + buffer.AddStr (new string ('X', 40)); + } + catch (Exception ex) when (ex is not OutOfMemoryException) + { + exceptions.Add (ex); + + break; + } + } + } + catch (Exception ex) + { + exceptions.Add (ex); + } + }) { IsBackground = true }; + + Thread fillRectThread = new (() => + { + try + { + for (var i = 0; i < ITERATIONS && !cts.IsCancellationRequested; i++) + { + try + { + buffer.FillRect (new Rectangle (0, 0, 20, 5), new Rune ('Z')); + } + catch (Exception ex) when (ex is not OutOfMemoryException) + { + exceptions.Add (ex); + + break; + } + } + } + catch (Exception ex) + { + exceptions.Add (ex); + } + }) { IsBackground = true }; + + Thread clearerThread = new (() => + { + try + { + for (var i = 0; i < ITERATIONS && !cts.IsCancellationRequested; i++) + { + try + { + buffer.SetSize (i % 2 == 0 ? 80 : 40, i % 2 == 0 ? 25 : 10); + } + catch (Exception ex) when (ex is not OutOfMemoryException) + { + exceptions.Add (ex); + + break; + } + } + } + catch (Exception ex) + { + exceptions.Add (ex); + } + }) { IsBackground = true }; + + addStrThread.Start (); + fillRectThread.Start (); + clearerThread.Start (); + + addStrThread.Join (TimeSpan.FromSeconds (10)); + fillRectThread.Join (TimeSpan.FromSeconds (10)); + clearerThread.Join (TimeSpan.FromSeconds (10)); + cts.Cancel (); + + AssertNoExceptions (exceptions); + } } diff --git a/plans/5130-output-buffer-race-condition.md b/plans/5130-output-buffer-race-condition.md new file mode 100644 index 0000000000..841fb2323e --- /dev/null +++ b/plans/5130-output-buffer-race-condition.md @@ -0,0 +1,240 @@ +# Plan: Fix OutputBufferImpl Race Condition (#5130) + +**PR**: [#5131](https://github.com/gui-cs/Terminal.Gui/pull/5131) +**Branch**: `fix/5130-output-buffer-race-condition` + +## Problem Statement + +`OutputBufferImpl` has a race condition where `ClearContents()` replaces the `Contents` array +reference **outside** any lock, then acquires a lock on the **new** object. Concurrently, +`AddGrapheme()` acquires a lock on whatever `Contents` happens to point to. When both run at +the same time: + +1. `AddGrapheme` reads `Contents` (old reference), passes the null check +2. `ClearContents` swaps `Contents` to a new array +3. `AddGrapheme` locks on `Contents` — now the **new** object — same object `ClearContents` locks on +4. Both threads can proceed through the lock without mutual exclusion on the **old** array +5. The old array's `Cell` data gets partially overwritten, corrupting `Rune` values +6. Native `Wcwidth` crashes with `AccessViolationException` on the corrupted data + +The `FillRect(Rectangle, Rune)` overload has the identical `lock (Contents!)` anti-pattern. + +Additionally, `OutputBase.WriteToScreen()` reads `Contents` without any lock during screen +flush, creating a third concurrent access point. + +## Root Cause + +Locking on `Contents` itself is fundamentally broken because `ClearContents` **replaces** the +reference. A `lock(obj)` only provides mutual exclusion when all threads lock on the **same** +object instance. + +## Approach + +Introduce a **stable, `readonly` dedicated lock object** (`_contentsLock`) that is never +replaced. All code that reads or writes `Contents`, `DirtyLines`, `Clip`, or `_urlMap` must +do so inside `lock (_contentsLock)`. + +### Design Considerations + +1. **Lock granularity**: A single lock object is simplest and correct. The critical sections + are short (cell writes, array swaps). Performance impact should be negligible since + drawing is already serialized on the main thread — the lock primarily protects against + the driver resize path which runs on a different thread. + +2. **`Rows`/`Cols` setters**: Both call `ClearContents()` which will now acquire the lock. + The `SetSize()` method sets `Cols` then `Rows`, triggering two `ClearContents` calls. We + should consider having `SetSize` set backing fields directly and call `ClearContents` + once. However, this is an optimization — the lock is re-entrant safe... actually, + `Monitor.Enter` (what `lock` uses) is **re-entrant** in .NET, so the double call won't + deadlock, but it's wasteful. We should fix `SetSize` to avoid the double clear. + +3. **`OutputBase.WriteToScreen()`**: This reads `Contents` without any synchronization. The + fix should expose the lock or provide a synchronized accessor. However, since + `WriteToScreen` runs on the main thread during `Refresh()`, and `ClearContents` is the + only cross-thread mutator, the primary fix in `OutputBufferImpl` should suffice. We can + address `OutputBase` as a follow-up if needed. + +4. **`Move()`**: Sets `Col` and `Row` without the lock. Since `Move` + `AddGrapheme` are + always called sequentially from the same thread, and the lock protects `AddGrapheme` + internals, `Move` doesn't need the lock. However, `Col`/`Row` are read inside the lock + in `AddGrapheme` — we should verify no cross-thread `Move` calls exist. + +5. **The `FillRect(Rectangle, char)` overload** (line 452): This calls `Move` + `AddRune` + in a loop without a lock. It's protected indirectly because `AddGrapheme` acquires the + lock. No change needed for this overload. + +## Implementation Steps + +### Step 1: Add `_contentsLock` field + +Add a `private readonly object _contentsLock = new ();` field near the top of the class, +next to the existing `_cols` / `_rows` fields. + +### Step 2: Fix `ClearContents(bool)` + +Move **all** mutations (`Contents`, `Clip`, `DirtyLines`, `_urlMap`) inside +`lock (_contentsLock)`. The entire method body goes inside the lock. + +```csharp +public void ClearContents (bool initiallyDirty) +{ + lock (_contentsLock) + { + Contents = new Cell [Rows, Cols]; + Clip = new Region (Screen); + DirtyLines = new bool [Rows]; + _urlMap?.Clear (); + + for (var row = 0; row < Rows; row++) + { + for (var c = 0; c < Cols; c++) + { + Contents [row, c] = new Cell + { + Grapheme = " ", + Attribute = new Attribute (Color.White, Color.Black), + IsDirty = initiallyDirty + }; + } + + DirtyLines [row] = initiallyDirty; + } + } +} +``` + +### Step 3: Fix `AddGrapheme(string)` + +Replace `lock (Contents)` with `lock (_contentsLock)`. Add a double-checked null guard +inside the lock. Move `Clip` access inside the lock since `ClearContents` now replaces +`Clip` under the lock. + +```csharp +private void AddGrapheme (string grapheme) +{ + lock (_contentsLock) + { + if (Contents is null) + { + return; + } + + Clip ??= new Region (Screen); + Rectangle clipRect = Clip!.GetBounds (); + int printableGraphemeWidth = -1; + + if (IsValidLocation (grapheme, Col, Row)) + { + SetAttributeAndDirty (Col, Row); + InvalidateOverlappedWideGlyph (Col, Row); + + string printableGrapheme = grapheme.MakePrintable (); + printableGraphemeWidth = printableGrapheme.GetColumns (); + WriteGraphemeByWidth (Col, Row, printableGrapheme, printableGraphemeWidth, clipRect); + + DirtyLines [Row] = true; + } + + Col++; + + if (printableGraphemeWidth <= 1) + { + return; + } + + if (Clip.Contains (Col, Row)) + { + if (Contents [Row, Col].Attribute != CurrentAttribute) + { + Contents [Row, Col].Attribute = CurrentAttribute; + Contents [Row, Col].IsDirty = true; + } + } + + Col++; + } +} +``` + +### Step 4: Fix `FillRect(Rectangle, Rune)` + +Replace `lock (Contents!)` with `lock (_contentsLock)`. Add null guard inside. + +### Step 5: Fix `SetSize` to avoid double `ClearContents` + +Currently `SetSize` sets `Cols` then `Rows`, each triggering `ClearContents()` via the +property setters. Refactor to set backing fields directly and call `ClearContents` once: + +```csharp +public void SetSize (int cols, int rows) +{ + _cols = cols; + _rows = rows; + ClearContents (); +} +``` + +### Step 6: Verify the test passes + +Run the existing concurrency test: + +```bash +dotnet test --project Tests/UnitTestsParallelizable --no-build --filter-method "*AddStr_And_ClearContents_Concurrent_DoesNotThrow" +``` + +### Step 7: Run full test suite + +Ensure no regressions: + +```bash +dotnet test --project Tests/UnitTestsParallelizable --no-build +dotnet test --project Tests/UnitTests.NonParallelizable --no-build +``` + +### Step 8: Consider additional test coverage + +- Test that `FillRect` + `ClearContents` concurrent access doesn't throw +- Test that `Move` + `AddStr` + `ClearContents` produces consistent state + +## Files to Change + +| File | Change | +|------|--------| +| `Terminal.Gui/Drivers/Output/OutputBufferImpl.cs` | Add `_contentsLock`; fix `ClearContents`, `AddGrapheme`, `FillRect`, `SetSize` | + +## Risks & Mitigations + +| Risk | Mitigation | +|------|-----------| +| Lock contention slows drawing | Lock is only held for cell-level operations; main thread drawing is already serialized. Measure if concerned. | +| Deadlock from re-entrant lock in `SetSize` | .NET `Monitor` (used by `lock`) is re-entrant. Plus Step 5 eliminates the double call. | +| `OutputBase.WriteToScreen` still reads without lock | Runs on main thread during `Refresh()`; cross-thread mutation is now synchronized. Document as future improvement. | +| `Col`/`Row` modified outside lock by `Move()` | `Move()` is always called from the same thread as `AddGrapheme`. No cross-thread `Move` calls observed. | + +## Benchmark Baseline (Before Fix) + +Captured with `--job short` on Intel Core i9-14900K, .NET 10.0.6: + +| Method | Cols | Rows | Mean | Allocated | +|-------------------- |----- |----- |------------:|-----------:| +| AddStr_FullRow | 80 | 25 | 31.48 μs | 63.77 KB | +| AddStr_AllRows | 80 | 25 | 776.68 μs | 1590.02 KB | +| FillRect_FullScreen | 80 | 25 | 666.49 μs | 1634.18 KB | +| ClearContents | 80 | 25 | 75.76 μs | 63.09 KB | +| SetSize | 80 | 25 | 225.99 μs | 189.26 KB | +| TypicalDrawCycle | 80 | 25 | 859.06 μs | 1653.11 KB | +| AddStr_FullRow | 200 | 50 | 78.34 μs | 159.63 KB | +| AddStr_AllRows | 200 | 50 | 4,205.18 μs | 7961.35 KB | +| FillRect_FullScreen | 200 | 50 | 3,799.05 μs | 8190.23 KB | +| ClearContents | 200 | 50 | 444.25 μs | 313.27 KB | +| SetSize | 200 | 50 | 1,310.77 μs | 939.82 KB | +| TypicalDrawCycle | 200 | 50 | 5,512.82 μs | 8274.62 KB | + +The fix must not regress these numbers significantly. Re-run after implementing +and compare. + +## Out of Scope + +- Making `OutputBase.WriteToScreen` acquire the lock (would require exposing the lock or adding a synchronized read API — separate PR) +- Thread-safety for `Clip` property getter/setter beyond what's protected by `_contentsLock` +- General thread-safety audit of the driver layer From 72b383a46d2cf52cafa06a13727d61763eff7340 Mon Sep 17 00:00:00 2001 From: Tig Date: Thu, 30 Apr 2026 13:15:26 -0600 Subject: [PATCH 3/6] Refactor OutputBufferImpl for thread safety Replaced locking on Contents with a dedicated _contentsLock object to ensure safe concurrent access to buffer state (Contents, DirtyLines, Clip, etc.). Refactored buffer management methods (ClearContents, SetSize, FillRect, Move, IsValidLocation) to use the new lock. Updated FillRect and AddGrapheme to avoid race conditions. Improved documentation and comments for concurrency. Moved SetCellUrl/GetCellUrl implementations under the lock. In OutputBufferImplConcurrencyTests, reordered and moved helper methods for clarity; no test logic changes. No public API changes, but internal concurrency is now robust. --- .../Drivers/Output/OutputBufferImpl.cs | 413 ++++++++++-------- .../OutputBuffer/OutputBufferBenchmark.cs | 2 +- .../OutputBufferImplConcurrencyTests.cs | 292 +++++++------ plans/5130-output-buffer-race-condition.md | 240 ---------- 4 files changed, 384 insertions(+), 563 deletions(-) delete mode 100644 plans/5130-output-buffer-race-condition.md diff --git a/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs b/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs index ba45bff6b7..71eebd5038 100644 --- a/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs +++ b/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs @@ -9,6 +9,27 @@ namespace Terminal.Gui.Drivers; /// public class OutputBufferImpl : IOutputBuffer { + /// + /// Stable lock object for synchronizing access to , , + /// , and related state. Unlike locking on Contents itself, this object is + /// never replaced, guaranteeing mutual exclusion across , + /// , and . + /// + private readonly Lock _contentsLock = new (); + + private int _cols; + private int _rows; + + /// + /// Maps cell positions to URLs for OSC 8 hyperlink support. + /// Only stores entries for cells that actually have URLs, minimizing memory overhead. + /// + private Dictionary? _urlMap; + + private Rune _column1ReplacementChar = Glyphs.WideGlyphReplacement; + + private Region? _clip; + /// /// The contents of the application output. The driver outputs this buffer to the terminal when /// UpdateScreen is called. @@ -16,9 +37,6 @@ public class OutputBufferImpl : IOutputBuffer /// public Cell [,]? Contents { get; set; } = new Cell [0, 0]; - private int _cols; - private int _rows; - /// /// The that will be used for the next or /// call. @@ -32,30 +50,18 @@ public class OutputBufferImpl : IOutputBuffer /// public string? CurrentUrl { get; set; } - /// - /// Maps cell positions to URLs for OSC 8 hyperlink support. - /// Only stores entries for cells that actually have URLs, minimizing memory overhead. - /// - private Dictionary? _urlMap; - /// /// Gets the URL associated with the cell at the specified position. /// /// The column. /// The row. /// The URL if one exists, otherwise null. - public string? GetCellUrl (int col, int row) => _urlMap?.TryGetValue (new Point (col, row), out string? url) == true ? url : null; - - /// - /// Sets the URL for the cell at the specified position. - /// - /// The column. - /// The row. - /// The URL to associate with this cell. - private void SetCellUrl (int col, int row, string url) + public string? GetCellUrl (int col, int row) { - _urlMap ??= []; - _urlMap [new Point (col, row)] = url; + lock (_contentsLock) + { + return _urlMap?.TryGetValue (new Point (col, row), out string? url) == true ? url : null; + } } /// The leftmost column in the terminal. @@ -98,8 +104,6 @@ public int Cols /// The topmost row in the terminal. public virtual int Top { get; set; } = 0; - private Rune _column1ReplacementChar = Glyphs.WideGlyphReplacement; - /// public void SetWideGlyphReplacement (Rune column1ReplacementChar) => _column1ReplacementChar = column1ReplacementChar; @@ -108,12 +112,6 @@ public int Cols /// public bool [] DirtyLines { get; set; } = []; - // QUESTION: When non-full screen apps are supported, will this represent the app size, or will that be in Application? - /// Gets the location and size of the terminal screen. - internal Rectangle Screen => new (0, 0, Cols, Rows); - - private Region? _clip; - /// /// Gets or sets the clip rectangle that and are subject /// to. @@ -177,24 +175,173 @@ public void AddStr (string str) } } + /// Clears the of the driver. + public void ClearContents () => ClearContents (!InlineMode); + + /// Tests whether the specified coordinate are valid for drawing the specified Text. + /// Used to determine if one or two columns are required. + /// The column. + /// The row. + /// + /// if the coordinate is outside the screen bounds or outside of . + /// otherwise. + /// + public bool IsValidLocation (string text, int col, int row) + { + int textWidth = text.GetColumns (); + + return col >= 0 && row >= 0 && col + textWidth <= Cols && row < Rows && Clip!.Contains (col, row); + } + + /// + public void SetSize (int cols, int rows) + { + lock (_contentsLock) + { + _cols = cols; + _rows = rows; + ClearContentsCore (!InlineMode); + } + } + + /// + public void FillRect (Rectangle rect, Rune rune) + { + lock (_contentsLock) + { + if (Contents is null) + { + return; + } + + Clip ??= new Region (Screen); + Rectangle clipBounds = Clip!.GetBounds (); + + rect = Rectangle.Intersect (rect, clipBounds); + + for (int r = rect.Y; r < rect.Y + rect.Height; r++) + { + for (int c = rect.X; c < rect.X + rect.Width; c++) + { + if (!IsValidLocation (rune.ToString (), c, r)) + { + continue; + } + + // We could call AddGrapheme here, but that would acquire the lock again. + // So we inline the logic instead. + SetAttributeAndDirty (c, r); + InvalidateOverlappedWideGlyph (c, r); + string grapheme = rune != default (Rune) ? rune.ToString () : " "; + WriteGraphemeByWidth (c, r, grapheme, grapheme.GetColumns (), clipBounds); + } + } + } + } + + /// + public void FillRect (Rectangle rect, char rune) + { + for (int y = rect.Top; y < rect.Top + rect.Height; y++) + { + for (int x = rect.Left; x < rect.Left + rect.Width; x++) + { + Move (x, y); + AddRune (rune); + } + } + } + + /// + /// Updates and to the specified column and row in . + /// Used by and to determine where to add content. + /// + /// + /// This does not move the cursor on the screen, it only updates the internal state of the driver. + /// + /// If or are negative or beyond and + /// , the method still sets those properties. + /// + /// + /// Column to move to. + /// Row to move to. + public void Move (int col, int row) + { + Col = col; + Row = row; + } + + /// Clears the of the driver. + /// + /// When (the default), all cells are marked dirty so the first render + /// overwrites the entire screen. When (used in inline mode), cells start + /// clean so only cells that are explicitly drawn will be flushed, leaving the rest of the terminal + /// untouched. + /// + public void ClearContents (bool initiallyDirty) + { + lock (_contentsLock) + { + ClearContentsCore (initiallyDirty); + } + } + + /// + /// Non-locking implementation of . + /// Caller must already hold . + /// + private void ClearContentsCore (bool initiallyDirty) + { + Contents = new Cell [Rows, Cols]; + + // TODO: ClearContents should not clear the clip; it should only clear the contents. Move clearing it elsewhere. + Clip = new Region (Screen); + + DirtyLines = new bool [Rows]; + + // Clear the URL map + _urlMap?.Clear (); + + for (var row = 0; row < Rows; row++) + { + for (var c = 0; c < Cols; c++) + { + Contents [row, c] = new Cell { Grapheme = " ", Attribute = new Attribute (Color.White, Color.Black), IsDirty = initiallyDirty }; + } + + DirtyLines [row] = initiallyDirty; + } + } + + /// + /// Gets or sets a value indicating whether this buffer is operating in inline mode. + /// When , initialises cells with + /// IsDirty = false so that only cells explicitly drawn are flushed to the + /// terminal, leaving the rest of the visible terminal untouched. + /// + public bool InlineMode { get; set; } + + /// Gets the location and size of the terminal screen. + internal Rectangle Screen => new (0, 0, Cols, Rows); + /// /// Adds a single grapheme to the display at the current cursor position. /// /// The grapheme to add. private void AddGrapheme (string grapheme) { - if (Contents is null) + lock (_contentsLock) { - return; - } + if (Contents is null) + { + return; + } - Clip ??= new Region (Screen); - Rectangle clipRect = Clip!.GetBounds (); + Clip ??= new Region (Screen); + Rectangle clipRect = Clip!.GetBounds (); - int printableGraphemeWidth = -1; + int printableGraphemeWidth = -1; - lock (Contents) - { if (IsValidLocation (grapheme, Col, Row)) { // Set attribute and mark dirty for current cell @@ -237,6 +384,22 @@ private void AddGrapheme (string grapheme) } } + /// + /// INTERNAL: If we're writing at an odd column and there's a wide glyph to our left, + /// invalidate it since we're overwriting the second half. + /// + /// The column. + /// The row. + private void InvalidateOverlappedWideGlyph (int col, int row) + { + if (col <= 0 || Contents! [row, col - 1].Grapheme.GetColumns () <= 1) + { + return; + } + Contents [row, col - 1].Grapheme = _column1ReplacementChar.ToString (); + Contents [row, col - 1].IsDirty = true; + } + /// /// INTERNAL: Helper to set the attribute and mark the cell as dirty. /// @@ -255,19 +418,34 @@ private void SetAttributeAndDirty (int col, int row) } /// - /// INTERNAL: If we're writing at an odd column and there's a wide glyph to our left, - /// invalidate it since we're overwriting the second half. + /// Sets the URL for the cell at the specified position. /// /// The column. /// The row. - private void InvalidateOverlappedWideGlyph (int col, int row) + /// The URL to associate with this cell. + private void SetCellUrl (int col, int row, string url) { - if (col <= 0 || Contents! [row, col - 1].Grapheme.GetColumns () <= 1) + _urlMap ??= []; + _urlMap [new Point (col, row)] = url; + } + + /// + /// INTERNAL: Writes a (0 or 1 column wide) Grapheme. + /// + /// The column. + /// The row. + /// The single-width Grapheme to write. + /// The clipping rectangle. + private void WriteGrapheme (int col, int row, string grapheme, Rectangle clipRect) + { + Debug.Assert (grapheme.GetColumns () < 2); + Contents! [row, col].Grapheme = grapheme; + + // Mark the next cell as dirty to ensure proper rendering of adjacent content + if (col < clipRect.Right - 1 && col + 1 < Cols) { - return; + Contents [row, col + 1].IsDirty = true; } - Contents [row, col - 1].Grapheme = _column1ReplacementChar.ToString (); - Contents [row, col - 1].IsDirty = true; } /// @@ -302,25 +480,6 @@ private void WriteGraphemeByWidth (int col, int row, string text, int textWidth, } } - /// - /// INTERNAL: Writes a (0 or 1 column wide) Grapheme. - /// - /// The column. - /// The row. - /// The single-width Grapheme to write. - /// The clipping rectangle. - private void WriteGrapheme (int col, int row, string grapheme, Rectangle clipRect) - { - Debug.Assert (grapheme.GetColumns () < 2); - Contents! [row, col].Grapheme = grapheme; - - // Mark the next cell as dirty to ensure proper rendering of adjacent content - if (col < clipRect.Right - 1 && col + 1 < Cols) - { - Contents [row, col + 1].IsDirty = true; - } - } - /// /// INTERNAL: Writes a wide Grapheme (2 columns wide) handling clipping and partial overlap cases. /// @@ -349,134 +508,4 @@ private void WriteWideGrapheme (int col, int row, string grapheme) // See: https://github.com/gui-cs/Terminal.Gui/issues/4258 } } - - /// - /// Gets or sets a value indicating whether this buffer is operating in inline mode. - /// When , initialises cells with - /// IsDirty = false so that only cells explicitly drawn are flushed to the - /// terminal, leaving the rest of the visible terminal untouched. - /// - public bool InlineMode { get; set; } - - /// Clears the of the driver. - public void ClearContents () => ClearContents (!InlineMode); - - /// Clears the of the driver. - /// - /// When (the default), all cells are marked dirty so the first render - /// overwrites the entire screen. When (used in inline mode), cells start - /// clean so only cells that are explicitly drawn will be flushed, leaving the rest of the terminal - /// untouched. - /// - public void ClearContents (bool initiallyDirty) - { - Contents = new Cell [Rows, Cols]; - - // CONCURRENCY: Unsynchronized access to Clip isn't safe. - // TODO: ClearContents should not clear the clip; it should only clear the contents. Move clearing it elsewhere. - Clip = new Region (Screen); - - DirtyLines = new bool [Rows]; - - // Clear the URL map - _urlMap?.Clear (); - - lock (Contents) - { - for (var row = 0; row < Rows; row++) - { - for (var c = 0; c < Cols; c++) - { - Contents [row, c] = new Cell { Grapheme = " ", Attribute = new Attribute (Color.White, Color.Black), IsDirty = initiallyDirty }; - } - - DirtyLines [row] = initiallyDirty; - } - } - } - - /// Tests whether the specified coordinate are valid for drawing the specified Text. - /// Used to determine if one or two columns are required. - /// The column. - /// The row. - /// - /// if the coordinate is outside the screen bounds or outside of . - /// otherwise. - /// - public bool IsValidLocation (string text, int col, int row) - { - int textWidth = text.GetColumns (); - - return col >= 0 && row >= 0 && col + textWidth <= Cols && row < Rows && Clip!.Contains (col, row); - } - - /// - public void SetSize (int cols, int rows) - { - Cols = cols; - Rows = rows; - ClearContents (); - } - - /// - public void FillRect (Rectangle rect, Rune rune) - { - Rectangle clipBounds = Clip?.GetBounds () ?? Screen; - - // BUGBUG: This should be a method on Region - rect = Rectangle.Intersect (rect, clipBounds); - - lock (Contents!) - { - for (int r = rect.Y; r < rect.Y + rect.Height; r++) - { - for (int c = rect.X; c < rect.X + rect.Width; c++) - { - if (!IsValidLocation (rune.ToString (), c, r)) - { - continue; - } - - // We could call AddGrapheme here, but that would acquire the lock again. - // So we inline the logic instead. - SetAttributeAndDirty (c, r); - InvalidateOverlappedWideGlyph (c, r); - string grapheme = rune != default (Rune) ? rune.ToString () : " "; - WriteGraphemeByWidth (c, r, grapheme, grapheme.GetColumns (), clipBounds); - } - } - } - } - - /// - public void FillRect (Rectangle rect, char rune) - { - for (int y = rect.Top; y < rect.Top + rect.Height; y++) - { - for (int x = rect.Left; x < rect.Left + rect.Width; x++) - { - Move (x, y); - AddRune (rune); - } - } - } - - /// - /// Updates and to the specified column and row in . - /// Used by and to determine where to add content. - /// - /// - /// This does not move the cursor on the screen, it only updates the internal state of the driver. - /// - /// If or are negative or beyond and - /// , the method still sets those properties. - /// - /// - /// Column to move to. - /// Row to move to. - public void Move (int col, int row) - { - Col = col; - Row = row; - } } diff --git a/Tests/Benchmarks/ConsoleDrivers/OutputBuffer/OutputBufferBenchmark.cs b/Tests/Benchmarks/ConsoleDrivers/OutputBuffer/OutputBufferBenchmark.cs index c57c3a111b..cfe036f637 100644 --- a/Tests/Benchmarks/ConsoleDrivers/OutputBuffer/OutputBufferBenchmark.cs +++ b/Tests/Benchmarks/ConsoleDrivers/OutputBuffer/OutputBufferBenchmark.cs @@ -27,7 +27,7 @@ public class OutputBufferBenchmark [GlobalSetup] public void Setup () { - _buffer = new OutputBufferImpl (); + _buffer = new (); _buffer.SetSize (Cols, Rows); _buffer.CurrentAttribute = new Attribute (Color.White, Color.Black); } diff --git a/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs b/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs index ac86b7310d..51cbc3eda9 100644 --- a/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs +++ b/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs @@ -15,84 +15,6 @@ public class OutputBufferImplConcurrencyTests private const int ITERATIONS = 5_000; - /// - /// Runs a writer action and a clearer action concurrently, collecting any exceptions. - /// Returns the collected exceptions for assertion. - /// - private static ConcurrentBag RunConcurrent (Action writerAction, - Action clearerAction) - { - OutputBufferImpl buffer = new (); - buffer.SetSize (80, 25); - - ConcurrentBag exceptions = []; - CancellationTokenSource cts = new (); - - Thread writer = new (() => - { - try - { - for (var i = 0; i < ITERATIONS && !cts.IsCancellationRequested; i++) - { - try - { - writerAction (buffer, i, cts.Token); - } - catch (Exception ex) when (ex is not OutOfMemoryException) - { - exceptions.Add (ex); - - break; - } - } - } - catch (Exception ex) - { - exceptions.Add (ex); - } - }) { IsBackground = true }; - - Thread clearer = new (() => - { - try - { - for (var i = 0; i < ITERATIONS && !cts.IsCancellationRequested; i++) - { - try - { - clearerAction (buffer, i, cts.Token); - } - catch (Exception ex) when (ex is not OutOfMemoryException) - { - exceptions.Add (ex); - - break; - } - } - } - catch (Exception ex) - { - exceptions.Add (ex); - } - }) { IsBackground = true }; - - writer.Start (); - clearer.Start (); - - writer.Join (TimeSpan.FromSeconds (10)); - clearer.Join (TimeSpan.FromSeconds (10)); - cts.Cancel (); - - return exceptions; - } - - private static void AssertNoExceptions (ConcurrentBag exceptions) - { - Assert.True (exceptions.IsEmpty, - $"Caught {exceptions.Count} exception(s) during concurrent access. " - + $"First: {exceptions.FirstOrDefault ()?.GetType ().Name}: {exceptions.FirstOrDefault ()?.Message}"); - } - /// /// Concurrently calls and /// from separate threads. @@ -103,8 +25,7 @@ private static void AssertNoExceptions (ConcurrentBag exceptions) [Fact] public void AddStr_And_ClearContents_Concurrent_DoesNotThrow () { - ConcurrentBag exceptions = RunConcurrent ( - (buffer, i, _) => + ConcurrentBag exceptions = RunConcurrent ((buffer, i, _) => { buffer.Move (0, i % 25); buffer.AddStr (new string ('A', 80)); @@ -118,53 +39,6 @@ public void AddStr_And_ClearContents_Concurrent_DoesNotThrow () AssertNoExceptions (exceptions); } - /// - /// Concurrently calls and - /// from separate threads. - /// The FillRect(Rectangle, Rune) overload has the same broken lock (Contents!) pattern. - /// - [Fact] - public void FillRect_And_ClearContents_Concurrent_DoesNotThrow () - { - ConcurrentBag exceptions = RunConcurrent ( - (buffer, i, _) => - { - // Fill a region that fits within the smallest size (40x10) - buffer.FillRect (new Rectangle (0, 0, 30, 8), new Rune ('B')); - }, - (buffer, i, _) => - { - buffer.SetSize (i % 2 == 0 ? 80 : 40, i % 2 == 0 ? 25 : 10); - }); - - AssertNoExceptions (exceptions); - } - - /// - /// Concurrently calls , , - /// and from separate threads to verify that - /// interleaved Move + AddStr doesn't corrupt state when Contents is being replaced. - /// - [Fact] - public void Move_AddStr_And_ClearContents_Concurrent_DoesNotThrow () - { - ConcurrentBag exceptions = RunConcurrent ( - (buffer, i, _) => - { - // Rapidly move around and write short strings - buffer.Move (i % 40, i % 10); - buffer.AddStr ("Hello"); - buffer.Move ((i + 20) % 40, (i + 5) % 10); - buffer.AddStr ("World!"); - }, - (buffer, i, _) => - { - buffer.SetSize (i % 2 == 0 ? 80 : 40, i % 2 == 0 ? 25 : 10); - }); - - AssertNoExceptions (exceptions); - } - /// /// Runs and /// concurrently with to exercise all three write paths @@ -256,11 +130,169 @@ public void AddStr_FillRect_And_ClearContents_ThreeWay_Concurrent_DoesNotThrow ( fillRectThread.Start (); clearerThread.Start (); - addStrThread.Join (TimeSpan.FromSeconds (10)); - fillRectThread.Join (TimeSpan.FromSeconds (10)); - clearerThread.Join (TimeSpan.FromSeconds (10)); + TimeSpan joinTimeout = TimeSpan.FromSeconds (10); + bool addStrJoined = addStrThread.Join (joinTimeout); + bool fillRectJoined = fillRectThread.Join (joinTimeout); + bool clearerJoined = clearerThread.Join (joinTimeout); + cts.Cancel (); + if (!addStrJoined) + { + addStrJoined = addStrThread.Join (TimeSpan.FromSeconds (5)); + } + + if (!fillRectJoined) + { + fillRectJoined = fillRectThread.Join (TimeSpan.FromSeconds (5)); + } + + if (!clearerJoined) + { + clearerJoined = clearerThread.Join (TimeSpan.FromSeconds (5)); + } + + Assert.True (addStrJoined, "addStrThread did not stop within the timeout."); + Assert.True (fillRectJoined, "fillRectThread did not stop within the timeout."); + Assert.True (clearerJoined, "clearerThread did not stop within the timeout."); + + AssertNoExceptions (exceptions); + } + + /// + /// Concurrently calls and + /// from separate threads. + /// The FillRect(Rectangle, Rune) overload has the same broken lock (Contents!) pattern. + /// + [Fact] + public void FillRect_And_ClearContents_Concurrent_DoesNotThrow () + { + ConcurrentBag exceptions = RunConcurrent ((buffer, _, _) => + { + // Fill a region that fits within the smallest size (40x10) + buffer.FillRect (new Rectangle (0, 0, 30, 8), new Rune ('B')); + }, + (buffer, i, _) => { buffer.SetSize (i % 2 == 0 ? 80 : 40, i % 2 == 0 ? 25 : 10); }); + + AssertNoExceptions (exceptions); + } + + /// + /// Concurrently calls , , + /// and from separate threads to verify that + /// interleaved Move + AddStr doesn't corrupt state when Contents is being replaced. + /// + [Fact] + public void Move_AddStr_And_ClearContents_Concurrent_DoesNotThrow () + { + ConcurrentBag exceptions = RunConcurrent ((buffer, i, _) => + { + // Rapidly move around and write short strings + buffer.Move (i % 40, i % 10); + buffer.AddStr ("Hello"); + buffer.Move ((i + 20) % 40, (i + 5) % 10); + buffer.AddStr ("World!"); + }, + (buffer, i, _) => { buffer.SetSize (i % 2 == 0 ? 80 : 40, i % 2 == 0 ? 25 : 10); }); + AssertNoExceptions (exceptions); } + + private static void AssertNoExceptions (ConcurrentBag exceptions) => + Assert.True (exceptions.IsEmpty, + $"Caught {exceptions.Count} exception(s) during concurrent access. " + + $"First: {exceptions.FirstOrDefault ()?.GetType ().Name}: {exceptions.FirstOrDefault ()?.Message}"); + + /// + /// Runs a writer action and a clearer action concurrently, collecting any exceptions. + /// Returns the collected exceptions for assertion. + /// + private static ConcurrentBag RunConcurrent (Action writerAction, + Action clearerAction) + { + OutputBufferImpl buffer = new (); + buffer.SetSize (80, 25); + + ConcurrentBag exceptions = []; + CancellationTokenSource cts = new (); + + Thread writer = new (() => + { + try + { + for (var i = 0; i < ITERATIONS && !cts.IsCancellationRequested; i++) + { + try + { + writerAction (buffer, i, cts.Token); + } + catch (Exception ex) when (ex is not OutOfMemoryException) + { + exceptions.Add (ex); + + break; + } + } + } + catch (Exception ex) + { + exceptions.Add (ex); + } + }) { IsBackground = true }; + + Thread clearer = new (() => + { + try + { + for (var i = 0; i < ITERATIONS && !cts.IsCancellationRequested; i++) + { + try + { + clearerAction (buffer, i, cts.Token); + } + catch (Exception ex) when (ex is not OutOfMemoryException) + { + exceptions.Add (ex); + + break; + } + } + } + catch (Exception ex) + { + exceptions.Add (ex); + } + }) { IsBackground = true }; + + writer.Start (); + clearer.Start (); + + try + { + bool writerCompleted = writer.Join (TimeSpan.FromSeconds (10)); + bool clearerCompleted = clearer.Join (TimeSpan.FromSeconds (10)); + + if (!writerCompleted || !clearerCompleted) + { + cts.Cancel (); + + if (!writerCompleted && !writer.Join (TimeSpan.FromSeconds (5))) + { + exceptions.Add (new TimeoutException ("Writer thread did not stop after cancellation.")); + } + + if (!clearerCompleted && !clearer.Join (TimeSpan.FromSeconds (5))) + { + exceptions.Add (new TimeoutException ("Clearer thread did not stop after cancellation.")); + } + } + } + finally + { + cts.Cancel (); + cts.Dispose (); + } + + return exceptions; + } } diff --git a/plans/5130-output-buffer-race-condition.md b/plans/5130-output-buffer-race-condition.md deleted file mode 100644 index 841fb2323e..0000000000 --- a/plans/5130-output-buffer-race-condition.md +++ /dev/null @@ -1,240 +0,0 @@ -# Plan: Fix OutputBufferImpl Race Condition (#5130) - -**PR**: [#5131](https://github.com/gui-cs/Terminal.Gui/pull/5131) -**Branch**: `fix/5130-output-buffer-race-condition` - -## Problem Statement - -`OutputBufferImpl` has a race condition where `ClearContents()` replaces the `Contents` array -reference **outside** any lock, then acquires a lock on the **new** object. Concurrently, -`AddGrapheme()` acquires a lock on whatever `Contents` happens to point to. When both run at -the same time: - -1. `AddGrapheme` reads `Contents` (old reference), passes the null check -2. `ClearContents` swaps `Contents` to a new array -3. `AddGrapheme` locks on `Contents` — now the **new** object — same object `ClearContents` locks on -4. Both threads can proceed through the lock without mutual exclusion on the **old** array -5. The old array's `Cell` data gets partially overwritten, corrupting `Rune` values -6. Native `Wcwidth` crashes with `AccessViolationException` on the corrupted data - -The `FillRect(Rectangle, Rune)` overload has the identical `lock (Contents!)` anti-pattern. - -Additionally, `OutputBase.WriteToScreen()` reads `Contents` without any lock during screen -flush, creating a third concurrent access point. - -## Root Cause - -Locking on `Contents` itself is fundamentally broken because `ClearContents` **replaces** the -reference. A `lock(obj)` only provides mutual exclusion when all threads lock on the **same** -object instance. - -## Approach - -Introduce a **stable, `readonly` dedicated lock object** (`_contentsLock`) that is never -replaced. All code that reads or writes `Contents`, `DirtyLines`, `Clip`, or `_urlMap` must -do so inside `lock (_contentsLock)`. - -### Design Considerations - -1. **Lock granularity**: A single lock object is simplest and correct. The critical sections - are short (cell writes, array swaps). Performance impact should be negligible since - drawing is already serialized on the main thread — the lock primarily protects against - the driver resize path which runs on a different thread. - -2. **`Rows`/`Cols` setters**: Both call `ClearContents()` which will now acquire the lock. - The `SetSize()` method sets `Cols` then `Rows`, triggering two `ClearContents` calls. We - should consider having `SetSize` set backing fields directly and call `ClearContents` - once. However, this is an optimization — the lock is re-entrant safe... actually, - `Monitor.Enter` (what `lock` uses) is **re-entrant** in .NET, so the double call won't - deadlock, but it's wasteful. We should fix `SetSize` to avoid the double clear. - -3. **`OutputBase.WriteToScreen()`**: This reads `Contents` without any synchronization. The - fix should expose the lock or provide a synchronized accessor. However, since - `WriteToScreen` runs on the main thread during `Refresh()`, and `ClearContents` is the - only cross-thread mutator, the primary fix in `OutputBufferImpl` should suffice. We can - address `OutputBase` as a follow-up if needed. - -4. **`Move()`**: Sets `Col` and `Row` without the lock. Since `Move` + `AddGrapheme` are - always called sequentially from the same thread, and the lock protects `AddGrapheme` - internals, `Move` doesn't need the lock. However, `Col`/`Row` are read inside the lock - in `AddGrapheme` — we should verify no cross-thread `Move` calls exist. - -5. **The `FillRect(Rectangle, char)` overload** (line 452): This calls `Move` + `AddRune` - in a loop without a lock. It's protected indirectly because `AddGrapheme` acquires the - lock. No change needed for this overload. - -## Implementation Steps - -### Step 1: Add `_contentsLock` field - -Add a `private readonly object _contentsLock = new ();` field near the top of the class, -next to the existing `_cols` / `_rows` fields. - -### Step 2: Fix `ClearContents(bool)` - -Move **all** mutations (`Contents`, `Clip`, `DirtyLines`, `_urlMap`) inside -`lock (_contentsLock)`. The entire method body goes inside the lock. - -```csharp -public void ClearContents (bool initiallyDirty) -{ - lock (_contentsLock) - { - Contents = new Cell [Rows, Cols]; - Clip = new Region (Screen); - DirtyLines = new bool [Rows]; - _urlMap?.Clear (); - - for (var row = 0; row < Rows; row++) - { - for (var c = 0; c < Cols; c++) - { - Contents [row, c] = new Cell - { - Grapheme = " ", - Attribute = new Attribute (Color.White, Color.Black), - IsDirty = initiallyDirty - }; - } - - DirtyLines [row] = initiallyDirty; - } - } -} -``` - -### Step 3: Fix `AddGrapheme(string)` - -Replace `lock (Contents)` with `lock (_contentsLock)`. Add a double-checked null guard -inside the lock. Move `Clip` access inside the lock since `ClearContents` now replaces -`Clip` under the lock. - -```csharp -private void AddGrapheme (string grapheme) -{ - lock (_contentsLock) - { - if (Contents is null) - { - return; - } - - Clip ??= new Region (Screen); - Rectangle clipRect = Clip!.GetBounds (); - int printableGraphemeWidth = -1; - - if (IsValidLocation (grapheme, Col, Row)) - { - SetAttributeAndDirty (Col, Row); - InvalidateOverlappedWideGlyph (Col, Row); - - string printableGrapheme = grapheme.MakePrintable (); - printableGraphemeWidth = printableGrapheme.GetColumns (); - WriteGraphemeByWidth (Col, Row, printableGrapheme, printableGraphemeWidth, clipRect); - - DirtyLines [Row] = true; - } - - Col++; - - if (printableGraphemeWidth <= 1) - { - return; - } - - if (Clip.Contains (Col, Row)) - { - if (Contents [Row, Col].Attribute != CurrentAttribute) - { - Contents [Row, Col].Attribute = CurrentAttribute; - Contents [Row, Col].IsDirty = true; - } - } - - Col++; - } -} -``` - -### Step 4: Fix `FillRect(Rectangle, Rune)` - -Replace `lock (Contents!)` with `lock (_contentsLock)`. Add null guard inside. - -### Step 5: Fix `SetSize` to avoid double `ClearContents` - -Currently `SetSize` sets `Cols` then `Rows`, each triggering `ClearContents()` via the -property setters. Refactor to set backing fields directly and call `ClearContents` once: - -```csharp -public void SetSize (int cols, int rows) -{ - _cols = cols; - _rows = rows; - ClearContents (); -} -``` - -### Step 6: Verify the test passes - -Run the existing concurrency test: - -```bash -dotnet test --project Tests/UnitTestsParallelizable --no-build --filter-method "*AddStr_And_ClearContents_Concurrent_DoesNotThrow" -``` - -### Step 7: Run full test suite - -Ensure no regressions: - -```bash -dotnet test --project Tests/UnitTestsParallelizable --no-build -dotnet test --project Tests/UnitTests.NonParallelizable --no-build -``` - -### Step 8: Consider additional test coverage - -- Test that `FillRect` + `ClearContents` concurrent access doesn't throw -- Test that `Move` + `AddStr` + `ClearContents` produces consistent state - -## Files to Change - -| File | Change | -|------|--------| -| `Terminal.Gui/Drivers/Output/OutputBufferImpl.cs` | Add `_contentsLock`; fix `ClearContents`, `AddGrapheme`, `FillRect`, `SetSize` | - -## Risks & Mitigations - -| Risk | Mitigation | -|------|-----------| -| Lock contention slows drawing | Lock is only held for cell-level operations; main thread drawing is already serialized. Measure if concerned. | -| Deadlock from re-entrant lock in `SetSize` | .NET `Monitor` (used by `lock`) is re-entrant. Plus Step 5 eliminates the double call. | -| `OutputBase.WriteToScreen` still reads without lock | Runs on main thread during `Refresh()`; cross-thread mutation is now synchronized. Document as future improvement. | -| `Col`/`Row` modified outside lock by `Move()` | `Move()` is always called from the same thread as `AddGrapheme`. No cross-thread `Move` calls observed. | - -## Benchmark Baseline (Before Fix) - -Captured with `--job short` on Intel Core i9-14900K, .NET 10.0.6: - -| Method | Cols | Rows | Mean | Allocated | -|-------------------- |----- |----- |------------:|-----------:| -| AddStr_FullRow | 80 | 25 | 31.48 μs | 63.77 KB | -| AddStr_AllRows | 80 | 25 | 776.68 μs | 1590.02 KB | -| FillRect_FullScreen | 80 | 25 | 666.49 μs | 1634.18 KB | -| ClearContents | 80 | 25 | 75.76 μs | 63.09 KB | -| SetSize | 80 | 25 | 225.99 μs | 189.26 KB | -| TypicalDrawCycle | 80 | 25 | 859.06 μs | 1653.11 KB | -| AddStr_FullRow | 200 | 50 | 78.34 μs | 159.63 KB | -| AddStr_AllRows | 200 | 50 | 4,205.18 μs | 7961.35 KB | -| FillRect_FullScreen | 200 | 50 | 3,799.05 μs | 8190.23 KB | -| ClearContents | 200 | 50 | 444.25 μs | 313.27 KB | -| SetSize | 200 | 50 | 1,310.77 μs | 939.82 KB | -| TypicalDrawCycle | 200 | 50 | 5,512.82 μs | 8274.62 KB | - -The fix must not regress these numbers significantly. Re-run after implementing -and compare. - -## Out of Scope - -- Making `OutputBase.WriteToScreen` acquire the lock (would require exposing the lock or adding a synchronized read API — separate PR) -- Thread-safety for `Clip` property getter/setter beyond what's protected by `_contentsLock` -- General thread-safety audit of the driver layer From 207f9dc3b9b1b4c37246014b2515324d2fed9591 Mon Sep 17 00:00:00 2001 From: Tig Date: Thu, 30 Apr 2026 14:01:39 -0600 Subject: [PATCH 4/6] Refactor StringExtensions for style and consistency Refactored methods to use expression-bodied members and improved code style (constants, variable declarations, and formatting). Added and then removed `IsSurrogatePair` and `MakePrintable` extension methods, resulting in no net functional changes. All updates are stylistic and organizational. --- Terminal.Gui/Text/StringExtensions.cs | 112 +++++++++++++------------- 1 file changed, 58 insertions(+), 54 deletions(-) diff --git a/Terminal.Gui/Text/StringExtensions.cs b/Terminal.Gui/Text/StringExtensions.cs index 59f433b60d..4693a28d7d 100644 --- a/Terminal.Gui/Text/StringExtensions.cs +++ b/Terminal.Gui/Text/StringExtensions.cs @@ -91,7 +91,7 @@ public static int GetColumns (this string str, bool ignoreLessThanZero = true) /// This is a Terminal.Gui extension method to to support TUI text manipulation. /// The string to count. /// - public static int GetRuneCount (this string str) { return str.EnumerateRunes ().Count (); } + public static int GetRuneCount (this string str) => str.EnumerateRunes ().Count (); /// /// Determines if this of is composed entirely of ASCII @@ -102,7 +102,7 @@ public static int GetColumns (this string str, bool ignoreLessThanZero = true) /// A indicating if all elements of the are ASCII digits ( /// ) or not ( /// - public static bool IsAllAsciiDigits (this ReadOnlySpan stringSpan) { return !stringSpan.IsEmpty && stringSpan.ToString ().All (char.IsAsciiDigit); } + public static bool IsAllAsciiDigits (this ReadOnlySpan stringSpan) => !stringSpan.IsEmpty && stringSpan.ToString ().All (char.IsAsciiDigit); /// /// Determines if this of is composed entirely of ASCII @@ -113,7 +113,42 @@ public static int GetColumns (this string str, bool ignoreLessThanZero = true) /// A indicating if all elements of the are ASCII digits ( /// ) or not ( /// - public static bool IsAllAsciiHexDigits (this ReadOnlySpan stringSpan) { return !stringSpan.IsEmpty && stringSpan.ToString ().All (char.IsAsciiHexDigit); } + public static bool IsAllAsciiHexDigits (this ReadOnlySpan stringSpan) => !stringSpan.IsEmpty && stringSpan.ToString ().All (char.IsAsciiHexDigit); + + /// Reports whether a string is a surrogate code point. + /// This is a Terminal.Gui extension method to to support TUI text manipulation. + /// The string to probe. + /// if the string is a surrogate code point; otherwise. + public static bool IsSurrogatePair (this string str) + { + if (str.Length != 2) + { + return false; + } + + var rune = Rune.GetRuneAt (str, 0); + + return rune.IsSurrogatePair (); + } + + /// + /// Ensures the text is not a control character and can be displayed by translating characters below 0x20 to + /// equivalent, printable, Unicode chars. + /// + /// This is a Terminal.Gui extension method to to support TUI text manipulation. + /// The text. + /// + public static string MakePrintable (this string str) + { + if (str.Length > 1) + { + return str; + } + + char ch = str [0]; + + return char.IsControl (ch) ? new string ((char)(ch + 0x2400), 1) : str; + } /// Repeats the string times. /// This is a Terminal.Gui extension method to to support TUI text manipulation. @@ -132,30 +167,28 @@ public static int GetColumns (this string str, bool ignoreLessThanZero = true) return str; } - return new StringBuilder (str.Length * n) - .Insert (0, str, n) - .ToString (); + return new StringBuilder (str.Length * n).Insert (0, str, n).ToString (); } /// Converts the string into a . /// This is a Terminal.Gui extension method to to support TUI text manipulation. /// The string to convert. /// - public static List ToRuneList (this string str) { return str.EnumerateRunes ().ToList (); } + public static List ToRuneList (this string str) => str.EnumerateRunes ().ToList (); /// Converts the string into a array. /// This is a Terminal.Gui extension method to to support TUI text manipulation. /// The string to convert. /// - public static Rune [] ToRunes (this string str) { return str.EnumerateRunes ().ToArray (); } + public static Rune [] ToRunes (this string str) => str.EnumerateRunes ().ToArray (); /// Converts a generic collection into a string. /// The enumerable rune to convert. /// public static string ToString (IEnumerable runes) { - const int maxCharsPerRune = 2; - const int maxStackallocTextBufferSize = 1048; // ~2 kB + const int MAX_CHARS_PER_RUNE = 2; + const int MAX_STACKALLOC_TEXT_BUFFER_SIZE = 1048; // ~2 kB // If rune count is easily available use stackalloc buffer or alternatively rented array. if (runes.TryGetNonEnumeratedCount (out int count)) @@ -165,22 +198,26 @@ public static string ToString (IEnumerable runes) return string.Empty; } - char[]? rentedBufferArray = null; + char []? rentedBufferArray = null; + try { - int maxRequiredTextBufferSize = count * maxCharsPerRune; - Span textBuffer = maxRequiredTextBufferSize <= maxStackallocTextBufferSize - ? stackalloc char[maxRequiredTextBufferSize] - : (rentedBufferArray = ArrayPool.Shared.Rent(maxRequiredTextBufferSize)); + int maxRequiredTextBufferSize = count * MAX_CHARS_PER_RUNE; + + Span textBuffer = maxRequiredTextBufferSize <= MAX_STACKALLOC_TEXT_BUFFER_SIZE + ? stackalloc char [maxRequiredTextBufferSize] + : rentedBufferArray = ArrayPool.Shared.Rent (maxRequiredTextBufferSize); Span remainingBuffer = textBuffer; + foreach (Rune rune in runes) { int charsWritten = rune.EncodeToUtf16 (remainingBuffer); remainingBuffer = remainingBuffer [charsWritten..]; } - ReadOnlySpan text = textBuffer[..^remainingBuffer.Length]; + ReadOnlySpan text = textBuffer [..^remainingBuffer.Length]; + return text.ToString (); } finally @@ -193,14 +230,16 @@ public static string ToString (IEnumerable runes) } // Fallback to StringBuilder append. - StringBuilder stringBuilder = new(); - Span runeBuffer = stackalloc char[maxCharsPerRune]; + StringBuilder stringBuilder = new (); + Span runeBuffer = stackalloc char [MAX_CHARS_PER_RUNE]; + foreach (Rune rune in runes) { int charsWritten = rune.EncodeToUtf16 (runeBuffer); ReadOnlySpan runeChars = runeBuffer [..charsWritten]; stringBuilder.Append (runeChars); } + return stringBuilder.ToString (); } @@ -218,7 +257,7 @@ public static string ToString (IEnumerable bytes, Encoding? encoding = nul /// Converts a generic collection into a string. /// The enumerable string to convert. /// - public static string ToString (IEnumerable strings) { return string.Concat (strings); } + public static string ToString (IEnumerable strings) => string.Concat (strings); /// Converts the string into a . /// This is a Terminal.Gui extension method to to support TUI text manipulation. @@ -235,39 +274,4 @@ public static List ToStringList (this string str) return strings; } - - /// Reports whether a string is a surrogate code point. - /// This is a Terminal.Gui extension method to to support TUI text manipulation. - /// The string to probe. - /// if the string is a surrogate code point; otherwise. - public static bool IsSurrogatePair (this string str) - { - if (str.Length != 2) - { - return false; - } - - Rune rune = Rune.GetRuneAt (str, 0); - - return rune.IsSurrogatePair (); - } - - /// - /// Ensures the text is not a control character and can be displayed by translating characters below 0x20 to - /// equivalent, printable, Unicode chars. - /// - /// This is a Terminal.Gui extension method to to support TUI text manipulation. - /// The text. - /// - public static string MakePrintable (this string str) - { - if (str.Length > 1) - { - return str; - } - - char ch = str [0]; - - return char.IsControl (ch) ? new ((char)(ch + 0x2400), 1) : str; - } } From e682a2705ebaad722e64dd4d7892e467d875244e Mon Sep 17 00:00:00 2001 From: Tig Date: Fri, 1 May 2026 12:00:24 -0600 Subject: [PATCH 5/6] Fix race condition in FillRect(Rectangle, char) overload The char overload called Move() + AddRune() without holding _contentsLock, allowing concurrent threads to corrupt Col/Row between the two calls and write characters to wrong positions. Fix: delegate to FillRect(Rectangle, Rune) which holds the lock atomically. Added test proving the race (detected wrong-position writes in <13ms before fix). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Drivers/Output/OutputBufferImpl.cs | 9 +- .../OutputBufferImplConcurrencyTests.cs | 143 ++++++++++++++++++ 2 files changed, 144 insertions(+), 8 deletions(-) diff --git a/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs b/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs index 71eebd5038..f266cab9b0 100644 --- a/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs +++ b/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs @@ -242,14 +242,7 @@ public void FillRect (Rectangle rect, Rune rune) /// public void FillRect (Rectangle rect, char rune) { - for (int y = rect.Top; y < rect.Top + rect.Height; y++) - { - for (int x = rect.Left; x < rect.Left + rect.Width; x++) - { - Move (x, y); - AddRune (rune); - } - } + FillRect (rect, new Rune (rune)); } /// diff --git a/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs b/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs index 51cbc3eda9..1371ef99b8 100644 --- a/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs +++ b/Tests/UnitTestsParallelizable/Drivers/Output/OutputBufferImplConcurrencyTests.cs @@ -198,6 +198,149 @@ public void Move_AddStr_And_ClearContents_Concurrent_DoesNotThrow () AssertNoExceptions (exceptions); } + /// + /// Proves that has a race condition + /// because it calls Move + AddRune without holding _contentsLock across + /// the pair. A concurrent writer can change Col/Row between the Move and AddRune, + /// causing characters to be written at wrong positions. + /// + [Fact] + public void FillRect_Char_NonAtomic_Move_AddRune_Causes_Wrong_Position () + { + // Copilot + // This test proves that FillRect(Rectangle, char) is not atomic: + // Two threads both call FillRect(char) targeting different rows. If Move+AddRune + // were atomic, each row should contain only its own character. Because they + // are NOT atomic, Col/Row shared state gets corrupted between threads. + OutputBufferImpl buffer = new (); + buffer.SetSize (80, 25); + + ConcurrentBag exceptions = []; + var wrongPositionDetected = false; + + const int iterations = 50_000; + CancellationTokenSource cts = new (); + + // Thread 1: fills row 0, cols 0-79 with 'A' + Thread thread1 = new (() => + { + try + { + for (var i = 0; i < iterations && !cts.IsCancellationRequested; i++) + { + buffer.FillRect (new Rectangle (0, 0, 80, 1), 'A'); + } + } + catch (Exception ex) when (ex is not OutOfMemoryException) + { + exceptions.Add (ex); + } + }) { IsBackground = true }; + + // Thread 2: fills row 20, cols 0-79 with 'B' + Thread thread2 = new (() => + { + try + { + for (var i = 0; i < iterations && !cts.IsCancellationRequested; i++) + { + buffer.FillRect (new Rectangle (0, 20, 80, 1), 'B'); + } + } + catch (Exception ex) when (ex is not OutOfMemoryException) + { + exceptions.Add (ex); + } + }) { IsBackground = true }; + + // Checker thread: looks for 'B' in row 0 or 'A' in row 20 + Thread checker = new (() => + { + try + { + while (!cts.IsCancellationRequested) + { + Cell [,]? contents = buffer.Contents; + + if (contents is null) + { + continue; + } + + // Check if row 0 has 'B' (should only have 'A' or space) + for (var c = 0; c < 80; c++) + { + string grapheme = contents [0, c].Grapheme; + + if (grapheme == "B") + { + wrongPositionDetected = true; + cts.Cancel (); + + return; + } + } + + // Check if row 20 has 'A' (should only have 'B' or space) + for (var c = 0; c < 80; c++) + { + string grapheme = contents [20, c].Grapheme; + + if (grapheme == "A") + { + wrongPositionDetected = true; + cts.Cancel (); + + return; + } + } + } + } + catch + { + // Buffer access may race; ignore exceptions in checker + } + }) { IsBackground = true }; + + thread1.Start (); + thread2.Start (); + checker.Start (); + + thread1.Join (TimeSpan.FromSeconds (10)); + thread2.Join (TimeSpan.FromSeconds (10)); + cts.Cancel (); + checker.Join (TimeSpan.FromSeconds (5)); + + // The test proves the bug exists: wrong position IS detected due to the race. + // After the fix, this should no longer detect wrong positions. + Assert.False (wrongPositionDetected, + "FillRect(Rectangle, char) wrote characters to wrong positions due to non-atomic Move+AddRune."); + } + + /// + /// Concurrently calls and + /// from separate threads. + /// The FillRect(Rectangle, char) overload lacks locking, so the non-atomic + /// Move + AddRune pair can be interleaved with buffer resizing, + /// causing IndexOutOfRangeException or NullReferenceException. + /// + [Fact] + public void FillRect_Char_And_SetSize_Concurrent_DoesNotThrow () + { + // Copilot + ConcurrentBag exceptions = RunConcurrent ((buffer, i, _) => + { + // Use the char overload which lacks a lock + buffer.FillRect (new Rectangle (0, 0, 30, 8), 'C'); + }, + (buffer, i, _) => + { + buffer.SetSize (i % 2 == 0 ? 80 : 40, i % 2 == 0 ? 25 : 10); + }); + + AssertNoExceptions (exceptions); + } + private static void AssertNoExceptions (ConcurrentBag exceptions) => Assert.True (exceptions.IsEmpty, $"Caught {exceptions.Count} exception(s) during concurrent access. " From 88ee73fe791aef54840e272ce0cab5a51ff193d8 Mon Sep 17 00:00:00 2001 From: Tig Date: Fri, 1 May 2026 16:40:59 -0600 Subject: [PATCH 6/6] Added null check in OutputBufferImpl.cs WriteGrapheme. Introduced locking in RuneExtensions.cs for thread-safe Unicode width calculation. --- Examples/UICatalog/Scenarios/TableEditor.cs | 226 +++++++++--------- Terminal.Gui/App/ApplicationImpl.Screen.cs | 7 +- Terminal.Gui/App/Mouse/ApplicationMouse.cs | 99 ++++---- .../Drivers/Output/OutputBufferImpl.cs | 5 + Terminal.Gui/Text/RuneExtensions.cs | 10 +- 5 files changed, 179 insertions(+), 168 deletions(-) diff --git a/Examples/UICatalog/Scenarios/TableEditor.cs b/Examples/UICatalog/Scenarios/TableEditor.cs index 955d778eed..336cfa4b7d 100644 --- a/Examples/UICatalog/Scenarios/TableEditor.cs +++ b/Examples/UICatalog/Scenarios/TableEditor.cs @@ -2,6 +2,7 @@ using System.Data; using System.Globalization; using System.Text; + // ReSharper disable StringLiteralTypo namespace UICatalog.Scenarios; @@ -13,7 +14,6 @@ namespace UICatalog.Scenarios; [ScenarioCategory ("Text and Formatting")] public class TableEditor : Scenario { - private IApplication? _app; private readonly HashSet? _checkedFileSystemInfos = []; private readonly List? _toDispose = []; @@ -163,6 +163,8 @@ public class TableEditor : Scenario new UnicodeRange (0xE0000, 0xE007F, "Tags") ]; + private IApplication? _app; + private Scheme? _alternatingScheme; private DataTable? _currentTable; private Scheme? _redScheme; @@ -259,7 +261,7 @@ public override void Main () _tableView!.Accepted += EditCurrentCell; _tableView!.KeyDown += TableViewKeyPress; - //SetupScrollBar (); + // SetupScrollBar (); _redScheme = new Scheme { @@ -322,6 +324,83 @@ public override void Main () app.Run (appWindow); } + protected override void Dispose (bool disposing) + { + base.Dispose (disposing); + + foreach (IDisposable d in _toDispose!) + { + d.Dispose (); + } + } + + private DataTable BuildUnicodeMap () + { + var dt = new DataTable (); + + // add cols called 0 to 9 + for (var i = 0; i < 10; i++) + { + DataColumn col = dt.Columns.Add (i.ToString (), typeof (uint)); + ColumnStyle style = _tableView!.Style.GetOrCreateColumnStyle (col.Ordinal); + + style.RepresentationGetter = RuneToString; + } + + // add cols called a to z + for (int i = 'a'; i < 'a' + 26; i++) + { + DataColumn col = dt.Columns.Add (((char)i).ToString (), typeof (uint)); + ColumnStyle style = _tableView!.Style.GetOrCreateColumnStyle (col.Ordinal); + style.RepresentationGetter = RuneToString; + } + + // now add table contents + List runes = []; + + foreach (UnicodeRange range in _ranges!) + { + for (uint i = range.Start; i <= range.End; i++) + { + runes.Add (i); + } + } + + DataRow? dr = null; + + for (var i = 0; i < runes.Count; i++) + { + if (dr == null || i % dt.Columns.Count == 0) + { + dr = dt.Rows.Add (); + } + + dr [i % dt.Columns.Count] = runes [i]; + } + + return dt; + } + + private void CheckOrUncheckFile (FileSystemInfo info, bool check) + { + if (check) + { + _checkedFileSystemInfos!.Add (info); + } + else + { + _checkedFileSystemInfos!.Remove (info); + } + } + + private void ClearColumnStyles () + { + _tableView!.Style.ColumnStyles.Clear (); + _tableView!.Update (); + } + + private void CloseExample () => _tableView!.Table = null; + private MenuBarItem CreateViewMenu () { // Store checkbox references for the toggle methods to access @@ -643,85 +722,6 @@ MenuItem CreateOptionSelectorMenuItem (string title, T initialState, Action runes = new (); - - foreach (UnicodeRange range in _ranges!) - { - for (uint i = range.Start; i <= range.End; i++) - { - runes.Add (i); - } - } - - DataRow? dr = null; - - for (var i = 0; i < runes.Count; i++) - { - if (dr == null || i % dt.Columns.Count == 0) - { - dr = dt.Rows.Add (); - } - - dr [i % dt.Columns.Count] = runes [i].ToString (); - } - - return dt; - } - - private string RuneToString (object o) => Rune.TryCreate ((uint)o, out Rune rune) ? rune.ToString () : " "; - - private void CheckOrUncheckFile (FileSystemInfo info, bool check) - { - if (check) - { - _checkedFileSystemInfos!.Add (info); - } - else - { - _checkedFileSystemInfos!.Remove (info); - } - } - - private void ClearColumnStyles () - { - _tableView!.Style.ColumnStyles.Clear (); - _tableView!.Update (); - } - - private void CloseExample () => _tableView!.Table = null; - private void EditCurrentCell (object? sender, CommandEventArgs args) { if (_tableView?.Table is not DataTableSource || _currentTable == null) @@ -884,36 +884,6 @@ private void OpenExample (bool big) private void OpenSimple (bool big) => SetTable (BuildSimpleDataTable (big ? 30 : 5, big ? 1000 : 5)); - // Demonstrates the fix for #5072: a column with very wide content used to consume all viewport - // space and push later columns off-screen. With the fix, "Description" is clamped so "Status" and - // "Owner" remain visible at their header widths. - private void OpenWideColumnExample () - { - DataTable dt = new (); - dt.Columns.Add ("Id", typeof (int)); - dt.Columns.Add ("Description", typeof (string)); - dt.Columns.Add ("Status", typeof (string)); - dt.Columns.Add ("Owner", typeof (string)); - - string [] statuses = ["Open", "InProgress", "Blocked", "Done"]; - string [] owners = ["Alice", "Bob", "Carol", "Dan"]; - - for (var i = 0; i < 25; i++) - { - dt.Rows.Add ( - i, - $"Row {i}: " + new string ('x', 120 + i % 40), - statuses [i % statuses.Length], - owners [i % owners.Length]); - } - - SetTable (dt); - - // Clear any styles inherited from a previous example - _tableView!.Style.ColumnStyles.Clear (); - _tableView!.Update (); - } - private void OpenTreeExample () { _tableView!.Style.ColumnStyles.Clear (); @@ -960,6 +930,32 @@ private void OpenUnicodeMap () _tableView?.Update (); } + // Demonstrates the fix for #5072: a column with very wide content used to consume all viewport + // space and push later columns off-screen. With the fix, "Description" is clamped so "Status" and + // "Owner" remain visible at their header widths. + private void OpenWideColumnExample () + { + DataTable dt = new (); + dt.Columns.Add ("Id", typeof (int)); + dt.Columns.Add ("Description", typeof (string)); + dt.Columns.Add ("Status", typeof (string)); + dt.Columns.Add ("Owner", typeof (string)); + + string [] statuses = ["Open", "InProgress", "Blocked", "Done"]; + string [] owners = ["Alice", "Bob", "Carol", "Dan"]; + + for (var i = 0; i < 25; i++) + { + dt.Rows.Add (i, $"Row {i}: " + new string ('x', 120 + i % 40), statuses [i % statuses.Length], owners [i % owners.Length]); + } + + SetTable (dt); + + // Clear any styles inherited from a previous example + _tableView!.Style.ColumnStyles.Clear (); + _tableView!.Update (); + } + private void Quit () => _tableView?.App?.RequestStop (); private void RunColumnWidthDialog (int? col, string prompt, Action setter, Func getter) @@ -1000,6 +996,8 @@ private void RunColumnWidthDialog (int? col, string prompt, Action Rune.TryCreate ((uint)o, out Rune rune) ? rune.ToString () : " "; + private void SetDemoTableStyles () { _tableView!.Style.ColumnStyles.Clear (); @@ -1084,8 +1082,8 @@ private void SetMinWidth () private void SetTable (DataTable dataTable) => _tableView!.Table = new DataTableSource (_currentTable = dataTable); - //private void SetupScrollBar () - //{ + // private void SetupScrollBar () + // { // var scrollBar = new ScrollBarView (_tableView, true); // scrollBar.ChangedPosition += (s, e) => @@ -1117,7 +1115,7 @@ private void SetMinWidth () // //scrollBar.OtherScrollBarView.Position = tableView.LeftItem; // scrollBar.Refresh (); // }; - //} + // } private void ShowAllColumns () { diff --git a/Terminal.Gui/App/ApplicationImpl.Screen.cs b/Terminal.Gui/App/ApplicationImpl.Screen.cs index b0f986d460..98c4492769 100644 --- a/Terminal.Gui/App/ApplicationImpl.Screen.cs +++ b/Terminal.Gui/App/ApplicationImpl.Screen.cs @@ -1,9 +1,8 @@ +using System.Diagnostics; using Terminal.Gui.Tracing; namespace Terminal.Gui.App; -using Trace = Trace; - internal partial class ApplicationImpl { /// @@ -102,7 +101,7 @@ private void Driver_SizeChanged (object? sender, SizeChangedEventArgs e) /// public void LayoutAndDraw (bool forceRedraw = false) { - Trace.Draw ("ApplicationImpl", "Start", $"forceRedraw={forceRedraw}, Screen={Screen}, _inlineScreenSized={_inlineScreenSized}"); + Tracing.Trace.Draw ("ApplicationImpl", "Start", $"forceRedraw={forceRedraw}, Screen={Screen}, _inlineScreenSized={_inlineScreenSized}"); if (ClearScreenNextIteration) { @@ -284,7 +283,7 @@ public void LayoutAndDraw (bool forceRedraw = false) { LayoutAndDrawComplete?.Invoke (this, EventArgs.Empty); } - Trace.Draw ("ApplicationImpl", "End", $"neededLayout={neededLayout}, needsDraw={needsDraw}"); + Tracing.Trace.Draw ("ApplicationImpl", "End", $"neededLayout={neededLayout}, needsDraw={needsDraw}"); } /// diff --git a/Terminal.Gui/App/Mouse/ApplicationMouse.cs b/Terminal.Gui/App/Mouse/ApplicationMouse.cs index 413b18b8e7..f6c101d756 100644 --- a/Terminal.Gui/App/Mouse/ApplicationMouse.cs +++ b/Terminal.Gui/App/Mouse/ApplicationMouse.cs @@ -1,5 +1,6 @@ using System.ComponentModel; -using Terminal.Gui.Tracing; +using System.Diagnostics; +using Trace = Terminal.Gui.Tracing.Trace; namespace Terminal.Gui.App; @@ -21,21 +22,6 @@ public ApplicationMouse () => // Subscribe to Application static property change events Application.IsMouseDisabledChanged += OnIsMouseDisabledChanged; - /// - public IApplication? App { get; set; } - - /// - public Point? LastMousePosition { get; set; } - - /// - public bool IsMouseDisabled { get; set; } - - // Event handler for Application static property changes - private void OnIsMouseDisabledChanged (object? sender, ValueChangedEventArgs e) => IsMouseDisabled = e.NewValue; - - /// - public List CachedViewsUnderMouse { get; } = []; - /// /// The popover that was just dismissed by a mouse-press-outside event. /// Used to prevent re-entrant show of the same popover during the @@ -58,12 +44,26 @@ public ApplicationMouse () => /// private bool _isDismissRecursing; - /// - /// Gets the popover that was just dismissed by a mouse-press-outside event, if any. - /// Checked by to suppress re-show during the - /// same press → release → click cycle. - /// - internal IPopoverView? DismissedByMousePress => _dismissedByMousePress; + /// + public void Dispose () + { + ResetState (); + + // Unsubscribe from Application static property change events + Application.IsMouseDisabledChanged -= OnIsMouseDisabledChanged; + } + + /// + public IApplication? App { get; set; } + + /// + public Point? LastMousePosition { get; set; } + + /// + public bool IsMouseDisabled { get; set; } + + /// + public List CachedViewsUnderMouse { get; } = []; /// public event EventHandler? MouseEvent; @@ -288,13 +288,6 @@ public void RaiseMouseEvent (Mouse mouseEvent) } } - /// - /// Returns when the mouse is currently grabbed by a view - /// that belongs to 's view hierarchy. - /// - private bool IsGrabbedByViewInHierarchy (View hierarchyRoot) => - _mouseGrabViewRef?.TryGetTarget (out View? grabbed) is true && View.IsInHierarchy (hierarchyRoot, grabbed, true); - /// public void RaiseMouseEnterLeaveEvents (Point screenPosition, List currentViewsUnderMouse) { @@ -364,6 +357,34 @@ public void RaiseMouseEnterLeaveEvents (Point screenPosition, List curren } } + /// + public void ResetState () + { + // Do not clear LastMousePosition; Popovers require it to stay set with last mouse pos. + CachedViewsUnderMouse.Clear (); + MouseEvent = null; + _mouseGrabViewRef = null; + _dismissedByMousePress = null; + _isDismissRecursing = false; + } + + /// + /// Gets the popover that was just dismissed by a mouse-press-outside event, if any. + /// Checked by to suppress re-show during the + /// same press → release → click cycle. + /// + internal IPopoverView? DismissedByMousePress => _dismissedByMousePress; + + /// + /// Returns when the mouse is currently grabbed by a view + /// that belongs to 's view hierarchy. + /// + private bool IsGrabbedByViewInHierarchy (View hierarchyRoot) => + _mouseGrabViewRef?.TryGetTarget (out View? grabbed) is true && View.IsInHierarchy (hierarchyRoot, grabbed, true); + + // Event handler for Application static property changes + private void OnIsMouseDisabledChanged (object? sender, ValueChangedEventArgs e) => IsMouseDisabled = e.NewValue; + #region IMouseGrabHandler Implementation private WeakReference? _mouseGrabViewRef; @@ -546,24 +567,4 @@ public bool HandleMouseGrab (View? deepestViewUnderMouse, Mouse mouse) } #endregion IMouseGrabHandler Implementation - - /// - public void ResetState () - { - // Do not clear LastMousePosition; Popovers require it to stay set with last mouse pos. - CachedViewsUnderMouse.Clear (); - MouseEvent = null; - _mouseGrabViewRef = null; - _dismissedByMousePress = null; - _isDismissRecursing = false; - } - - /// - public void Dispose () - { - ResetState (); - - // Unsubscribe from Application static property change events - Application.IsMouseDisabledChanged -= OnIsMouseDisabledChanged; - } } diff --git a/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs b/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs index f266cab9b0..9a40d2153d 100644 --- a/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs +++ b/Terminal.Gui/Drivers/Output/OutputBufferImpl.cs @@ -431,6 +431,11 @@ private void SetCellUrl (int col, int row, string url) /// The clipping rectangle. private void WriteGrapheme (int col, int row, string grapheme, Rectangle clipRect) { + if (grapheme is null) + { + return; + } + Debug.Assert (grapheme.GetColumns () < 2); Contents! [row, col].Grapheme = grapheme; diff --git a/Terminal.Gui/Text/RuneExtensions.cs b/Terminal.Gui/Text/RuneExtensions.cs index c8a273c834..ffa20ddb62 100644 --- a/Terminal.Gui/Text/RuneExtensions.cs +++ b/Terminal.Gui/Text/RuneExtensions.cs @@ -7,6 +7,8 @@ namespace Terminal.Gui.Text; /// Extends to support TUI text manipulation. public static class RuneExtensions { + private static readonly Lock _wcwidthLock = new Lock (); + /// Maximum Unicode code point. public static readonly int MaxUnicodeCodePoint = 0x10FFFF; @@ -125,7 +127,13 @@ public static bool EncodeSurrogatePair (char highSurrogate, char lowSurrogate, o /// The number of columns required to fit the rune, 0 if the argument is the null character, or -1 if the value is /// not printable, otherwise the number of columns that the rune occupies. /// - public static int GetColumns (this Rune rune) { return UnicodeCalculator.GetWidth (rune); } + public static int GetColumns (this Rune rune) + { + lock (_wcwidthLock) + { + return UnicodeCalculator.GetWidth (rune); + } + } /// Get number of bytes required to encode the rune, based on the provided encoding. /// This is a Terminal.Gui extension method to to support TUI text manipulation.