From 1cdf6a1b458ea399044a9b9b46d22336bf2ab6c0 Mon Sep 17 00:00:00 2001 From: montekarlos Date: Thu, 15 Sep 2022 11:26:19 +1000 Subject: [PATCH] Fixes #1994. BREAKING CHANGE. Ensure only a single IdleHandlers list can exist. (#1997) * Add unit test that demonstrates the loss of idle handlers added via Application.MainLoop.Invoke * Fixes #1994. Ensure that only a single idle handlers list exists at one time * Previous implementation locked on idleHandlers but this is dangerous because it is reallocated. It was therefore possible for two different threads to hold locks on two different instances of idleHandlers simultaneously. The idle handlers added to the older instance of idleHandlers would then be lost. This was particularity catastrophic when combined with async/await continuations being lost. * Add dedicated lock object idleHandlersLock and use when modifying idleHandlers * Fix additional bug in RemoveIdle that was locking the token instead of idleHandlers * Return a copy via the IdleHandlers property as it was directly returning idleHandlers. This cannot safely be done without first acquiring idleHandlersLock * Address code review feedback for #1994: Make IdleHandler immutable via ReadOnlyCollection * Address code review feedback for #1994: Avoid the possibility of IdleHandlers changing while being used Co-authored-by: Karl Janke Co-authored-by: Tig Kindel --- Terminal.Gui/Core/MainLoop.cs | 26 +++++++++++----- UnitTests/MainLoopTests.cs | 58 +++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 7 deletions(-) diff --git a/Terminal.Gui/Core/MainLoop.cs b/Terminal.Gui/Core/MainLoop.cs index 4988a5a8e..43789e228 100644 --- a/Terminal.Gui/Core/MainLoop.cs +++ b/Terminal.Gui/Core/MainLoop.cs @@ -6,6 +6,7 @@ // using System; using System.Collections.Generic; +using System.Collections.ObjectModel; namespace Terminal.Gui { /// @@ -61,6 +62,11 @@ namespace Terminal.Gui { internal SortedList timeouts = new SortedList (); object timeoutsLockToken = new object (); + + /// + /// The idle handlers and lock that must be held while manipulating them + /// + object idleHandlersLock = new object (); internal List> idleHandlers = new List> (); /// @@ -71,9 +77,15 @@ namespace Terminal.Gui { public SortedList Timeouts => timeouts; /// - /// Gets the list of all idle handlers. + /// Gets a copy of the list of all idle handlers. /// - public List> IdleHandlers => idleHandlers; + public ReadOnlyCollection> IdleHandlers { + get { + lock (idleHandlersLock) { + return new List> (idleHandlers).AsReadOnly (); + } + } + } /// /// The current IMainLoopDriver in use. @@ -123,7 +135,7 @@ namespace Terminal.Gui { /// Token that can be used to remove the idle handler with . public Func AddIdle (Func idleHandler) { - lock (idleHandlers) { + lock (idleHandlersLock) { idleHandlers.Add (idleHandler); } @@ -139,7 +151,7 @@ namespace Terminal.Gui { /// This method also returns false if the idle handler is not found. public bool RemoveIdle (Func token) { - lock (token) + lock (idleHandlersLock) return idleHandlers.Remove (token); } @@ -242,14 +254,14 @@ namespace Terminal.Gui { void RunIdle () { List> iterate; - lock (idleHandlers) { + lock (idleHandlersLock) { iterate = idleHandlers; idleHandlers = new List> (); } foreach (var idle in iterate) { if (idle ()) - lock (idleHandlers) + lock (idleHandlersLock) idleHandlers.Add (idle); } } @@ -294,7 +306,7 @@ namespace Terminal.Gui { Driver.MainIteration (); - lock (idleHandlers) { + lock (idleHandlersLock) { if (idleHandlers.Count > 0) RunIdle (); } diff --git a/UnitTests/MainLoopTests.cs b/UnitTests/MainLoopTests.cs index 69cff3e4b..1299c2b55 100644 --- a/UnitTests/MainLoopTests.cs +++ b/UnitTests/MainLoopTests.cs @@ -525,5 +525,63 @@ namespace Terminal.Gui.Core { // - wait = false // TODO: Add IMainLoop tests + + volatile static int tbCounter = 0; + + private static void Launch (Random r, TextField tf) + { + Task.Run (() => { + Thread.Sleep (r.Next (2, 4)); + Application.MainLoop.Invoke (() => { + tf.Text = $"index{r.Next ()}"; + Interlocked.Increment (ref tbCounter); + }); + }); + } + + private static void RunTest (Random r, TextField tf, int numPasses, int numIncrements, int pollMs) + { + for (int j = 0; j < numPasses; j++) { + for (var i = 0; i < numIncrements; i++) { + Launch (r, tf); + } + + while (tbCounter != (j + 1) * numIncrements) // Wait for tbCounter to reach expected value + { + var tbNow = tbCounter; + Thread.Sleep (pollMs); + if (tbCounter == tbNow) { + // No change after sleep: Idle handlers added via Application.MainLoop.Invoke have gone missing + Application.MainLoop.Invoke (() => Application.RequestStop ()); + throw new TimeoutException ( + $"Timeout: Increment lost. tbCounter ({tbCounter}) didn't " + + $"change after waiting {pollMs} ms. Failed to reach {(j + 1) * numIncrements} on pass {j + 1}"); + } + }; + } + Application.MainLoop.Invoke (() => Application.RequestStop ()); + } + + [Fact] + [AutoInitShutdown] + public async Task InvokeLeakTest () + { + Random r = new (); + TextField tf = new (); + Application.Top.Add (tf); + + const int numPasses = 10; + const int numIncrements = 10000; + const int pollMs = 500; + + var task = Task.Run (() => RunTest (r, tf, numPasses, numIncrements, pollMs)); + + // blocks here until the RequestStop is processed at the end of the test + Application.Run (); + + await task; // Propagate exception if any occurred + + Assert.Equal ((numIncrements * numPasses), tbCounter); + } } }