From d8cd68c64f05b401edcf9ed094a14c5df25b93db Mon Sep 17 00:00:00 2001 From: Tom Meschter Date: Thu, 2 Mar 2023 16:15:31 -0800 Subject: [PATCH] Do not throw ObjectDisposedException in OnWorkspaceUpdateAsync The `Workspace` type (which is responsible for passing data from a single `ConfiguredProject` through to the Language Service) derives from `OnceInitializedOnceDisposedUnderLockAsync`; the latter ensures that initialization and disposal will happen at most once and allows implementers to protect themselves from disposal while in the middle of work. This is done by calling the protected `ExecuteUnderLockAsync` and passing in a delegate representing the work to be protected. There are a couple of potential problems, however. First, `Workspace` does not protect _all_ of its work; usually it does some amount of validation and processing and then calls `ExecuteUnderLockAsync` for the parts that may change state. Second, the methods that can be called from other types assert that the instance has not already been disposed by calling `Verify.NotDisposed(this)`; this throws an `ObjectDisposedException` if the verification fails. As best I can tell the first _potential_ problem is not an actual problem. By inspecting the code it appears that none of the state accessed outside `ExecuteUnderLockAsync` will be problematic if accessed after disposal. So I'm going to leave that alone for now. This commit addresses the second issue, at least in part. The `OnWorkspaceUpdateAsync` method is called from a dataflow block when new evaluation or design-time build data becomes available and we need to process it and update the Language Service. It starts with a call to `Verify.NotDisposed(this)`. However, a `Workspace` is disposed by the `LanguageServiceHost` when it decides it is no longer needed--which is based on data from a _different_ workflow. I can see no indication that the processing of these two workflows is coordinated, so the `Workspace` could be disposed after the call to `NotDisposed`, or we could see calls to `OnWorkspaceUpdateAsync` after disposal. And indeed I've occasionally seen the latter. (Note that disposing the Workspace will break the links in the dataflow subscription, but by that time the dataflow block may have already decided to call `OnWorkspaceUpdateAsync` with the current input value.) Here we replace the call to `Verify.NotDisposed(this)` with a simple check to see if the `Workspace` has already been disposed, or is in the process. In that case we can simply bail out early; since the `Workspace` has been disposed we know we don't care about processing the evaluation or build data. Note that this may not be a complete fix; as previously mentioned disposal could occur during `OnWorkspaceUpdateAsync`. This is instead the minimal change that I believe could be sufficient to avoid the exception without introducing other problems. A more certain fix would be to have each entry point to the `Workspace` wrap its work in `ExecuteUnderLockAsync`. However that is a more invasive change so I'm hoping to avoid it. --- .../ProjectSystem/VS/LanguageServices/Workspace.cs | 7 +++++-- .../VS/LanguageServices/WorkspaceTests.cs | 12 +++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/LanguageServices/Workspace.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/LanguageServices/Workspace.cs index 10f68a3a7d4..d35b874a75d 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/LanguageServices/Workspace.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/LanguageServices/Workspace.cs @@ -166,8 +166,11 @@ public void ChainDisposal(IDisposable disposable) /// A task that completes when the update has been integrated. internal async Task OnWorkspaceUpdateAsync(IProjectVersionedValue update) { - Verify.NotDisposed(this); - + if (IsDisposed || IsDisposing) + { + return; + } + await InitializeAsync(_unloadCancellationToken); Assumes.True(_state is WorkspaceState.Uninitialized or WorkspaceState.Initialized); diff --git a/tests/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/ProjectSystem/VS/LanguageServices/WorkspaceTests.cs b/tests/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/ProjectSystem/VS/LanguageServices/WorkspaceTests.cs index 7fb1a421313..21db844cdfe 100644 --- a/tests/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/ProjectSystem/VS/LanguageServices/WorkspaceTests.cs +++ b/tests/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/ProjectSystem/VS/LanguageServices/WorkspaceTests.cs @@ -85,11 +85,21 @@ public async Task Dispose_TriggersObjectDisposedExceptionsOnPublicMembers() await Assert.ThrowsAsync(() => workspace.WriteAsync(w => Task.CompletedTask, CancellationToken.None)); await Assert.ThrowsAsync(() => workspace.WriteAsync(w => TaskResult.EmptyString, CancellationToken.None)); - await Assert.ThrowsAsync(() => workspace.OnWorkspaceUpdateAsync(null!)); + //await Assert.ThrowsAsync(() => workspace.OnWorkspaceUpdateAsync(null!)); Assert.Throws(() => workspace.ChainDisposal(null!)); } + [Fact] + public async Task Dispose_DoesNotTriggerObjectDisposedExceptionsOnUpdate() + { + var workspace = await CreateInstanceAsync(); + + await workspace.DisposeAsync(); + + await workspace.OnWorkspaceUpdateAsync(null!); + } + [Theory] [CombinatorialData] public async Task WriteAsync_ThrowsIfNullAction(bool isGeneric)