From fdd18d500e241363c316e2c7fa123716c745217d Mon Sep 17 00:00:00 2001 From: David Kean Date: Wed, 14 Nov 2018 22:00:32 +1100 Subject: [PATCH] IWorkspaceProjectContextHost.Loaded -> PublishAsync This fixes a deadlock when the UI thread is blocked on Loaded, such as during PrioritizedProjectLoad. This occured because MultiLifetimeComponent.Loaded was backed by a TaskCompletionSource that did not have a JoinableTask relationship to the code that completed it (MultiLifetimeComponent.LoadAsync). I sat down with Andrew Arnott came up with a design that creates the relationship. I've used the name Publish (mimicing ITreeService, etc) as this method is basically the precursor to https://github.com/dotnet/project-system/issues/3425. --- .../AbstractMultiLifetimeComponentFactory.cs | 10 ++- .../AbstractMultiLifetimeComponentTests.cs | 64 ++++++++------ .../WorkspaceContextHostTests.cs | 38 +++++--- ...ctiveWorkspaceProjectContextHostFactory.cs | 7 +- .../VsContainedLanguageComponentsFactory.cs | 2 +- .../AbstractMultiLifetimeComponent.cs | 88 ++++++++++++------- .../IWorkspaceProjectContextHost.cs | 21 +++-- .../LanguageServices/LanguageServiceHost.cs | 10 +-- .../LanguageServices/WorkspaceContextHost.cs | 21 ++--- 9 files changed, 156 insertions(+), 105 deletions(-) diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/Mocks/AbstractMultiLifetimeComponentFactory.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/Mocks/AbstractMultiLifetimeComponentFactory.cs index b8491e73975..8d81631faf2 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/Mocks/AbstractMultiLifetimeComponentFactory.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/Mocks/AbstractMultiLifetimeComponentFactory.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Threading; using System.Threading.Tasks; using Microsoft.VisualStudio.Threading; @@ -14,14 +15,14 @@ public static MultiLifetimeComponent Create() return new MultiLifetimeComponent(joinableTaskContextNode); } - public class MultiLifetimeComponent : AbstractMultiLifetimeComponent + public class MultiLifetimeComponent : AbstractMultiLifetimeComponent { public MultiLifetimeComponent(JoinableTaskContextNode joinableTaskContextNode) : base(joinableTaskContextNode) { } - protected override IMultiLifetimeInstance CreateInstance() + protected override MultiLifetimeInstance CreateInstance() { return new MultiLifetimeInstance(); } @@ -31,6 +32,11 @@ protected override IMultiLifetimeInstance CreateInstance() get { return base.IsInitialized; } } + public new Task PublishInstanceAsync(CancellationToken cancellationToken = default) + { + return base.PublishInstanceAsync(cancellationToken); + } + public class MultiLifetimeInstance : IMultiLifetimeInstance { public bool IsInitialized diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/ProjectSystem/AbstractMultiLifetimeComponentTests.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/ProjectSystem/AbstractMultiLifetimeComponentTests.cs index bbf8794c4a1..85f7ad2df38 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/ProjectSystem/AbstractMultiLifetimeComponentTests.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/ProjectSystem/AbstractMultiLifetimeComponentTests.cs @@ -3,64 +3,73 @@ using System.Threading.Tasks; using Xunit; -using static Microsoft.VisualStudio.ProjectSystem.AbstractMultiLifetimeComponentFactory.MultiLifetimeComponent; namespace Microsoft.VisualStudio.ProjectSystem { public class AbstractMultiLifetimeComponentTests { [Fact] - public void Loaded_WhenNotUnloaded_ReturnsNonCompletedTask() + public void PublishInstanceAsync_WhenNotUnPublishInstanceAsync_ReturnsNonCompletedTask() { var component = CreateInstance(); - Assert.False(component.Loaded.IsCanceled); - Assert.False(component.Loaded.IsCompleted); - Assert.False(component.Loaded.IsFaulted); + var result = component.PublishInstanceAsync(); + + Assert.False(result.IsCanceled); + Assert.False(result.IsCompleted); + Assert.False(result.IsFaulted); } [Fact] - public async Task Loaded_WhenLoaded_ReturnsCompletedTask() + public async Task PublishInstanceAsync_WhenLoaded_ReturnsCompletedTask() { var component = CreateInstance(); await component.LoadAsync(); - Assert.True(component.Loaded.IsCompleted); + var result = component.PublishInstanceAsync(); + + Assert.True(result.IsCompleted); } [Fact] - public async Task Loaded_WhenUnloaded_ReturnsNonCompletedTask() + public async Task PublishInstanceAsync_WhenUnloaded_ReturnsNonCompletedTask() { var component = CreateInstance(); await component.LoadAsync(); await component.UnloadAsync(); - Assert.False(component.Loaded.IsCanceled); - Assert.False(component.Loaded.IsCompleted); - Assert.False(component.Loaded.IsFaulted); + var result = component.PublishInstanceAsync(); + + Assert.False(result.IsCanceled); + Assert.False(result.IsCompleted); + Assert.False(result.IsFaulted); } [Fact] - public async Task Loaded_DisposedWhenUnloaded_ReturnsCancelledTask() + public async Task PublishInstanceAsync_DisposedWhenUnloaded_ReturnsCancelledTask() { var component = CreateInstance(); await component.DisposeAsync(); - Assert.True(component.Loaded.IsCanceled); + var result = component.PublishInstanceAsync(); + + Assert.True(result.IsCanceled); } [Fact] - public async Task Loaded_DisposedWhenLoaded_ReturnsCancelledTask() + public async Task PublishInstanceAsync_DisposedWhenLoaded_ReturnsCancelledTask() { var component = CreateInstance(); await component.LoadAsync(); await component.DisposeAsync(); - Assert.True(component.Loaded.IsCanceled); + var result = component.PublishInstanceAsync(); + + Assert.True(result.IsCanceled); } [Fact] @@ -80,7 +89,7 @@ public async Task LoadAsync_InitializesUnderlyingInstance() await component.LoadAsync(); - var result = (MultiLifetimeInstance)component.Instance; + var result = await component.PublishInstanceAsync(); Assert.True(result.IsInitialized); } @@ -92,11 +101,13 @@ public async Task LoadAsync_WhenAlreadyLoaded_DoesNotCreateNewInstance() await component.LoadAsync(); - var instance = component.Instance; + var instance = await component.PublishInstanceAsync(); await component.LoadAsync(); - Assert.Same(instance, component.Instance); + var result = await component.PublishInstanceAsync(); + + Assert.Same(instance, result); } [Fact] @@ -106,14 +117,16 @@ public async Task LoadAsync_WhenUnloaded_CreatesNewInstance() await component.LoadAsync(); - var instance = component.Instance; + var instance = await component.PublishInstanceAsync(); await component.UnloadAsync(); // We should create a new instance here await component.LoadAsync(); - Assert.NotSame(instance, component.Instance); + var result = await component.PublishInstanceAsync(); + + Assert.NotSame(instance, result); } [Fact] @@ -122,12 +135,11 @@ public async Task UnloadAsync_WhenLoaded_DisposesUnderlyingInstance() var component = CreateInstance(); await component.LoadAsync(); - var instance = component.Instance; + var result = await component.PublishInstanceAsync(); await component.UnloadAsync(); - Assert.Null(component.Instance); - Assert.True(((MultiLifetimeInstance)instance).IsDisposed); + Assert.True(result.IsDisposed); } [Fact] @@ -136,8 +148,6 @@ public async Task UnloadAsync_WhenNotLoaded_DoesNothing() var component = CreateInstance(); await component.UnloadAsync(); - - Assert.Null(component.Instance); } [Fact] @@ -165,11 +175,11 @@ public async Task Dispose_WhenLoaded_DisposesUnderlyingInstance() var component = CreateInstance(); await component.LoadAsync(); - var instance = component.Instance; + var instance = await component.PublishInstanceAsync(); component.Dispose(); - Assert.True(((MultiLifetimeInstance)instance).IsDisposed); + Assert.True(instance.IsDisposed); } private static AbstractMultiLifetimeComponentFactory.MultiLifetimeComponent CreateInstance() diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/ProjectSystem/LanguageServices/WorkspaceContextHostTests.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/ProjectSystem/LanguageServices/WorkspaceContextHostTests.cs index f183020bb8d..0ed5b877d31 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/ProjectSystem/LanguageServices/WorkspaceContextHostTests.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/ProjectSystem/LanguageServices/WorkspaceContextHostTests.cs @@ -11,57 +11,67 @@ namespace Microsoft.VisualStudio.ProjectSystem.LanguageServices public class WorkspaceContextHostTests { [Fact] - public void Loaded_WhenNotActivated_ReturnsNonCompletedTask() + public void PublishAsync_WhenNotActivated_ReturnsNonCompletedTask() { var component = CreateInstance(); - Assert.False(component.Loaded.IsCanceled); - Assert.False(component.Loaded.IsCompleted); - Assert.False(component.Loaded.IsFaulted); + var result = component.PublishAsync(); + + Assert.False(result.IsCanceled); + Assert.False(result.IsCompleted); + Assert.False(result.IsFaulted); } [Fact] - public async Task Loaded_WhenActivated_ReturnsCompletedTask() + public async Task PublishAsync_WhenActivated_ReturnsCompletedTask() { var component = CreateInstance(); await component.ActivateAsync(); - Assert.True(component.Loaded.IsCompleted); + var result = component.PublishAsync(); + + Assert.True(result.IsCompleted); } [Fact] - public async Task Loaded_WhenDeactivated_ReturnsNonCompletedTask() + public async Task PublishAsync_WhenDeactivated_ReturnsNonCompletedTask() { var component = CreateInstance(); await component.ActivateAsync(); await component.UnloadAsync(); - Assert.False(component.Loaded.IsCanceled); - Assert.False(component.Loaded.IsCompleted); - Assert.False(component.Loaded.IsFaulted); + var result = component.PublishAsync(); + + Assert.False(result.IsCanceled); + Assert.False(result.IsCompleted); + Assert.False(result.IsFaulted); } [Fact] - public async Task Loaded_DisposedWhenNotActivated_ReturnsCancelledTask() + public async Task PublishAsync_DisposedWhenNotActivated_ReturnsCancelledTask() { var component = CreateInstance(); await component.DisposeAsync(); - Assert.True(component.Loaded.IsCanceled); + var result = component.PublishAsync(); + + Assert.True(result.IsCanceled); } [Fact] - public async Task Loaded_DisposedWhenActivated_ReturnsCancelledTask() + public async Task PublishAsync_DisposedWhenActivated_ReturnsCancelledTask() { var component = CreateInstance(); await component.ActivateAsync(); await component.DisposeAsync(); - Assert.True(component.Loaded.IsCanceled); + var result = component.PublishAsync(); + + Assert.True(result.IsCanceled); } [Fact] diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/Mocks/IActiveWorkspaceProjectContextHostFactory.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/Mocks/IActiveWorkspaceProjectContextHostFactory.cs index 586d3604345..214ea52916d 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/Mocks/IActiveWorkspaceProjectContextHostFactory.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/Mocks/IActiveWorkspaceProjectContextHostFactory.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Threading; using System.Threading.Tasks; using Moq; @@ -29,9 +30,9 @@ public ActiveWorkspaceProjectContextHost(IWorkspaceProjectContextAccessor access _accessor = accessor; } - public Task Loaded + public Task PublishAsync(CancellationToken cancellationToken = default) { - get { return Task.CompletedTask; } + return Task.CompletedTask; } public Task OpenContextForWriteAsync(Func action) @@ -43,6 +44,8 @@ public Task OpenContextForWriteAsync(Func GetContainedLanguageFactoryForFileAsync(string filePath) { - await _projectContextHost.Loaded; + await _projectContextHost.PublishAsync(); await _projectVsServices.ThreadingService.SwitchToUIThread(); diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/AbstractMultiLifetimeComponent.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/AbstractMultiLifetimeComponent.cs index 44a4ec1b398..ed41347d148 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/AbstractMultiLifetimeComponent.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/AbstractMultiLifetimeComponent.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Threading; using System.Threading.Tasks; @@ -12,33 +13,43 @@ namespace Microsoft.VisualStudio.ProjectSystem /// a component that is loaded and unloaded multiple times. /// internal abstract class AbstractMultiLifetimeComponent : OnceInitializedOnceDisposedAsync - where T : IMultiLifetimeInstance + where T : class, IMultiLifetimeInstance { private readonly object _lock = new object(); - private TaskCompletionSource _loadedSource = new TaskCompletionSource(); - private T _instance; + private TaskCompletionSource<(T instance, JoinableTask initializeAsyncTask)> _instanceTaskSource = new TaskCompletionSource<(T instance, JoinableTask initializeAsyncTask)>(); protected AbstractMultiLifetimeComponent(JoinableTaskContextNode joinableTaskContextNode) - : base(joinableTaskContextNode) + : base(joinableTaskContextNode) { } - public T Instance - { - get { return _instance; } - } - /// - /// Gets a task that is completed when current has - /// completed loading. + /// Returns a task that will complete when current has completed + /// loading and has published its instance. /// + /// + /// The result is awaited and the is unloaded. + /// + /// -or + /// + /// The result is awaited and is cancelled. + /// /// - /// The returned is canceled when the - /// is disposed. + /// This method does not initiate loading of the , however, + /// it will join the load when it starts. /// - public Task Loaded + protected async Task PublishInstanceAsync(CancellationToken cancellationToken = default) { - get { return _loadedSource.Task; } + // Wait until LoadAsync has been called, force switching to thread-pool in case + // there's already someone waiting for us on the UI thread. + (T instance, JoinableTask initializeAsyncTask) = await _instanceTaskSource.Task.WithCancellation(cancellationToken) + .ConfigureAwait(false); + + // Now join Instance.InitializeAsync so that if someone is waiting on the UI thread for us, + // the instance is allowed to switch to that thread to complete if needed. + await initializeAsyncTask.JoinAsync(cancellationToken); + + return instance; } public async Task LoadAsync() @@ -48,38 +59,37 @@ public async Task LoadAsync() await LoadCoreAsync(); } - private async Task LoadCoreAsync() + public async Task LoadCoreAsync() { - TaskCompletionSource loadedSource = null; - IMultiLifetimeInstance instance; + JoinableTask initializeAsyncTask; + lock (_lock) { - if (_instance == null) + if (!_instanceTaskSource.Task.IsCompleted) { - _instance = CreateInstance(); - loadedSource = _loadedSource; + (T instance, JoinableTask initializeAsyncTask) result = CreateInitializedInstanceAsync(); + _instanceTaskSource.SetResult(result); } - instance = _instance; + Assumes.True(_instanceTaskSource.Task.IsCompleted); + + // Should throw TaskCancellatedException if already cancelled in Dispose + (_, initializeAsyncTask) = _instanceTaskSource.Task.GetAwaiter().GetResult(); } - // While all callers should wait on InitializeAsync, - // only one should complete the completion source - await instance.InitializeAsync(); - loadedSource?.SetResult(null); + await initializeAsyncTask; } public Task UnloadAsync() { - IMultiLifetimeInstance instance = null; - + T instance = null; lock (_lock) { - if (_instance != null) + if (_instanceTaskSource.Task.IsCompleted) { - instance = _instance; - _instance = default; - _loadedSource = new TaskCompletionSource(); + // Should throw TaskCancellatedException if already cancelled in Dispose + (instance, _) = _instanceTaskSource.Task.GetAwaiter().GetResult(); + _instanceTaskSource = new TaskCompletionSource<(T instance, JoinableTask initializeAsyncTask)>(); } } @@ -95,7 +105,10 @@ protected override async Task DisposeCoreAsync(bool initialized) { await UnloadAsync(); - _loadedSource.TrySetCanceled(); + lock (_lock) + { + _instanceTaskSource.TrySetCanceled(); + } } protected override Task InitializeCoreAsync(CancellationToken cancellationToken) @@ -107,5 +120,14 @@ protected override Task InitializeCoreAsync(CancellationToken cancellationToken) /// Creates a new instance of the underlying . /// protected abstract T CreateInstance(); + + private (T instance, JoinableTask initializeAsyncTask) CreateInitializedInstanceAsync() + { + T instance = CreateInstance(); + + JoinableTask initializeAsyncTask = JoinableFactory.RunAsync(instance.InitializeAsync); + + return (instance, initializeAsyncTask); + } } } diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/IWorkspaceProjectContextHost.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/IWorkspaceProjectContextHost.cs index 6948d19c0bd..609c28cab00 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/IWorkspaceProjectContextHost.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/IWorkspaceProjectContextHost.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Threading; using System.Threading.Tasks; using Microsoft.VisualStudio.LanguageServices.ProjectSystem; @@ -13,17 +14,22 @@ namespace Microsoft.VisualStudio.ProjectSystem.LanguageServices internal interface IWorkspaceProjectContextHost { /// - /// Gets a task that is completed when current has - /// completed loading. + /// Returns a task that will complete when current has completed + /// loading. /// /// /// The result is awaited and the is unloaded. + /// + /// -or + /// + /// The result is awaited and is cancelled. /// - Task Loaded - { - get; - } - + /// + /// This method does not initiate loading of the , however, + /// it will join the load when it starts. + /// + Task PublishAsync(CancellationToken cancellationToken = default); + /// /// Opens the , passing it to the specified action for writing. /// @@ -38,7 +44,6 @@ Task Loaded /// Task OpenContextForWriteAsync(Func action); - /// /// Opens the , passing it to the specified action for writing. /// diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs index e3b383a8e52..3ae77322e88 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs @@ -80,7 +80,7 @@ public LanguageServiceHost(IUnconfiguredProjectCommonServices commonServices, [AppliesTo(ProjectCapability.DotNetLanguageService)] public Task OnProjectFactoryCompletedAsync() { - return Loaded; + return PublishAsync(); } protected override Task InitializeCoreAsync(CancellationToken cancellationToken) @@ -96,21 +96,21 @@ protected override Task InitializeCoreAsync(CancellationToken cancellationToken) return Task.CompletedTask; } - public Task Loaded + public Task PublishAsync(CancellationToken cancellationToken = default) { - get { return InitializeAsync(CancellationToken.None); } + return InitializeAsync(cancellationToken); } public async Task OpenContextForWriteAsync(Func action) { - await Loaded; + await PublishAsync(); await action(new WorkspaceProjectContextAccessor(ActiveProjectContext)); } public async Task OpenContextForWriteAsync(Func> action) { - await Loaded; + await PublishAsync(); return await action(new WorkspaceProjectContextAccessor(ActiveProjectContext)); } diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/WorkspaceContextHost.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/WorkspaceContextHost.cs index ce73b21812f..f56916ed51c 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/WorkspaceContextHost.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/WorkspaceContextHost.cs @@ -2,6 +2,7 @@ using System; using System.ComponentModel.Composition; +using System.Threading; using System.Threading.Tasks; using Microsoft.VisualStudio.LanguageServices.ProjectSystem; @@ -53,11 +54,16 @@ public Task DeactivateAsync() return UnloadAsync(); } + public Task PublishAsync(CancellationToken cancellationToken = default) + { + return PublishInstanceAsync(cancellationToken); + } + public async Task OpenContextForWriteAsync(Func action) { Requires.NotNull(action, nameof(action)); - WorkspaceContextHostInstance instance = await GetLoadedInstanceAsync(); + WorkspaceContextHostInstance instance = await PublishInstanceAsync(); // Throws OperationCanceledException if 'instance' is Disposed await instance.OpenContextForWriteAsync(action); @@ -67,23 +73,12 @@ public async Task OpenContextForWriteAsync(Func GetLoadedInstanceAsync() - { - await Loaded; - - WorkspaceContextHostInstance instance = Instance; - if (instance == null) // Unloaded between being Loaded and here - throw new OperationCanceledException(); - - return instance; - } - protected override WorkspaceContextHostInstance CreateInstance() { return new WorkspaceContextHostInstance(_project, _threadingService, _tasksService, _projectSubscriptionService, _workspaceProjectContextProvider, _activeWorkspaceProjectContextTracker, _applyChangesToWorkspaceContextFactory);