Fix threading issues

Reduce the number of transactions being created by propagating them.
Created a transaction when the engine is initialized, so that the initial
loading of add-in roots is all done using a single transaction.
Fixed unit tests.

5 files changed, 169 insertions, 74 deletions

diff --git a/Mono.Addins/Mono.Addins/AddinEngine.cs b/Mono.Addins/Mono.Addins/AddinEngine.cs
index 69a4112..507d973 100644
--- a/Mono.Addins/Mono.Addins/AddinEngine.cs
+++ b/Mono.Addins/Mono.Addins/AddinEngine.cs
@@ -284,12 +284,14 @@ namespace Mono.Addins
 				else
 					registry = new AddinRegistry (this, configDir, startupDirectory, addinsDir, databaseDir, null);
 
+				using var transaction = BeginTransaction();
+
 				if ((asmFile != null && registry.CreateHostAddinsFile (asmFile)) || registry.UnknownDomain)
-					registry.Update (new ConsoleProgressStatus (false));
+					registry.Update (new ConsoleProgressStatus (false), transaction);
 				
 				initialized = true;
 				
-				ActivateRoots ();
+				ActivateRoots (transaction);
 				OnAssemblyLoaded (null, null);
 				AppDomain.CurrentDomain.AssemblyLoad += new AssemblyLoadEventHandler (OnAssemblyLoaded);
 				AppDomain.CurrentDomain.AssemblyResolve += CurrentDomainAssemblyResolve;
@@ -515,7 +517,14 @@ namespace Mono.Addins
 		{
 			ValidateAddinRoots();
 			RuntimeAddin a;
-			transaction.AddinEngineSnapshot.LoadedAddins.TryGetValue(Addin.GetIdName(id), out a);
+
+			// If a transaction is provided, try using the snapshot from that transaction,
+			// but this is only possible if the transaction was created for this context
+
+			if (transaction.Context == this)
+				transaction.AddinEngineSnapshot.LoadedAddins.TryGetValue(Addin.GetIdName(id), out a);
+			else
+				currentSnapshot.LoadedAddins.TryGetValue(Addin.GetIdName(id), out a);
 			return a;
 		}
 
@@ -601,24 +610,20 @@ namespace Mono.Addins
 					statusMonitor.SetMessage ("Loading Addins");
 
 				if (addins.Count > 0) {
-					try {
-						for (int n = 0; n < addins.Count; n++) {
+					for (int n = 0; n < addins.Count; n++) {
 
-							if (statusMonitor != null)
-								statusMonitor.SetProgress ((double)n / (double)addins.Count);
+						if (statusMonitor != null)
+							statusMonitor.SetProgress ((double)n / (double)addins.Count);
 
-							Addin iad = addins [n];
-							if (IsAddinLoaded (transaction.AddinEngineSnapshot, iad.Id))
-								continue;
+						Addin iad = addins [n];
+						if (IsAddinLoaded (transaction.AddinEngineSnapshot, iad.Id))
+							continue;
 
-							if (statusMonitor != null)
-								statusMonitor.SetMessage (string.Format (GettextCatalog.GetString ("Loading {0} add-in"), iad.Id));
+						if (statusMonitor != null)
+							statusMonitor.SetMessage (string.Format (GettextCatalog.GetString ("Loading {0} add-in"), iad.Id));
 
-							if (!InsertAddin (transaction, statusMonitor, iad))
-								return false;
-						}
-					} finally {
-						transaction?.Dispose ();
+						if (!InsertAddin (transaction, statusMonitor, iad))
+							return false;
 					}
 				}
 				return true;
@@ -633,11 +638,11 @@ namespace Mono.Addins
 			}
 		}
 
-		internal override void ResetCachedData (ExtensionContextTransaction transaction)
+		internal override void OnResetCachedData (ExtensionContextTransaction transaction)
 		{
 			foreach (RuntimeAddin ad in transaction.AddinEngineSnapshot.LoadedAddins.Values)
 				ad.Addin.ResetCachedData ();
-			base.ResetCachedData (transaction);
+			base.OnResetCachedData (transaction);
 		}
 			
 		bool InsertAddin (ExtensionContextTransaction transaction, IProgressStatus statusMonitor, Addin iad)
@@ -684,8 +689,8 @@ namespace Mono.Addins
 		}
 
 		internal void InsertExtensionPoint (ExtensionContextTransaction transaction, RuntimeAddin addin, ExtensionPoint ep)
