diff --git a/src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests.cs b/src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests.cs index 3bacbe7e26cfa2c7f1d296c602f24aba5f6a3bc4..48009bb39fd77e52c31224d565d27d58e4e3a049 100644 --- a/src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests.cs +++ b/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() .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); } } diff --git a/src/Features/Core/Diagnostics/DiagnosticService.cs b/src/Features/Core/Diagnostics/DiagnosticService.cs index 0ba84db145ea1d509f82fc7ab751df282b923d5b..0600d9bce2517c62eb6fb6a485cbd3f223b729e9 100644 --- a/src/Features/Core/Diagnostics/DiagnosticService.cs +++ b/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>(DiagnosticsUpdatedEventName); - if (handlers.Length > 0) + var ev = _eventMap.GetEventHandlers>(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); } } diff --git a/src/Features/Core/SolutionCrawler/SolutionCrawlerProgressReporter.cs b/src/Features/Core/SolutionCrawler/SolutionCrawlerProgressReporter.cs index 41cb413a5fc61f7b1a1c7bd6cbe12661bee05822..b0a31c39135c75428940d6da1f24feb94841a1dc 100644 --- a/src/Features/Core/SolutionCrawler/SolutionCrawlerProgressReporter.cs +++ b/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(eventName); - if (handlers.Length > 0) + var ev = _eventMap.GetEventHandlers(eventName); + if (ev.HasHandlers) { return _eventQueue.ScheduleTask(() => { - foreach (var handler in handlers) - { - handler(this, EventArgs.Empty); - } + ev.RaiseEvent(handler => handler(this, EventArgs.Empty)); }); } diff --git a/src/Workspaces/Core/Portable/Notification/GlobalOperationNotificationService.cs b/src/Workspaces/Core/Portable/Notification/GlobalOperationNotificationService.cs index 1c62545c1683595d328eaff8499d6534cf115655..6e09972b8a18f87ec09b138ed47963b174245c3c 100644 --- a/src/Workspaces/Core/Portable/Notification/GlobalOperationNotificationService.cs +++ b/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(GlobalOperationStartedEventName); - if (handlers.Length > 0) + var ev = _eventMap.GetEventHandlers(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 operations, bool cancelled) { - var handlers = _eventMap.GetEventHandlers>(GlobalOperationStoppedEventName); - if (handlers.Length > 0) + var ev = _eventMap.GetEventHandlers>(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)); }); } diff --git a/src/Workspaces/Core/Portable/Utilities/EventMap.cs b/src/Workspaces/Core/Portable/Utilities/EventMap.cs index 568a42af68f09ff1fce7ca563329a411023eb87f..2431aa4c3c8c022cbf8793e054d3a06aaa48ae3a 100644 --- a/src/Workspaces/Core/Portable/Utilities/EventMap.cs +++ b/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 _eventNameToHandlers = + private readonly Dictionary _eventNameToRegistries = new Dictionary(); public EventMap() @@ -18,64 +19,147 @@ public EventMap() } public void AddEventHandler(string eventName, TEventHandler eventHandler) + where TEventHandler : class { using (_guard.DisposableWait()) { - var handlers = GetEvents_NoLock(eventName); - var newHandlers = handlers.Add(eventHandler); - SetEvents_NoLock(eventName, newHandlers); + var registries = GetRegistries_NoLock(eventName); + var newRegistries = registries.Add(new Registry(eventHandler)); + SetRegistries_NoLock(eventName, newRegistries); } } public void RemoveEventHandler(string eventName, TEventHandler eventHandler) + where TEventHandler : class { using (_guard.DisposableWait()) { - var handlers = GetEvents_NoLock(eventName); - var newHandlers = handlers.Remove(eventHandler); - if (newHandlers != handlers) + var registries = GetRegistries_NoLock(eventName); + + // remove disabled registrations from list + var newRegistries = registries.RemoveAll(r => r.HasHandler(eventHandler)); + + if (newRegistries != registries) { - SetEvents_NoLock(eventName, newHandlers); + // 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))) + { + registry.Unregister(); + } + + SetRegistries_NoLock(eventName, newRegistries); } } } - public ImmutableArray GetEventHandlers(string eventName) + public EventHandlerSet GetEventHandlers(string eventName) + where TEventHandler : class + { + return new EventHandlerSet(this.GetRegistries(eventName)); + } + + private ImmutableArray> GetRegistries(string eventName) + where TEventHandler : class { using (_guard.DisposableWait()) { - return GetEvents_NoLock(eventName); + return GetRegistries_NoLock(eventName); } } - public void RaiseEvent(string eventName, object sender, TEventArgs args) - where TEventArgs : EventArgs + private ImmutableArray> GetRegistries_NoLock(string eventName) + where TEventHandler : class { - var handlers = GetEventHandlers>(eventName); - foreach (var handler in handlers) + _guard.AssertHasLock(); + + object registries; + if (_eventNameToRegistries.TryGetValue(eventName, out registries)) { - handler(sender, args); + return (ImmutableArray>)registries; } + + return ImmutableArray.Create>(); } - private ImmutableArray GetEvents_NoLock(string eventName) + private void SetRegistries_NoLock(string eventName, ImmutableArray> registries) + where TEventHandler : class { _guard.AssertHasLock(); - object handlers; - if (_eventNameToHandlers.TryGetValue(eventName, out handlers)) + _eventNameToRegistries[eventName] = registries; + } + + private class Registry : IEquatable> + where TEventHandler : class + { + private TEventHandler handler; + + public Registry(TEventHandler handler) + { + this.handler = handler; + } + + public void Unregister() { - return (ImmutableArray)handlers; + this.handler = null; } - return ImmutableArray.Create(); + public void Invoke(Action invoker) + { + var handler = this.handler; + if (handler != null) + { + invoker(handler); + } + } + + public bool HasHandler(TEventHandler handler) + { + return this.handler == handler; + } + + public bool Equals(Registry other) + { + return other != null && other.handler == this.handler; + } + + public override bool Equals(object obj) + { + return Equals(obj as Registry); + } + + public override int GetHashCode() + { + return this.handler.GetHashCode(); + } } - private void SetEvents_NoLock(string name, ImmutableArray events) + internal struct EventHandlerSet + where TEventHandler : class { - _guard.AssertHasLock(); + private ImmutableArray> registries; - _eventNameToHandlers[name] = events; + internal EventHandlerSet(object registries) + { + this.registries = (ImmutableArray>) registries; + } + + public bool HasHandlers + { + get { return this.registries != null && this.registries.Length > 0; } + } + + public void RaiseEvent(Action invoker) + { + if (this.HasHandlers) + { + foreach (var registry in registries) + { + registry.Invoke(invoker); + } + } + } } } } diff --git a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs b/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs index c033f2ca4d20b102f4c902b05191835572a26fe6..ddad8215dc9837468b0331202570920cb22a5594 100644 --- a/src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs +++ b/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>(WorkspaceChangeEventName); - if (handlers.Length > 0) + var ev = _eventMap.GetEventHandlers>(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>(WorkspaceFailedEventName); - if (handlers.Length > 0) + var ev = _eventMap.GetEventHandlers>(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>(DocumentOpenedEventName); - if (handlers.Length > 0) + var ev = _eventMap.GetEventHandlers>(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>(DocumentClosedEventName); - if (handlers.Length > 0) + var ev = _eventMap.GetEventHandlers>(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>(DocumentActiveContextChangedName); - if (handlers.Length > 0) + var ev = _eventMap.GetEventHandlers>(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