Do not throw ObjectDisposedException in OnWorkspaceUpdateAsync #8892
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
MiYanni
approved these changes
Mar 3, 2023
| @@ -85,11 +85,21 @@ public async Task Dispose_TriggersObjectDisposedExceptionsOnPublicMembers() | |||
|
|
|||
| await Assert.ThrowsAsync<ObjectDisposedException>(() => workspace.WriteAsync(w => Task.CompletedTask, CancellationToken.None)); | |||
| await Assert.ThrowsAsync<ObjectDisposedException>(() => workspace.WriteAsync(w => TaskResult.EmptyString, CancellationToken.None)); | |||
| await Assert.ThrowsAsync<ObjectDisposedException>(() => workspace.OnWorkspaceUpdateAsync(null!)); | |||
| //await Assert.ThrowsAsync<ObjectDisposedException>(() => workspace.OnWorkspaceUpdateAsync(null!)); | |||
There was a problem hiding this comment.
nit: Is this commented code intended to remain since this solution may not be a complete fix?
There was a problem hiding this comment.
Oh, no; I meant to delete that.
|
#8895 wraps the whole update in a lock to prevent disposal during updates. |
|
Closing in favor of #8895. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
Workspacetype (which is responsible for passing data from a singleConfiguredProjectthrough to the Language Service) derives fromOnceInitializedOnceDisposedUnderLockAsync; 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 protectedExecuteUnderLockAsyncand passing in a delegate representing the work to be protected.There are a couple of potential problems, however. First,
Workspacedoes not protect all of its work; usually it does some amount of validation and processing and then callsExecuteUnderLockAsyncfor 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 callingVerify.NotDisposed(this); this throws anObjectDisposedExceptionif 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
ExecuteUnderLockAsyncwill 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
OnWorkspaceUpdateAsyncmethod 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 toVerify.NotDisposed(this). However, aWorkspaceis disposed by theLanguageServiceHostwhen 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 theWorkspacecould be disposed after the call toNotDisposed, or we could see calls toOnWorkspaceUpdateAsyncafter 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
OnWorkspaceUpdateAsyncwith the current input value.)Here we replace the call to
Verify.NotDisposed(this)with a simple check to see if theWorkspacehas already been disposed, or is in the process. In that case we can simply bail out early; since theWorkspacehas 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
Workspacewrap its work inExecuteUnderLockAsync. However that is a more invasive change so I'm hoping to avoid it.Microsoft Reviewers: Open in CodeFlow