Skip to content

Commit 57f49fd

Browse files
tigCopilotBDisp
authored
Fixes #5130. Fix OutputBufferImpl race condition on Contents reference (#5131)
* 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> * 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> * 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. * 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. * 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> * Added null check in OutputBufferImpl.cs WriteGrapheme. Introduced locking in RuneExtensions.cs for thread-safe Unicode width calculation. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: BDisp <bd.bdisp@gmail.com>
1 parent 688c292 commit 57f49fd

8 files changed

Lines changed: 993 additions & 413 deletions

File tree

‎Examples/UICatalog/Scenarios/TableEditor.cs‎

Lines changed: 112 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
using System.Data;
33
using System.Globalization;
44
using System.Text;
5+
56
// ReSharper disable StringLiteralTypo
67

78
namespace UICatalog.Scenarios;
@@ -13,7 +14,6 @@ namespace UICatalog.Scenarios;
1314
[ScenarioCategory ("Text and Formatting")]
1415
public class TableEditor : Scenario
1516
{
16-
private IApplication? _app;
1717
private readonly HashSet<FileSystemInfo>? _checkedFileSystemInfos = [];
1818
private readonly List<IDisposable>? _toDispose = [];
1919

@@ -163,6 +163,8 @@ public class TableEditor : Scenario
163163
new UnicodeRange (0xE0000, 0xE007F, "Tags")
164164
];
165165

166+
private IApplication? _app;
167+
166168
private Scheme? _alternatingScheme;
167169
private DataTable? _currentTable;
168170
private Scheme? _redScheme;
@@ -259,7 +261,7 @@ public override void Main ()
259261
_tableView!.Accepted += EditCurrentCell;
260262
_tableView!.KeyDown += TableViewKeyPress;
261263

262-
//SetupScrollBar ();
264+
// SetupScrollBar ();
263265

264266
_redScheme = new Scheme
265267
{
@@ -322,6 +324,83 @@ public override void Main ()
322324
app.Run (appWindow);
323325
}
324326

327+
protected override void Dispose (bool disposing)
328+
{
329+
base.Dispose (disposing);
330+
331+
foreach (IDisposable d in _toDispose!)
332+
{
333+
d.Dispose ();
334+
}
335+
}
336+
337+
private DataTable BuildUnicodeMap ()
338+
{
339+
var dt = new DataTable ();
340+
341+
// add cols called 0 to 9
342+
for (var i = 0; i < 10; i++)
343+
{
344+
DataColumn col = dt.Columns.Add (i.ToString (), typeof (uint));
345+
ColumnStyle style = _tableView!.Style.GetOrCreateColumnStyle (col.Ordinal);
346+
347+
style.RepresentationGetter = RuneToString;
348+
}
349+
350+
// add cols called a to z
351+
for (int i = 'a'; i < 'a' + 26; i++)
352+
{
353+
DataColumn col = dt.Columns.Add (((char)i).ToString (), typeof (uint));
354+
ColumnStyle style = _tableView!.Style.GetOrCreateColumnStyle (col.Ordinal);
355+
style.RepresentationGetter = RuneToString;
356+
}
357+
358+
// now add table contents
359+
List<uint> runes = [];
360+
361+
foreach (UnicodeRange range in _ranges!)
362+
{
363+
for (uint i = range.Start; i <= range.End; i++)
364+
{
365+
runes.Add (i);
366+
}
367+
}
368+
369+
DataRow? dr = null;
370+
371+
for (var i = 0; i < runes.Count; i++)
372+
{
373+
if (dr == null || i % dt.Columns.Count == 0)
374+
{
375+
dr = dt.Rows.Add ();
376+
}
377+
378+
dr [i % dt.Columns.Count] = runes [i];
379+
}
380+
381+
return dt;
382+
}
383+
384+
private void CheckOrUncheckFile (FileSystemInfo info, bool check)
385+
{
386+
if (check)
387+
{
388+
_checkedFileSystemInfos!.Add (info);
389+
}
390+
else
391+
{
392+
_checkedFileSystemInfos!.Remove (info);
393+
}
394+
}
395+
396+
private void ClearColumnStyles ()
397+
{
398+
_tableView!.Style.ColumnStyles.Clear ();
399+
_tableView!.Update ();
400+
}
401+
402+
private void CloseExample () => _tableView!.Table = null;
403+
325404
private MenuBarItem CreateViewMenu ()
326405
{
327406
// Store checkbox references for the toggle methods to access
@@ -643,85 +722,6 @@ MenuItem CreateOptionSelectorMenuItem<T> (string title, T initialState, Action<T
643722
}
644723
}
645724

