Merge pull request #1684 from tznind/add-timeout-thread-safety

Add timeout thread safety

Terminal.Gui/Core/MainLoop.cs

@@ -51,6 +51,7 @@ namespace Terminal.Gui {
 		}
 
 		internal SortedList<long, Timeout> timeouts = new SortedList<long, Timeout> ();
+		object timeoutsLockToken = new object ();
 		internal List<Func<bool>> idleHandlers = new List<Func<bool>> ();
 
 		/// <summary>
@@ -117,7 +118,7 @@ namespace Terminal.Gui {
 
 		void AddTimeout (TimeSpan time, Timeout timeout)
 		{
-			lock (timeouts) {
+			lock (timeoutsLockToken) {
 				var k = (DateTime.UtcNow + time).Ticks;
 				while (timeouts.ContainsKey (k)) {
 					k = (DateTime.UtcNow + time).Ticks;
@@ -159,7 +160,7 @@ namespace Terminal.Gui {
 		/// This method also returns <c>false</c> if the timeout is not found.
 		public bool RemoveTimeout (object token)
 		{
-			lock (timeouts) {
+			lock (timeoutsLockToken) {
 				var idx = timeouts.IndexOfValue (token as Timeout);
 				if (idx == -1)
 					return false;
@@ -171,8 +172,17 @@ namespace Terminal.Gui {
 		void RunTimers ()
 		{
 			long now = DateTime.UtcNow.Ticks;
-			var copy = timeouts;
-			timeouts = new SortedList<long, Timeout> ();
+			SortedList<long, Timeout> copy;
+
+			// lock prevents new timeouts being added
+			// after we have taken the copy but before
+			// we have allocated a new list (which would
+			// result in lost timeouts or errors during enumeration)
+			lock (timeoutsLockToken) {
+				copy = timeouts;
+				timeouts = new SortedList<long, Timeout> ();
+			}
+
 			foreach (var t in copy) {
 				var k = t.Key;
 				var timeout = t.Value;
@@ -180,11 +190,12 @@ namespace Terminal.Gui {
 					if (timeout.Callback (this))
 						AddTimeout (timeout.Span, timeout);
 				} else {
-					lock (timeouts) {
+					lock (timeoutsLockToken) {
 						timeouts.Add (k, timeout);
 					}
 				}
 			}
+			
 		}
 
 		void RunIdle ()

UnitTests/ApplicationTests.cs

@@ -1286,5 +1286,54 @@ namespace Terminal.Gui.Core {
 			var cultures = Application.GetSupportedCultures ();
 			Assert.Equal (cultures.Count, Application.SupportedCultures.Count);
 		}
+
+		[Fact, AutoInitShutdown]
+		public void TestAddManyTimeouts ()
+		{
+			int delegatesRun = 0;
+			int numberOfThreads = 100;
+			int numberOfTimeoutsPerThread = 100;
+
+
+			// start lots of threads
+			for (int i = 0; i < numberOfThreads; i++) {
+				
+				var myi = i;
+
+				Task.Run (() => {
+					Task.Delay (100).Wait ();
+
+					// each thread registers lots of 1s timeouts
+					for(int j=0;j< numberOfTimeoutsPerThread; j++) {
+
+						Application.MainLoop.AddTimeout (TimeSpan.FromSeconds(1), (s) => {
+
+							// each timeout delegate increments delegatesRun count by 1 every second
+							Interlocked.Increment (ref delegatesRun);
+							return true; 
+						});
+					}
+					 
+					// if this is the first Thread created
+					if (myi == 0) {
+
+						// let the timeouts run for a bit
+						Task.Delay (5000).Wait ();
+
+						// then tell the application to quuit
+						Application.MainLoop.Invoke (() => Application.RequestStop ());
+					}
+				});
+			}
+
+			// blocks here until the RequestStop is processed at the end of the test
+			Application.Run ();
+
+			// undershoot a bit to be on the safe side.  The 5000 ms wait allows the timeouts to run
+			// a lot but all those timeout delegates could end up going slowly on a slow machine perhaps
+			// so the final number of delegatesRun might vary by computer.  So for this assert we say
+			// that it should have run at least 2 seconds worth of delegates
+			Assert.True (delegatesRun >= numberOfThreads * numberOfTimeoutsPerThread * 2);
+		}
 	}
 }