Fix nested event dispatch issue

Event handler invocations are done through a dispatch queue, to ensure that
they are dispatched in the right order in multi-threading scenarios.
The dispatch queue is also used when invoking a handler as a result of
an event subscription. For example, when calling AddExtensionNodeHandler,
the handler should be immediately invoked for all existing nodes in the provided
extension path. However, if AddExtensionNodeHandler was invoked within an
event handler, those invocations would be queued and not executed immediately
as the caller would expect.

The solution is to do the handler invocation for existing nodes like it used
to be, not through the queue. However, the actual event subscription still
needs to be done through the queue, to avoid receiving unnecessary events.
There is a more complete explanation in the code.

Mono.Addins/Mono.Addins/AddinEngine.cs

@@ -988,5 +988,7 @@ namespace Mono.Addins
 		{
 			notificationQueue.Invoke(action, source);
 		}
+
+		internal NotificationQueue NotificationQueue => notificationQueue;
 	}
 }

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

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.
+						// 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.
 
-					var children = GetChildNodes();
+						children = GetChildNodes();
 
-					// 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).
+						// 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.
 
-					extensionNodeChanged += value;
+						// Freeze the queue here since we don't want callbacks to be invoked while holding the node lock.
 
-					// 3) Send the notifications for the events. Again, any change done after the above GetChildNodes
+						addinEngine.NotificationQueue.Freeze();
+						frozen = true;
+
+						addinEngine.NotificationQueue.Invoke(() =>
+						{
+							extensionNodeChanged += value;
+						},
+						this);
+					}
+
+					// 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(() =>

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--;
+		}
+	}
 }

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>