Merge pull request #2056 from mattwar/Bug1154956

Reduce likelyhood of race in EventMap

src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests.cs

@@ -27,27 +27,38 @@ private TestWorkspace CreateWorkspace(bool disablePartialSolutions = true)
 
         private static void WaitForWorkspaceOperationsToComplete(TestWorkspace workspace)
         {
-            var workspasceWaiter = workspace.ExportProvider
+            var workspaceWaiter = workspace.ExportProvider
                 .GetExports<IAsynchronousOperationListener, FeatureMetadata>()
                 .First(l => l.Metadata.FeatureName == FeatureAttribute.Workspace).Value as IAsynchronousOperationWaiter;
-            workspasceWaiter.CreateWaitTask().PumpingWait();
+            workspaceWaiter.CreateWaitTask().PumpingWait();
         }
 
         [Fact]
-        public void TestEmptySolutionUpdate()
+        public void TestEmptySolutionUpdateDoesNotFireEvents()
         {
             using (var workspace = CreateWorkspace())
             {
                 var project = new TestHostProject(workspace);
                 workspace.AddTestProject(project);
+
+                // wait for all previous operations to complete
+                WaitForWorkspaceOperationsToComplete(workspace);
+
                 var solution = workspace.CurrentSolution;
                 bool workspaceChanged = false;
+
                 workspace.WorkspaceChanged += (s, e) => workspaceChanged = true;
 
+                // make an 'empty' update by claiming something changed, but its the same as before
                 workspace.OnParseOptionsChanged(project.Id, project.ParseOptions);
+
+                // wait for any new outstanding operations to complete (there shouldn't be any)
                 WaitForWorkspaceOperationsToComplete(workspace);
 
+                // same solution instance == nothing changed
                 Assert.Equal(solution, workspace.CurrentSolution);
+
+                // no event was fired because nothing was changed
                 Assert.False(workspaceChanged);
             }
         }

src/Features/Core/Diagnostics/DiagnosticService.cs

@@ -58,18 +58,14 @@ internal class DiagnosticService : IDiagnosticService
 
         private void RaiseDiagnosticsUpdated(object sender, DiagnosticsUpdatedArgs args)
         {
-            var handlers = _eventMap.GetEventHandlers<EventHandler<DiagnosticsUpdatedArgs>>(DiagnosticsUpdatedEventName);
-            if (handlers.Length > 0)
+            var ev = _eventMap.GetEventHandlers<EventHandler<DiagnosticsUpdatedArgs>>(DiagnosticsUpdatedEventName);
+            if (ev.HasHandlers)
             {
                 var eventToken = _listener.BeginAsyncOperation(DiagnosticsUpdatedEventName);
                 _eventQueue.ScheduleTask(() =>
                 {
                     UpdateDataMap(sender, args);
-
-                    foreach (var handler in handlers)
-                    {
-                        handler(sender, args);
-                    }
+                    ev.RaiseEvent(handler => handler(sender, args));
                 }).CompletesAsyncOperation(eventToken);
             }
         }

src/Features/Core/SolutionCrawler/SolutionCrawlerProgressReporter.cs