646-
protected override void Dispose (bool disposing)
647-
{
648-
base.Dispose (disposing);
649-
650-
foreach (IDisposable d in _toDispose!)
651-
{
652-
d.Dispose ();
653-
}
654-
}
655-
656-
private DataTable BuildUnicodeMap ()
657-
{
658-
var dt = new DataTable ();
659-
660-
// add cols called 0 to 9
661-
for (var i = 0; i < 10; i++)
662-
{
663-
DataColumn col = dt.Columns.Add (i.ToString (), typeof (uint));
664-
ColumnStyle style = _tableView!.Style.GetOrCreateColumnStyle (col.Ordinal);
665-
666-
style.RepresentationGetter = RuneToString;
667-
}
668-
669-
// add cols called a to z
670-
for (int i = 'a'; i < 'a' + 26; i++)
671-
{
672-
DataColumn col = dt.Columns.Add (((char)i).ToString (), typeof (uint));
673-
ColumnStyle style = _tableView!.Style.GetOrCreateColumnStyle (col.Ordinal);
674-
style.RepresentationGetter = RuneToString;
675-
}
676-
677-
// now add table contents
678-
List<uint> runes = new ();
679-
680-
foreach (UnicodeRange range in _ranges!)
681-
{
682-
for (uint i = range.Start; i <= range.End; i++)
683-
{
684-
runes.Add (i);
685-
}
686-
}
687-
688-
DataRow? dr = null;
689-
690-
for (var i = 0; i < runes.Count; i++)
691-
{
692-
if (dr == null || i % dt.Columns.Count == 0)
693-
{
694-
dr = dt.Rows.Add ();
695-
}
696-
697-
dr [i % dt.Columns.Count] = runes [i].ToString ();
698-
}
699-
700-
return dt;
701-
}
702-
703-
private string RuneToString (object o) => Rune.TryCreate ((uint)o, out Rune rune) ? rune.ToString () : " ";
704-
705-
private void CheckOrUncheckFile (FileSystemInfo info, bool check)
706-
{
707-
if (check)
708-
{
709-
_checkedFileSystemInfos!.Add (info);
710-
}
711-
else
712-
{
713-
_checkedFileSystemInfos!.Remove (info);
714-
}
715-
}
716-
717-
private void ClearColumnStyles ()
718-
{
719-
_tableView!.Style.ColumnStyles.Clear ();
720-
_tableView!.Update ();
721-
}
722-
723-
private void CloseExample () => _tableView!.Table = null;
724-
725725
private void EditCurrentCell (object? sender, CommandEventArgs args)
726726
{
727727
if (_tableView?.Table is not DataTableSource || _currentTable == null)
@@ -884,36 +884,6 @@ private void OpenExample (bool big)
884884

885885
private void OpenSimple (bool big) => SetTable (BuildSimpleDataTable (big ? 30 : 5, big ? 1000 : 5));
886886

887-
// Demonstrates the fix for #5072: a column with very wide content used to consume all viewport
888-
// space and push later columns off-screen. With the fix, "Description" is clamped so "Status" and
889-
// "Owner" remain visible at their header widths.
890-
private void OpenWideColumnExample ()
891-
{
892-
DataTable dt = new ();
893-
dt.Columns.Add ("Id", typeof (int));
894-
dt.Columns.Add ("Description", typeof (string));
895-
dt.Columns.Add ("Status", typeof (string));
896-
dt.Columns.Add ("Owner", typeof (string));
897-
898-
string [] statuses = ["Open", "InProgress", "Blocked", "Done"];
899-
string [] owners = ["Alice", "Bob", "Carol", "Dan"];
900-
901-
for (var i = 0; i < 25; i++)
902-
{
903-
dt.Rows.Add (
904-
i,
905-
$"Row {i}: " + new string ('x', 120 + i % 40),
906-
statuses [i % statuses.Length],
907-
owners [i % owners.Length]);
908-
}
909-
910-
SetTable (dt);
911-
912-
// Clear any styles inherited from a previous example
913-
_tableView!.Style.ColumnStyles.Clear ();
914-
_tableView!.Update ();
915-
}
916-
917887
private void OpenTreeExample ()
918888
{
919889
_tableView!.Style.ColumnStyles.Clear ();
@@ -960,6 +930,32 @@ private void OpenUnicodeMap ()
960930
_tableView?.Update ();
961931
}
962932

933+
// Demonstrates the fix for #5072: a column with very wide content used to consume all viewport
934+
// space and push later columns off-screen. With the fix, "Description" is clamped so "Status" and
935+
// "Owner" remain visible at their header widths.
936+
private void OpenWideColumnExample ()
937+
{
938+
DataTable dt = new ();
939+
dt.Columns.Add ("Id", typeof (int));
940+
dt.Columns.Add ("Description", typeof (string));
941+
dt.Columns.Add ("Status", typeof (string));
942+
dt.Columns.Add ("Owner", typeof (string));
943+
944+
string [] statuses = ["Open", "InProgress", "Blocked", "Done"];
945+
string [] owners = ["Alice", "Bob", "Carol", "Dan"];
946+
947+
for (var i = 0; i < 25; i++)
948+
{
949+
dt.Rows.Add (i, $"Row {i}: " + new string ('x', 120 + i % 40), statuses [i % statuses.Length], owners [i % owners.Length]);
950+
}
951+
952+
SetTable (dt);
953+
954+
// Clear any styles inherited from a previous example
955+
_tableView!.Style.ColumnStyles.Clear ();
956+
_tableView!.Update ();
957+
}
958+
963959
private void Quit () => _tableView?.App?.RequestStop ();
964960

965961
private void RunColumnWidthDialog (int? col, string prompt, Action<ColumnStyle, int> setter, Func<ColumnStyle, int> getter)
@@ -1000,6 +996,8 @@ private void RunColumnWidthDialog (int? col, string prompt, Action<ColumnStyle,
1000996
_tableView!.Update ();
1001997
}
1002998

999+
private string RuneToString (object o) => Rune.TryCreate ((uint)o, out Rune rune) ? rune.ToString () : " ";
1000+
10031001
private void SetDemoTableStyles ()
10041002
{
10051003
_tableView!.Style.ColumnStyles.Clear ();
@@ -1084,8 +1082,8 @@ private void SetMinWidth ()
10841082

10851083
private void SetTable (DataTable dataTable) => _tableView!.Table = new DataTableSource (_currentTable = dataTable);
10861084

1087-
//private void SetupScrollBar ()
1088-
//{
1085+
// private void SetupScrollBar ()
1086+
// {
10891087
// var scrollBar = new ScrollBarView (_tableView, true);
10901088

10911089
// scrollBar.ChangedPosition += (s, e) =>
@@ -1117,7 +1115,7 @@ private void SetMinWidth ()
11171115
// //scrollBar.OtherScrollBarView.Position = tableView.LeftItem;
11181116
// scrollBar.Refresh ();
11191117
// };
1120-
//}
1118+
// }
11211119

11221120
private void ShowAllColumns ()
11231121
{

‎Terminal.Gui/App/ApplicationImpl.Screen.cs‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
1+
using System.Diagnostics;
12
using Terminal.Gui.Tracing;
23

34
namespace Terminal.Gui.App;
45

5-
using Trace = Trace;
6-
76
internal partial class ApplicationImpl
87
{
98
/// <inheritdoc/>
@@ -102,7 +101,7 @@ private void Driver_SizeChanged (object? sender, SizeChangedEventArgs e)
102101
/// <inheritdoc/>
103102
public void LayoutAndDraw (bool forceRedraw = false)
104103
{
105-
Trace.Draw ("ApplicationImpl", "Start", $"forceRedraw={forceRedraw}, Screen={Screen}, _inlineScreenSized={_inlineScreenSized}");
104+
Tracing.Trace.Draw ("ApplicationImpl", "Start", $"forceRedraw={forceRedraw}, Screen={Screen}, _inlineScreenSized={_inlineScreenSized}");
106105

107106
if (ClearScreenNextIteration)
108107
{
@@ -284,7 +283,7 @@ public void LayoutAndDraw (bool forceRedraw = false)
284283
{
285284
LayoutAndDrawComplete?.Invoke (this, EventArgs.Empty);
286285
}
287-
Trace.Draw ("ApplicationImpl", "End", $"neededLayout={neededLayout}, needsDraw={needsDraw}");
286+
Tracing.Trace.Draw ("ApplicationImpl", "End", $"neededLayout={neededLayout}, needsDraw={needsDraw}");
288287
}
289288

290289
/// <inheritdoc />

0 commit comments

Comments
 (0)