From c2d0a28c86560b30ee88f0b78f2a33e364672c85 Mon Sep 17 00:00:00 2001 From: BDisp Date: Mon, 17 May 2021 01:01:03 +0100 Subject: [PATCH] Fixes #1307 MainLoop timeouts duplicate keys error. (#1308) * Fixes #1307 MainLoop timeouts duplicate keys error. * Fixes key not exist in collection on parallel. --- Terminal.Gui/Core/MainLoop.cs | 30 +++++--- UnitTests/MainLoopTests.cs | 129 +++++++++++++++++++++++++--------- 2 files changed, 118 insertions(+), 41 deletions(-) diff --git a/Terminal.Gui/Core/MainLoop.cs b/Terminal.Gui/Core/MainLoop.cs index 22bfa372a..c6bdbbd4f 100644 --- a/Terminal.Gui/Core/MainLoop.cs +++ b/Terminal.Gui/Core/MainLoop.cs @@ -31,7 +31,7 @@ namespace Terminal.Gui { bool EventsPending (bool wait); /// - /// The interation function. + /// The iteration function. /// void MainIteration (); } @@ -107,22 +107,30 @@ namespace Terminal.Gui { /// Removes an idle handler added with from processing. /// /// A token returned by - public void RemoveIdle (Func token) + /// Returns trueif the idle handler is successfully removed; otherwise, false. + /// This method also returns false if the idle handler is not found. + public bool RemoveIdle (Func token) { lock (token) - idleHandlers.Remove (token); + return idleHandlers.Remove (token); } void AddTimeout (TimeSpan time, Timeout timeout) { - timeouts.Add ((DateTime.UtcNow + time).Ticks, timeout); + var k = (DateTime.UtcNow + time).Ticks; + lock (timeouts) { + while (timeouts.ContainsKey (k)) { + k = (DateTime.UtcNow + time).Ticks; + } + } + timeouts.Add (k, timeout); } /// /// Adds a timeout to the mainloop. /// /// - /// When time time specified passes, the callback will be invoked. + /// When time specified passes, the callback will be invoked. /// If the callback returns true, the timeout will be reset, repeating /// the invocation. If it returns false, the timeout will stop and be removed. /// @@ -147,12 +155,15 @@ namespace Terminal.Gui { /// /// The token parameter is the value returned by AddTimeout. /// - public void RemoveTimeout (object token) + /// Returns trueif the timeout is successfully removed; otherwise, false. + /// This method also returns false if the timeout is not found. + public bool RemoveTimeout (object token) { var idx = timeouts.IndexOfValue (token as Timeout); if (idx == -1) - return; + return false; timeouts.RemoveAt (idx); + return true; } void RunTimers () @@ -160,8 +171,9 @@ namespace Terminal.Gui { long now = DateTime.UtcNow.Ticks; var copy = timeouts; timeouts = new SortedList (); - foreach (var k in copy.Keys) { - var timeout = copy [k]; + foreach (var t in copy) { + var k = t.Key; + var timeout = t.Value; if (k < now) { if (timeout.Callback (this)) AddTimeout (timeout.Span, timeout); diff --git a/UnitTests/MainLoopTests.cs b/UnitTests/MainLoopTests.cs index c4b69b14a..c85409021 100644 --- a/UnitTests/MainLoopTests.cs +++ b/UnitTests/MainLoopTests.cs @@ -5,6 +5,7 @@ using System.Globalization; using System.Linq; using System.Runtime.InteropServices.ComTypes; using System.Threading; +using System.Threading.Tasks; using Terminal.Gui; using Xunit; using Xunit.Sdk; @@ -33,28 +34,31 @@ namespace Terminal.Gui.Core { ml.AddIdle (fnTrue); ml.AddIdle (fnFalse); - ml.RemoveIdle (fnTrue); + Assert.True (ml.RemoveIdle (fnTrue)); - // BUGBUG: This doens't throw or indicate an error. Ideally RemoveIdle would either - // trhow an exception in this case, or return an error. - ml.RemoveIdle (fnTrue); + // BUGBUG: This doesn't throw or indicate an error. Ideally RemoveIdle would either + // throw an exception in this case, or return an error. + // No. Only need to return a boolean. + Assert.False (ml.RemoveIdle (fnTrue)); - ml.RemoveIdle (fnFalse); + Assert.True (ml.RemoveIdle (fnFalse)); // BUGBUG: This doesn't throw an exception or indicate an error. Ideally RemoveIdle would either - // trhow an exception in this case, or return an error. - ml.RemoveIdle (fnFalse); + // throw an exception in this case, or return an error. + // No. Only need to return a boolean. + Assert.False (ml.RemoveIdle (fnFalse)); // Add again, but with dupe ml.AddIdle (fnTrue); ml.AddIdle (fnTrue); - ml.RemoveIdle (fnTrue); - ml.RemoveIdle (fnTrue); + Assert.True (ml.RemoveIdle (fnTrue)); + Assert.True (ml.RemoveIdle (fnTrue)); // BUGBUG: This doesn't throw an exception or indicate an error. Ideally RemoveIdle would either - // trhow an exception in this case, or return an error. - ml.RemoveIdle (fnTrue); + // throw an exception in this case, or return an error. + // No. Only need to return a boolean. + Assert.False (ml.RemoveIdle (fnTrue)); } [Fact] @@ -84,8 +88,7 @@ namespace Terminal.Gui.Core { return true; }; - functionCalled = 0; - ml.RemoveIdle (fn); + Assert.False (ml.RemoveIdle (fn)); ml.MainIteration (); Assert.Equal (0, functionCalled); } @@ -101,9 +104,8 @@ namespace Terminal.Gui.Core { return true; }; - functionCalled = 0; ml.AddIdle (fn); - ml.RemoveIdle (fn); + Assert.True (ml.RemoveIdle (fn)); ml.MainIteration (); Assert.Equal (0, functionCalled); } @@ -119,21 +121,21 @@ namespace Terminal.Gui.Core { return true; }; - functionCalled = 0; ml.AddIdle (fn); ml.AddIdle (fn); ml.MainIteration (); Assert.Equal (2, functionCalled); functionCalled = 0; - ml.RemoveIdle (fn); + Assert.True (ml.RemoveIdle (fn)); ml.MainIteration (); Assert.Equal (1, functionCalled); functionCalled = 0; - ml.RemoveIdle (fn); + Assert.True (ml.RemoveIdle (fn)); ml.MainIteration (); Assert.Equal (0, functionCalled); + Assert.False (ml.RemoveIdle (fn)); } [Fact] @@ -163,10 +165,11 @@ namespace Terminal.Gui.Core { ml.AddIdle (fnStop); ml.AddIdle (fn1); ml.Run (); - ml.RemoveIdle (fnStop); - ml.RemoveIdle (fn1); + Assert.True (ml.RemoveIdle (fnStop)); + Assert.False (ml.RemoveIdle (fn1)); Assert.Equal (10, functionCalled); + Assert.Equal (20, stopCount); } [Fact] @@ -194,9 +197,9 @@ namespace Terminal.Gui.Core { ml.AddIdle (fn1); ml.AddIdle (fn1); ml.Run (); - ml.RemoveIdle (fnStop); - ml.RemoveIdle (fn1); - ml.RemoveIdle (fn1); + Assert.True (ml.RemoveIdle (fnStop)); + Assert.False (ml.RemoveIdle (fn1)); + Assert.False (ml.RemoveIdle (fn1)); Assert.Equal (2, functionCalled); } @@ -217,7 +220,7 @@ namespace Terminal.Gui.Core { ml.AddIdle (fn); ml.Run (); - ml.RemoveIdle (fn); + Assert.True (ml.RemoveIdle (fn)); Assert.Equal (10, functionCalled); } @@ -237,10 +240,11 @@ namespace Terminal.Gui.Core { var token = ml.AddTimeout (TimeSpan.FromMilliseconds (ms), callback); - ml.RemoveTimeout (token); + Assert.True (ml.RemoveTimeout (token)); // BUGBUG: This should probably fault? - ml.RemoveTimeout (token); + // Must return a boolean. + Assert.False (ml.RemoveTimeout (token)); } [Fact] @@ -258,11 +262,72 @@ namespace Terminal.Gui.Core { var token = ml.AddTimeout (TimeSpan.FromMilliseconds (ms), callback); ml.Run (); - ml.RemoveTimeout (token); + Assert.True (ml.RemoveTimeout (token)); Assert.Equal (1, callbackCount); } + [Fact] + public async Task AddTimer_Duplicate_Keys_Not_Allowed () + { + var ml = new MainLoop (new FakeMainLoop (() => FakeConsole.ReadKey (true))); + const int ms = 100; + object token1 = null, token2 = null; + + var callbackCount = 0; + Func callback = (MainLoop loop) => { + callbackCount++; + if (callbackCount == 2) { + ml.Stop (); + } + return true; + }; + + var task1 = new Task (() => token1 = ml.AddTimeout (TimeSpan.FromMilliseconds (ms), callback)); + var task2 = new Task (() => token2 = ml.AddTimeout (TimeSpan.FromMilliseconds (ms), callback)); + Assert.Null (token1); + Assert.Null (token2); + task1.Start (); + task2.Start (); + ml.Run (); + Assert.NotNull (token1); + Assert.NotNull (token2); + await Task.WhenAll (task1, task2); + Assert.True (ml.RemoveTimeout (token1)); + Assert.True (ml.RemoveTimeout (token2)); + + Assert.Equal (2, callbackCount); + } + + [Fact] + public void AddTimer_In_Parallel_Wont_Throw () + { + var ml = new MainLoop (new FakeMainLoop (() => FakeConsole.ReadKey (true))); + const int ms = 100; + object token1 = null, token2 = null; + + var callbackCount = 0; + Func callback = (MainLoop loop) => { + callbackCount++; + if (callbackCount == 2) { + ml.Stop (); + } + return true; + }; + + Parallel.Invoke ( + () => token1 = ml.AddTimeout (TimeSpan.FromMilliseconds (ms), callback), + () => token2 = ml.AddTimeout (TimeSpan.FromMilliseconds (ms), callback) + ); + ml.Run (); + Assert.NotNull (token1); + Assert.NotNull (token2); + Assert.True (ml.RemoveTimeout (token1)); + Assert.True (ml.RemoveTimeout (token2)); + + Assert.Equal (2, callbackCount); + } + class MillisecondTolerance : IEqualityComparer { int _tolerance = 0; @@ -293,7 +358,7 @@ namespace Terminal.Gui.Core { // https://github.com/xunit/assert.xunit/pull/25 Assert.Equal (ms * callbackCount, watch.Elapsed, new MillisecondTolerance (100)); - ml.RemoveTimeout (token); + Assert.True (ml.RemoveTimeout (token)); Assert.Equal (1, callbackCount); } @@ -321,7 +386,7 @@ namespace Terminal.Gui.Core { // https://github.com/xunit/assert.xunit/pull/25 Assert.Equal (ms * callbackCount, watch.Elapsed, new MillisecondTolerance (100)); - ml.RemoveTimeout (token); + Assert.True (ml.RemoveTimeout (token)); Assert.Equal (2, callbackCount); } @@ -349,7 +414,7 @@ namespace Terminal.Gui.Core { }; var token = ml.AddTimeout (ms, callback); - ml.RemoveTimeout (token); + Assert.True (ml.RemoveTimeout (token)); ml.Run (); Assert.Equal (0, callbackCount); } @@ -363,7 +428,7 @@ namespace Terminal.Gui.Core { // Force stop if 10 iterations var stopCount = 0; Func fnStop = () => { - Thread.Sleep (10); // Sleep to enable timeer to fire + Thread.Sleep (10); // Sleep to enable timer to fire stopCount++; if (stopCount == 10) { ml.Stop (); @@ -382,7 +447,7 @@ namespace Terminal.Gui.Core { ml.Run (); Assert.Equal (1, callbackCount); Assert.Equal (10, stopCount); - ml.RemoveTimeout (token); + Assert.False (ml.RemoveTimeout (token)); } // Invoke Tests