From 87139240a38f2289d9309b552c18befff0089845 Mon Sep 17 00:00:00 2001 From: Rolf Madsen Date: Mon, 9 Oct 2023 11:05:26 +0200 Subject: [PATCH 1/2] 1282-GetSettingsLookupRaceFix: Test Added unit test to show the issue. --- Engine.UnitTests/TestStepTests.cs | 59 +++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/Engine.UnitTests/TestStepTests.cs b/Engine.UnitTests/TestStepTests.cs index e7825bc38..c3d739b47 100644 --- a/Engine.UnitTests/TestStepTests.cs +++ b/Engine.UnitTests/TestStepTests.cs @@ -1,5 +1,9 @@ +using System; +using System.Collections.Generic; +using System.Threading; using NUnit.Framework; using OpenTap.Plugins.BasicSteps; +using OpenTap.UnitTests; namespace OpenTap.Engine.UnitTests { @@ -41,5 +45,60 @@ public void FormattedNameIssue() var run = plan.Execute(); Assert.AreEqual(Verdict.NotSet, run.Verdict); } + + [Test] + public void TestGetObjectSettings() + { + // A race condition issue occured inside GetObjectSettings. + // to reproduce it, do it in two synchronized threads. + // the mix of threads and semaphores below are to ensure that the threadsa + // starts as much in sync as possible. + var steps = new ITestStep[] + { + new DelayStep(), new SequenceStep(), new LockStep(), new SequenceStep(), + new DialogStep(), new BusyStep(), new ArtifactStep(), new SerializeEnumTest.Step1(), new SerializeEnumTest.Step2(), + new MemberDataProviderTests.Delay2Step(), new ResultTest.ActionStep(), new DutStep2(), new IfStep() + }; + HashSet values = new HashSet(); + int threadCount = 2; + var threadWaitSem = new Semaphore(0, threadCount); + var mainWaitSem = new Semaphore(0, threadCount); + Exception error = null; + for (int i = 0; i < threadCount; i++) + { + TapThread.Start(() => + { + //signal the main thread that we are ready to go. + mainWaitSem.Release(); + // wait for the main thread to signal back. + threadWaitSem.WaitOne(); + try + { + TestStepExtensions.GetObjectSettings(steps, false, (t, data) => t, values); + } + catch (Exception e) + { + error = e; + } + //signal the main thread that we are done. + mainWaitSem.Release(); + }); + } + + // wait for the threads to be ready. + for(int i = 0; i < threadCount; i++) + mainWaitSem.WaitOne(); + + // signal that the threads can start. + threadWaitSem.Release(threadCount); + + // Wait for all to complete. + for(int i = 0; i < threadCount; i++) + mainWaitSem.WaitOne(); + + // the issue should cause an exception when reproduced. + // if its fixed the error will be null. + Assert.IsNull(error); + } } } \ No newline at end of file From 913d99b603c1ff36804c3d5ed3365b521f324ef6 Mon Sep 17 00:00:00 2001 From: Rolf Madsen Date: Mon, 9 Oct 2023 11:07:14 +0200 Subject: [PATCH 2/2] 1282 Get Settings Lookup Race Fix: Fix Fixed the issue by using ImmutableDictionary.SetItem instead of ImmutableDictionary.Add. The difference is that SetItem updates values of existing keys while Add throws an exception if the key exists in the dictionary. --- Shared/Cache.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Shared/Cache.cs b/Shared/Cache.cs index 2ea70b118..9608f30f2 100644 --- a/Shared/Cache.cs +++ b/Shared/Cache.cs @@ -51,7 +51,7 @@ public bool TryGetValue(K key, out V value) public V AddValue(K key, V value) { CheckClear(); - dict = dict.Add(key, value); + dict = dict.SetItem(key, value); return value; } }