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 <[email protected]>
Co-authored-by: Tig Kindel <[email protected]>

Terminal.Gui/Core/MainLoop.cs

@@ -6,6 +6,7 @@
 //
 using System;
 using System.Collections.Generic;
+using System.Collections.ObjectModel;
 
 namespace Terminal.Gui {
 	/// <summary>
@@ -61,6 +62,11 @@ namespace Terminal.Gui {
 
 		internal SortedList<long, Timeout> timeouts = new SortedList<long, Timeout> ();
 		object timeoutsLockToken = new object ();
+
+		/// <summary>
+		/// The idle handlers and lock that must be held while manipulating them
+		/// </summary>
+		object idleHandlersLock = new object ();
 		internal List<Func<bool>> idleHandlers = new List<Func<bool>> ();
 
 		/// <summary>
@@ -71,9 +77,15 @@ namespace Terminal.Gui {
 		public SortedList<long, Timeout> Timeouts => timeouts;
 
 		/// <summary>
-		/// Gets the list of all idle handlers.
+		/// Gets a copy of the list of all idle handlers.
 		/// </summary>
-		public List<Func<bool>> IdleHandlers => idleHandlers;
+		public ReadOnlyCollection<Func<bool>> IdleHandlers {
+			get {
+				lock (idleHandlersLock) {
+					return new List<Func<bool>> (idleHandlers).AsReadOnly ();
+				}
+			}
+		}
 
 		/// <summary>
 		/// The current IMainLoopDriver in use.
@@ -123,7 +135,7 @@ namespace Terminal.Gui {
 		/// <param name="idleHandler">Token that can be used to remove the idle handler with <see cref="RemoveIdle(Func{bool})"/> .</param>
 		public Func<bool> AddIdle (Func<bool> idleHandler)
 		{
-			lock (idleHandlers) {
+			lock (idleHandlersLock) {
 				idleHandlers.Add (idleHandler);
 			}
 
@@ -139,7 +151,7 @@ namespace Terminal.Gui {
 		///  This method also returns <c>false</c> if the idle handler is not found.
 		public bool RemoveIdle (Func<bool> token)
 		{
-			lock (token)
+			lock (idleHandlersLock)
 				return idleHandlers.Remove (token);
 		}
 
@@ -242,14 +254,14 @@ namespace Terminal.Gui {
 		void RunIdle ()
 		{
 			List<Func<bool>> iterate;
-			lock (idleHandlers) {
+			lock (idleHandlersLock) {
 				iterate = idleHandlers;
 				idleHandlers = new List<Func<bool>> ();
 			}
 
 			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 ();
 			}

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);
+		}
 	}
 }