Merge pull request #200 from mono/dev/lluis/fix-nested-event-invoke

Fix nested event dispatch issue

5 files changed, 171 insertions, 20 deletions

diff --git a/Mono.Addins/Mono.Addins/AddinEngine.cs b/Mono.Addins/Mono.Addins/AddinEngine.cs
index 9986416..695623c 100644
--- a/Mono.Addins/Mono.Addins/AddinEngine.cs
+++ b/Mono.Addins/Mono.Addins/AddinEngine.cs
@@ -988,5 +988,7 @@ namespace Mono.Addins
 		{
 			notificationQueue.Invoke(action, source);
 		}
+
+		internal NotificationQueue NotificationQueue => notificationQueue;
 	}
 }
diff --git a/Mono.Addins/Mono.Addins/ExtensionContext.cs b/Mono.Addins/Mono.Addins/ExtensionContext.cs
index e46e657..5468179 100644
--- a/Mono.Addins/Mono.Addins/ExtensionContext.cs
+++ b/Mono.Addins/Mono.Addins/ExtensionContext.cs
@@ -1365,25 +1365,26 @@ namespace Mono.Addins
 	}
 
 	/// <summary>
-	/// A queue that can be used to dispatch callbacks sequentially
+	/// A queue that can be used to dispatch callbacks sequentially.
 	/// </summary>
 	class NotificationQueue
 	{
         readonly AddinEngine addinEngine;
-        readonly Queue<(Action Action,object Source)> notificationQueue = new Queue<(Action,object)>();
+        readonly Queue<(Action Action, object Source)> notificationQueue = new Queue<(Action, object)>();
 
 		bool sending;
+		int frozenCount;
 
 		public NotificationQueue(AddinEngine addinEngine)
 		{
             this.addinEngine = addinEngine;
         }
 
-		internal void Invoke(Action action, object source)
+		public void Invoke(Action action, object source)
 		{
 			lock (notificationQueue)
 			{
-				if (sending)
+				if (sending || frozenCount > 0)
 				{
 					// Already sending, enqueue the action so whoever is sending will take it
 					notificationQueue.Enqueue((action,source));
@@ -1397,23 +1398,55 @@ namespace Mono.Addins
 			}
 
 			SafeInvoke(action, source);
+			DispatchPendingCallbacks();
+		}
 
+		void DispatchPendingCallbacks()
+		{
 			do
 			{
+				Action action;
+				object source;
+
 				lock (notificationQueue)
 				{
-					if (notificationQueue.Count == 0)
+					if (notificationQueue.Count == 0 || frozenCount > 0)
 					{
 						sending = false;
 						return;
 					}
-					(action,source) = notificationQueue.Dequeue();
+
+					(action, source) = notificationQueue.Dequeue();
 				}
 				SafeInvoke(action, source);
 			}
 			while (true);
 		}
 
+		public void Freeze()
+		{
+			lock (notificationQueue)
+			{
+				frozenCount++;
+			}
+		}
+
+		public void Unfreeze()
+		{
+			bool dispatch = false;
+
+			lock (notificationQueue)
+			{
+				if (--frozenCount == 0 && !sending)
+				{
+					dispatch = true;
+					sending = true;
+				}
+			}
+			if (dispatch)
+				DispatchPendingCallbacks();
+		}
+
 		void SafeInvoke(Action action, object source)
 		{
 			try
diff --git a/Mono.Addins/Mono.Addins/ExtensionNode.cs b/Mono.Addins/Mono.Addins/ExtensionNode.cs
index 0947a89..bc48ac1 100644
--- a/Mono.Addins/Mono.Addins/ExtensionNode.cs
+++ b/Mono.Addins/Mono.Addins/ExtensionNode.cs
@@ -179,21 +179,47 @@ namespace Mono.Addins
 		public event ExtensionNodeEventHandler ExtensionNodeChanged {
 			add
 			{
-				addinEngine.InvokeCallback(() =>
+				ExtensionNodeList children;
+				bool frozen = false;
+
+				try
 				{
-					// The invocation here needs to be done right to make sure there are no duplicate or missing events.
+					lock (localLock)
+					{
+						// The invocation here needs to be done right to make sure there are no duplicate or missing events.
+
+						// 1) Get the list of current nodes and store it in a variable. This is the list of nodes for which
+						// notifications will initially be sent. We hold the node lock to prevent node changes during
+						// the event subscription.
 
-					// 1) Get the list of current nodes and store it in a variable. This is the list of nodes for which
-					// notifications will initially be sent.
+						children = GetChildNodes();
 
-					var children = GetChildNodes();
+						// 2) Subscribe the real event, but do it through the notification queue. Doing it through the queue
+						// is important since then the event will be subscribed once all currently queued events are dispatched.
+						//
+						// An example: let's say a new child is added to this node, but the event queue was frozen so the
+						// Child Added event is still in the queue. Then the ExtensionNodeChanged event is subscribed for
+						// the parent. In this case the GetChildNodes() call above will contain the new child (because it
+						// has already been added), so the handler will be invoked for that node. Child Added event that's
+						// in the queue should be ignored for this handler, since it has already been raised when subscribing.
+						//
+						// To solve this problem, we do the event subscription through the event queue. In this way,
+						// the actual subscription won't be done until the current queue is flushed, so the handler will
+						// only be called for any event that was queued just after the GetChildNodes() call.
 
-					// 2) Subscribe the real event. If nodes were added or removed since the above GetChildNodes
-					// call, callback invocations will now be queued (since we are inside InvokeCallback).
+						// Freeze the queue here since we don't want callbacks to be invoked while holding the node lock.
 
-					extensionNodeChanged += value;
+						addinEngine.NotificationQueue.Freeze();
+						frozen = true;
+
+						addinEngine.NotificationQueue.Invoke(() =>
+						{
+							extensionNodeChanged += value;
+						},
+						this);
+					}
 
-					// 3) Send the notifications for the events. Again, any change done after the above GetChildNodes
+					// 3) Send the notifications for the event. Again, any change done after the above GetChildNodes
 					// call will have queued a notification.
 
 					foreach (ExtensionNode node in children)
@@ -201,11 +227,17 @@ namespace Mono.Addins
 						var theNode = node;
 						value(this, new ExtensionNodeEventArgs(ExtensionChange.Add, theNode));
 					}
-
+				}
+				finally
+				{
 					// 4) We are done notifying the pre-existing nodes. If there was any further change in the children
-					// list, the corresponding events will be raised after this callback ends.
+					// list, the corresponding events will be raised after unfreezing.
 
-				}, this);
+					if (frozen)
+					{
+						addinEngine.NotificationQueue.Unfreeze();
+					}
+				}
 			}
 			remove {
 				addinEngine.InvokeCallback(() =>
diff --git a/Test/UnitTests/TestEvents.cs b/Test/UnitTests/TestEvents.cs
index 2b4b988..69fce20 100644
--- a/Test/UnitTests/TestEvents.cs
+++ b/Test/UnitTests/TestEvents.cs
@@ -479,5 +479,89 @@ namespace UnitTests
         private void ExtensionNode_ExtensionNodeChanged (object sender, ExtensionNodeEventArgs args)
         {
         }
-    }
+
+		[Test]
+		public void TestSubscriptionWithinHandler()
+		{
+			nestedEventSubscriptionSet = false;
+			nestedEventSubscriptionHandled = false;
+			try
+			{
+				AddinManager.AddExtensionNodeHandler("/SimpleApp/Writers", OnExtensionChange3);
+			}
+			finally
+			{
+				AddinManager.RemoveExtensionNodeHandler("/SimpleApp/Writers", OnExtensionChange3);
+				AddinManager.RemoveExtensionNodeHandler("/SystemInformation/Modules", OnExtensionChange4);
+			}
+		}
+
+		bool nestedEventSubscriptionSet;
+		bool nestedEventSubscriptionHandled;
+
+		void OnExtensionChange3(object s, ExtensionNodeEventArgs args)
+		{
+			if (args.Change == ExtensionChange.Add)
+			{
+				if (!nestedEventSubscriptionSet)
+				{
+					nestedEventSubscriptionSet = true;
+
+					// The OnExtensionChange4 handler should be invoked immediately for existing nodes. 
+					AddinManager.AddExtensionNodeHandler("/SystemInformation/Modules", OnExtensionChange4);
+					Assert.IsTrue(nestedEventSubscriptionHandled);
+				}
+			}
+		}
+
+		void OnExtensionChange4(object s, ExtensionNodeEventArgs args)
+		{
+			nestedEventSubscriptionHandled = true;
+		}
+
+		int conditionedWriterCount = 0;
+
+		[Test]
+		public void TestSubscriptionWithinHandler2()
+		{
+			try
+			{
+				AddinManager.GetExtensionNodes("/SimpleApp/ConditionedWriters");
+				nestedEventSubscriptionSet = false;
+				GlobalInfoCondition.Value = "";
+
+				AddinManager.ExtensionChanged += ExtensionChanged6;
+
+				AddinManager.Registry.DisableAddin("SimpleApp.FileContentExtension,0.1.0");
+
+				Assert.AreEqual(1, conditionedWriterCount);
+			}
+			finally
+			{
+				AddinManager.Registry.EnableAddin("SimpleApp.FileContentExtension,0.1.0");
+			}
+		}
+
+        private void ExtensionChanged6 (object sender, ExtensionEventArgs args)
+        {
+			if (nestedEventSubscriptionSet)
+				return;
+
+			nestedEventSubscriptionSet = true;
+
+			GlobalInfoCondition.Value = "foo";
+
+			nestedEventSubscriptionSet = true;
+			AddinManager.AddExtensionNodeHandler("/SimpleApp/ConditionedWriters", OnConditionedWritersChanged);
+			Assert.AreEqual(1, conditionedWriterCount);
+		}
+
+		void OnConditionedWritersChanged(object s, ExtensionNodeEventArgs args)
+		{
+			if (args.Change == ExtensionChange.Add)
+				conditionedWriterCount++;
+			else
+				conditionedWriterCount--;
+		}
+	}
 }
diff --git a/Version.props b/Version.props
index cc2b09f..e40bdce 100644
--- a/Version.props
+++ b/Version.props
@@ -1,6 +1,6 @@
 <Project>
   <PropertyGroup>
-    <PackageVersion>1.4.2-alpha.3</PackageVersion>
+    <PackageVersion>1.4.2-alpha.4</PackageVersion>
     <Authors>Microsoft</Authors>
     <Owners>microsoft, xamarin</Owners>
     <PackageLicenseUrl>https://github.com/mono/mono-addins/blob/main/COPYING</PackageLicenseUrl>