-		{
-			CreateExtensionPoint (ep);
+        {
+			CreateExtensionPoint (transaction, ep);
 			foreach (ExtensionNodeType nt in ep.NodeSet.NodeTypes) {
 				if (nt.ObjectTypeName.Length > 0) {
 					RegisterAutoTypeExtensionPoint (transaction, nt.ObjectTypeName, ep.Path);
@@ -843,14 +848,13 @@ namespace Mono.Addins
 			}
 		}
 
-		internal void ActivateRoots ()
+		internal void ActivateRoots (ExtensionContextTransaction transaction)
 		{
 			lock (pendingRootChecks)
 				pendingRootChecks.Clear ();
 
-			using var tr = BeginTransaction();
 			foreach (Assembly asm in AppDomain.CurrentDomain.GetAssemblies ())
-				CheckHostAssembly (tr, asm);
+				CheckHostAssembly (transaction, asm);
 		}
 		
 		void CheckHostAssembly (ExtensionContextTransaction transaction, Assembly asm)
diff --git a/Mono.Addins/Mono.Addins/AddinManager.cs b/Mono.Addins/Mono.Addins/AddinManager.cs
index 823bccc..663e8fa 100644
--- a/Mono.Addins/Mono.Addins/AddinManager.cs
+++ b/Mono.Addins/Mono.Addins/AddinManager.cs
@@ -155,7 +155,8 @@ namespace Mono.Addins
 		/// </summary>
 		public static void Shutdown ()
 		{
-			AddinEngine.Shutdown ();
+			sessionService?.Shutdown ();
+			sessionService = null;
 		}
 		
 		/// <summary>
diff --git a/Mono.Addins/Mono.Addins/ExtensionContext.cs b/Mono.Addins/Mono.Addins/ExtensionContext.cs
index 722b23f..bb9c478 100644
--- a/Mono.Addins/Mono.Addins/ExtensionContext.cs
+++ b/Mono.Addins/Mono.Addins/ExtensionContext.cs
@@ -166,13 +166,21 @@ namespace Mono.Addins
 			}
 		}
 
-		internal void ResetCachedData()
+		internal void ResetCachedData(ExtensionContextTransaction transaction = null)
 		{
-			using (var transaction = BeginTransaction())
-				ResetCachedData(transaction);
+			var currentTransaction = transaction ?? BeginTransaction();
+			try
+			{
+				OnResetCachedData(currentTransaction);
+			}
+			finally
+			{
+				if (currentTransaction != transaction)
+					currentTransaction.Dispose();
+			}
 		}
 
-		internal virtual void ResetCachedData (ExtensionContextTransaction transaction)
+		internal virtual void OnResetCachedData (ExtensionContextTransaction transaction)
 		{
 			tree.ResetCachedData (transaction);
 
@@ -870,9 +878,9 @@ namespace Mono.Addins
 				ctx.NotifyAddinLoaded (ad);
 		}
 		