@@ -106,15 +106,12 @@ private Task RaiseStopped()
             private Task RaiseEvent(string eventName)
             {
                 // this method name doesnt have Async since it should work as async void.
-                var handlers = _eventMap.GetEventHandlers<EventHandler>(eventName);
-                if (handlers.Length > 0)
+                var ev = _eventMap.GetEventHandlers<EventHandler>(eventName);
+                if (ev.HasHandlers)
                 {
                     return _eventQueue.ScheduleTask(() =>
                     {
-                        foreach (var handler in handlers)
-                        {
-                            handler(this, EventArgs.Empty);
-                        }
+                        ev.RaiseEvent(handler => handler(this, EventArgs.Empty));
                     });
                 }
 

src/Workspaces/Core/Portable/Notification/GlobalOperationNotificationService.cs

@@ -49,15 +49,12 @@ public override GlobalOperationRegistration Start(string operation)
 
         protected virtual Task RaiseGlobalOperationStarted()
         {
-            var handlers = _eventMap.GetEventHandlers<EventHandler>(GlobalOperationStartedEventName);
-            if (handlers.Length > 0)
+            var ev = _eventMap.GetEventHandlers<EventHandler>(GlobalOperationStartedEventName);
+            if (ev.HasHandlers)
             {
                 return _eventQueue.ScheduleTask(() =>
                 {
-                    foreach (var handler in handlers)
-                    {
-                        handler(this, EventArgs.Empty);
-                    }
+                    ev.RaiseEvent(handler => handler(this, EventArgs.Empty));
                 });
             }
 
@@ -66,17 +63,14 @@ protected virtual Task RaiseGlobalOperationStarted()
 
         protected virtual Task RaiseGlobalOperationStopped(IReadOnlyList<string> operations, bool cancelled)
         {
-            var handlers = _eventMap.GetEventHandlers<EventHandler<GlobalOperationEventArgs>>(GlobalOperationStoppedEventName);
-            if (handlers.Length > 0)
+            var ev = _eventMap.GetEventHandlers<EventHandler<GlobalOperationEventArgs>>(GlobalOperationStoppedEventName);
+            if (ev.HasHandlers)
             {
                 var args = new GlobalOperationEventArgs(operations, cancelled);
 
                 return _eventQueue.ScheduleTask(() =>
                 {
-                    foreach (var handler in handlers)
-                    {
-                        handler(this, args);
-                    }
+                    ev.RaiseEvent(handler => handler(this, args));
                 });
             }
 

src/Workspaces/Core/Portable/Utilities/EventMap.cs

@@ -3,6 +3,7 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.Immutable;
+using System.Linq;
 
 namespace Roslyn.Utilities
 {
@@ -10,7 +11,7 @@ internal class EventMap
     {
         private readonly NonReentrantLock _guard = new NonReentrantLock();
 
-        private readonly Dictionary<string, object> _eventNameToHandlers =
+        private readonly Dictionary<string, object> _eventNameToRegistries =
             new Dictionary<string, object>();
 
         public EventMap()
@@ -18,64 +19,147 @@ public EventMap()
         }
 
         public void AddEventHandler<TEventHandler>(string eventName, TEventHandler eventHandler)
+            where TEventHandler : class
         {
             using (_guard.DisposableWait())
             {
-                var handlers = GetEvents_NoLock<TEventHandler>(eventName);
-                var newHandlers = handlers.Add(eventHandler);
-                SetEvents_NoLock(eventName, newHandlers);
+                var registries = GetRegistries_NoLock<TEventHandler>(eventName);
+                var newRegistries = registries.Add(new Registry<TEventHandler>(eventHandler));
+                SetRegistries_NoLock(eventName, newRegistries);
             }
         }
 
         public void RemoveEventHandler<TEventHandler>(string eventName, TEventHandler eventHandler)
+            where TEventHandler : class
         {
             using (_guard.DisposableWait())
             {
-                var handlers = GetEvents_NoLock<TEventHandler>(eventName);
-                var newHandlers = handlers.Remove(eventHandler);
-                if (newHandlers != handlers)
+                var registries = GetRegistries_NoLock<TEventHandler>(eventName);
+
+                // remove disabled registrations from list
+                var newRegistries = registries.RemoveAll(r => r.HasHandler(eventHandler));
+
+                if (newRegistries != registries)
+                {
+                    // disable all registrations of this handler (so pending raise events can be squelched)
+                    // This does not guarantee no race condition between Raise and Remove but greatly reduces it.
+                    foreach (var registry in registries.Where(r => r.HasHandler(eventHandler)))
                     {
-                    SetEvents_NoLock(eventName, newHandlers);
+                        registry.Unregister();
+                    }
+
+                    SetRegistries_NoLock(eventName, newRegistries);
+                }
             }
         }
+
+        public EventHandlerSet<TEventHandler> GetEventHandlers<TEventHandler>(string eventName)
+            where TEventHandler : class
+        {
+            return new EventHandlerSet<TEventHandler>(this.GetRegistries<TEventHandler>(eventName));
         }
 
-        public ImmutableArray<TEventHandler> GetEventHandlers<TEventHandler>(string eventName)
+        private ImmutableArray<Registry<TEventHandler>> GetRegistries<TEventHandler>(string eventName)
+            where TEventHandler : class
         {
             using (_guard.DisposableWait())
             {
-                return GetEvents_NoLock<TEventHandler>(eventName);
+                return GetRegistries_NoLock<TEventHandler>(eventName);
             }
         }
 
-        public void RaiseEvent<TEventArgs>(string eventName, object sender, TEventArgs args)
-            where TEventArgs : EventArgs
+        private ImmutableArray<Registry<TEventHandler>> GetRegistries_NoLock<TEventHandler>(string eventName)
+            where TEventHandler : class
         {
-            var handlers = GetEventHandlers<EventHandler<TEventArgs>>(eventName);
-            foreach (var handler in handlers)
+            _guard.AssertHasLock();
+
+            object registries;
+            if (_eventNameToRegistries.TryGetValue(eventName, out registries))
             {
-                handler(sender, args);
+                return (ImmutableArray<Registry<TEventHandler>>)registries;
             }
+
+            return ImmutableArray.Create<Registry<TEventHandler>>();
         }
 
-        private ImmutableArray<TEventHandler> GetEvents_NoLock<TEventHandler>(string eventName)
+        private void SetRegistries_NoLock<TEventHandler>(string eventName, ImmutableArray<Registry<TEventHandler>> registries)
+            where TEventHandler : class
         {
             _guard.AssertHasLock();
 
-            object handlers;
-            if (_eventNameToHandlers.TryGetValue(eventName, out handlers))
+            _eventNameToRegistries[eventName] = registries;
+        }
+
+        private class Registry<TEventHandler> : IEquatable<Registry<TEventHandler>>
+            where TEventHandler : class
+        {
+            private TEventHandler handler;
+
+            public Registry(TEventHandler handler)
             {
-                return (ImmutableArray<TEventHandler>)handlers;
+                this.handler = handler;
             }
 
-            return ImmutableArray.Create<TEventHandler>();
+            public void Unregister()
+            {
+                this.handler = null;
             }
 
-        private void SetEvents_NoLock<TEventHandler>(string name, ImmutableArray<TEventHandler> events)
+            public void Invoke(Action<TEventHandler> invoker)
             {
-            _guard.AssertHasLock();
+                var handler = this.handler;
+                if (handler != null)
+                {
+                    invoker(handler);
+                }
+            }
+
+            public bool HasHandler(TEventHandler handler)
+            {
+                return this.handler == handler;
+            }
 
-            _eventNameToHandlers[name] = events;
+            public bool Equals(Registry<TEventHandler> other)
+            {
+                return other != null && other.handler == this.handler;
+            }
+
+            public override bool Equals(object obj)
+            {
+                return Equals(obj as Registry<TEventHandler>);
+            }
+
+            public override int GetHashCode()
+            {
+                return this.handler.GetHashCode();
+            }
+        }
+
+        internal struct EventHandlerSet<TEventHandler>
+            where TEventHandler : class
+        {
+            private ImmutableArray<Registry<TEventHandler>> registries;
+
+            internal EventHandlerSet(object registries)
+            {
+                this.registries = (ImmutableArray<Registry<TEventHandler>>) registries;
+            }
+
+            public bool HasHandlers
+            {
+                get { return this.registries != null && this.registries.Length > 0; }
+            }
+
+            public void RaiseEvent(Action<TEventHandler> invoker)
+            {
+                if (this.HasHandlers)
+                {
+                    foreach (var registry in registries)
+                    {
+                        registry.Invoke(invoker);
+                    }
+                }
+            }
         }
     }
 }

src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs

@@ -48,16 +48,13 @@ protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solutio
                 projectId = documentId.ProjectId;
             }
 
-            var handlers = _eventMap.GetEventHandlers<EventHandler<WorkspaceChangeEventArgs>>(WorkspaceChangeEventName);
-            if (handlers.Length > 0)
+            var ev = _eventMap.GetEventHandlers<EventHandler<WorkspaceChangeEventArgs>>(WorkspaceChangeEventName);
+            if (ev.HasHandlers)
             {
                 return this.ScheduleTask(() =>
                 {
                     var args = new WorkspaceChangeEventArgs(kind, oldSolution, newSolution, projectId, documentId);
-                    foreach (var handler in handlers)
-                    {
-                        handler(this, args);
-                    }
+                    ev.RaiseEvent(handler => handler(this, args));
                 }, "Workspace.WorkspaceChanged");
             }
             else
@@ -85,14 +82,11 @@ protected Task RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind kind, Solutio
 
         protected internal virtual void OnWorkspaceFailed(WorkspaceDiagnostic diagnostic)
         {
-            var handlers = _eventMap.GetEventHandlers<EventHandler<WorkspaceDiagnosticEventArgs>>(WorkspaceFailedEventName);
-            if (handlers.Length > 0)
+            var ev = _eventMap.GetEventHandlers<EventHandler<WorkspaceDiagnosticEventArgs>>(WorkspaceFailedEventName);
+            if (ev.HasHandlers)
             {
                 var args = new WorkspaceDiagnosticEventArgs(diagnostic);
-                foreach (var handler in handlers)
-                {
-                    handler(this, args);
-                }
+                ev.RaiseEvent(handler => handler(this, args));
             }
         }
 
@@ -114,16 +108,13 @@ protected internal virtual void OnWorkspaceFailed(WorkspaceDiagnostic diagnostic
 
         protected Task RaiseDocumentOpenedEventAsync(Document document)
         {
-            var handlers = _eventMap.GetEventHandlers<EventHandler<DocumentEventArgs>>(DocumentOpenedEventName);
-            if (handlers.Length > 0)
+            var ev = _eventMap.GetEventHandlers<EventHandler<DocumentEventArgs>>(DocumentOpenedEventName);
+            if (ev.HasHandlers)
             {
                 return this.ScheduleTask(() =>
                 {
                     var args = new DocumentEventArgs(document);
-                    foreach (var handler in handlers)
-                    {
-                        handler(this, args);
-                    }
+                    ev.RaiseEvent(handler => handler(this, args));
                 }, "Workspace.WorkspaceChanged");
             }
             else
@@ -150,16 +141,13 @@ protected Task RaiseDocumentOpenedEventAsync(Document document)
 
         protected Task RaiseDocumentClosedEventAsync(Document document)
         {
-            var handlers = _eventMap.GetEventHandlers<EventHandler<DocumentEventArgs>>(DocumentClosedEventName);
-            if (handlers.Length > 0)
+            var ev = _eventMap.GetEventHandlers<EventHandler<DocumentEventArgs>>(DocumentClosedEventName);
+            if (ev.HasHandlers)
             {
                 return this.ScheduleTask(() =>
                 {
                     var args = new DocumentEventArgs(document);
-                    foreach (var handler in handlers)
-                    {
-                        handler(this, args);
-                    }
+                    ev.RaiseEvent(handler => handler(this, args));
                 }, "Workspace.DocumentClosed");
             }
             else
@@ -187,16 +175,13 @@ protected Task RaiseDocumentClosedEventAsync(Document document)
 
         protected Task RaiseDocumentActiveContextChangedEventAsync(Document document)
         {
-            var handlers = _eventMap.GetEventHandlers<EventHandler<DocumentEventArgs>>(DocumentActiveContextChangedName);
-            if (handlers.Length > 0)
+            var ev = _eventMap.GetEventHandlers<EventHandler<DocumentEventArgs>>(DocumentActiveContextChangedName);
+            if (ev.HasHandlers)
             {
                 return this.ScheduleTask(() =>
                 {
                     var args = new DocumentEventArgs(document);
-                    foreach (var handler in handlers)
-                    {
-                        handler(this, args);
-                    }
+                    ev.RaiseEvent(handler => handler(this, args));
                 }, "Workspace.WorkspaceChanged");
             }
             else