-		internal void CreateExtensionPoint (ExtensionPoint ep)
+		internal void CreateExtensionPoint (ExtensionContextTransaction transaction, ExtensionPoint ep)
 		{
-			TreeNode node = tree.GetNode (ep.Path, true);
+			TreeNode node = tree.GetNode (ep.Path, true, transaction);
 			if (node.ExtensionPoint == null) {
 				node.ExtensionPoint = ep;
 				node.ExtensionNodeSet = ep.NodeSet;
diff --git a/Mono.Addins/Mono.Addins/TreeNode.cs b/Mono.Addins/Mono.Addins/TreeNode.cs
index 533e092..0c0d992 100644
--- a/Mono.Addins/Mono.Addins/TreeNode.cs
+++ b/Mono.Addins/Mono.Addins/TreeNode.cs
@@ -268,8 +268,13 @@ namespace Mono.Addins
 		{
 			return GetNode (path, false);
 		}
-		
-		public TreeNode GetNode (string path, bool buildPath)
+
+		public TreeNode GetNode(string path, bool buildPath)
+		{
+			return GetNode(path, buildPath, null);
+		}
+
+		public TreeNode GetNode (string path, bool buildPath, ExtensionContextTransaction transaction)
 		{
 			if (path.StartsWith ("/"))
 				path = path.Substring (1);
@@ -278,6 +283,7 @@ namespace Mono.Addins
 			TreeNode curNode = this;
 
 			foreach (string part in parts) {
+				curNode.EnsureChildrenLoaded(transaction);
 				var node = curNode.GetChildNode (part);
 				if (node != null) {
 					curNode = node;
@@ -285,9 +291,10 @@ namespace Mono.Addins
 				}
 				
 				if (buildPath) {
-					var transaction = BeginContextTransaction (out var dispose);
+					transaction = BeginContextTransaction (transaction, out var dispose);
 					try {
 						// Check again inside the lock, just in case
+						curNode.EnsureChildrenLoaded(transaction);
 						node = curNode.GetChildNode (part);
 						if (node != null) {
 							curNode = node;
@@ -332,24 +339,37 @@ namespace Mono.Addins
 				if (IsInChildrenUpdateTransaction) {
 					return childrenBuilder;
 				}
+				EnsureChildrenLoaded(null);
+				return (IReadOnlyList<TreeNode>)childrenBuilder ?? (IReadOnlyList<TreeNode>)children;
+			}
+		}
 
-				if (!childrenFromExtensionsLoaded) {
-					var transaction = BeginContextTransaction (out var disposeTransaction);
-					try {
-						if (!childrenFromExtensionsLoaded) {
-							if (extensionPoint != null) {
-								BeginChildrenUpdateTransaction (transaction);
-								Context.LoadExtensions (transaction, GetPath (), this);
-								// We have to keep the reference to the extension point, since add-ins may be loaded/unloaded
-							}
+		void EnsureChildrenLoaded(ExtensionContextTransaction transaction)
+		{
+			if (IsInChildrenUpdateTransaction)
+				return;
+
+			if (!childrenFromExtensionsLoaded)
+			{
+				transaction = BeginContextTransaction(transaction, out var disposeTransaction);
+				try
+				{
+					if (!childrenFromExtensionsLoaded)
+					{
+						if (extensionPoint != null)
+						{
+							BeginChildrenUpdateTransaction(transaction);
+							Context.LoadExtensions(transaction, GetPath(), this);
+							// We have to keep the reference to the extension point, since add-ins may be loaded/unloaded
 						}
-					} finally {
-						childrenFromExtensionsLoaded = true;
-						if (disposeTransaction)
-							transaction.Dispose ();
 					}
 				}
-				return (IReadOnlyList<TreeNode>)childrenBuilder ?? (IReadOnlyList<TreeNode>)children;
+				finally
+				{
+					childrenFromExtensionsLoaded = true;
+					if (disposeTransaction)
+						transaction.Dispose();
+				}
 			}
 		}
 
@@ -367,9 +387,14 @@ namespace Mono.Addins
 		/// </summary>
 		bool IsInChildrenUpdateTransaction => childrenBuilder != null && Context.IsCurrentThreadInTransaction;
 
-		ExtensionContextTransaction BeginContextTransaction (out bool dispose)
+		ExtensionContextTransaction BeginContextTransaction (ExtensionContextTransaction currentTransaction, out bool dispose)
 		{
-			if (IsInChildrenUpdateTransaction) {
+			if (currentTransaction != null)
+			{
+				dispose = false;
+				return currentTransaction;
+			}
+			else if (IsInChildrenUpdateTransaction) {
 				dispose = false;
 				return buildTransaction;
 			} else {
diff --git a/Test/UnitTests/TestMultithreading.cs b/Test/UnitTests/TestMultithreading.cs
index fd3cbcf..f827788 100644
--- a/Test/UnitTests/TestMultithreading.cs
+++ b/Test/UnitTests/TestMultithreading.cs
@@ -27,6 +27,9 @@ namespace UnitTests
 
 			GlobalInfoCondition.Value = "";
 
+			// Threads check the number of nodes in /SimpleApp/ConditionedWriters, which changes
+			// from 0 to 1 as the GlobalInfoCondition condition changes
+
 			testData.StartThreads (RunChildConditionAttributeTest);
 
 			for (int n = 0; n < 20; n++) {
@@ -84,41 +87,94 @@ namespace UnitTests
 			}
 		}
 
+		int EnableDisableStress_totalAdd;
+		int EnableDisableStress_totalRemove;
+		int EnableDisableStress_nodesCount;
+		int EnableDisableStress_minCount;
+		int EnableDisableStress_maxCount;
+
 		[Test]
-		public void EventsMultithread ()
+		public void EventsMultithread()
 		{
 			int threads = 50;
-			int steps = 20;
 
-			var testData = new TestData (threads);
+			int addinsLoaded = 0;
+			int addinsUnloaded = 0;
 
-			GlobalInfoCondition.Value = "";
+			var node = AddinManager.GetExtensionNode("/SimpleApp/Writers");
 
-			testData.StartThreads (RunEventsMultithread);
+			try
+			{
+				EnableDisableStress_nodesCount = 0;
+
+				node.ExtensionNodeChanged += Node_ExtensionNodeChanged;
+
+				Assert.AreEqual(4, EnableDisableStress_nodesCount);
+
+				EnableDisableStress_minCount = 4;
+				EnableDisableStress_maxCount = 4;
+				EnableDisableStress_totalAdd = 0;
+				EnableDisableStress_totalRemove = 0;
+
+				var ainfo1 = AddinManager.Registry.GetAddin("SimpleApp.HelloWorldExtension");
+				var ainfo2 = AddinManager.Registry.GetAddin("SimpleApp.FileContentExtension");
+
+				AddinManager.AddinLoaded += (s,a) => addinsLoaded++;
+				AddinManager.AddinUnloaded += (s,a) => addinsUnloaded++;
+
+				using var enablers = new TestData(threads);
+
+				enablers.StartThreads((index, data) =>
+				{
+					var random = new Random(10000 + index);
+					while (!data.Stopped)
+					{
+						var action = random.Next(4);
+						switch (action)
+						{
+							case 0: ainfo1.Enabled = false; break;
+							case 1: ainfo1.Enabled = true; break;
+							case 2: ainfo2.Enabled = false; break;
+							case 3: ainfo2.Enabled = true; break;
+						}
+					}
+				});
+				Thread.Sleep(3000);
+			}
+			finally
+			{
+				node.ExtensionNodeChanged -= Node_ExtensionNodeChanged;
+			}
 
-			for (int n = 0; n < steps; n++) {
-				Console.WriteLine ("Step " + n);
-				testData.CheckCounters (0, 10000);
+			// If all events have been sent correctly, the node count should have never gone below 2 and over 4.
 
-				GlobalInfoCondition.Value = "foo";
+			Assert.That(EnableDisableStress_minCount, Is.AtLeast(2));
+			Assert.That(EnableDisableStress_maxCount, Is.AtMost(4));
 
-				testData.CheckCounters (1, 1000);
+			// Every time one of these add-ins is enabled, a new node is added (likewise when removed), so
+			// the total count of nodes added must match the number of times the addins were enabled.
 
-				GlobalInfoCondition.Value = "";
-			}
+			Assert.AreEqual(EnableDisableStress_totalAdd, addinsLoaded);
+			Assert.AreEqual(EnableDisableStress_totalRemove, addinsUnloaded);
 		}
 
-		void RunEventsMultithread (int index, TestData data)
-		{
-			while (!data.Stopped) {
-				int count = 0;
-				ExtensionNodeEventHandler handler = (s, args) => {
-					count++;
-				};
-				AddinManager.AddExtensionNodeHandler("/SimpleApp/ConditionedWriters", handler);
-				data.Counters [index] = count;
-				AddinManager.RemoveExtensionNodeHandler ("/SimpleApp/ConditionedWriters", handler);
+        private void Node_ExtensionNodeChanged (object sender, ExtensionNodeEventArgs args)
+        {
+			if (args.Change == ExtensionChange.Add)
+			{
+				EnableDisableStress_nodesCount++;
+				EnableDisableStress_totalAdd++;
+			}
+			else
+			{
+				EnableDisableStress_nodesCount--;
+				EnableDisableStress_totalRemove++;
 			}
+
+			if (EnableDisableStress_nodesCount < EnableDisableStress_minCount)
+				EnableDisableStress_minCount = EnableDisableStress_nodesCount;
+			if (EnableDisableStress_nodesCount > EnableDisableStress_maxCount)
+				EnableDisableStress_maxCount = EnableDisableStress_nodesCount;
 		}
 
 		[Test]
@@ -148,9 +204,10 @@ namespace UnitTests
 			var ctx = AddinManager.CreateExtensionContext ();
 			var writers = ctx.GetExtensionNode ("/SimpleApp/ConditionedWriters");
 			while (!data.Stopped) {
-				data.Counters [index] = writers.ChildNodes.Count;
+				data.Counters [index] = writers.GetChildNodes().Count;
 			}
 		}
+
 		class TestData: IDisposable
 		{
 			int numThreads;
@@ -173,7 +230,7 @@ namespace UnitTests
 					Thread.Sleep (10);
 				} while (sw.ElapsedMilliseconds < timeout);
 
-				Assert.Fail ();
+				Assert.Fail ("Expected " + expectedResult);
 			}
 
 			public void Dispose ()