Language service tidyup #8550

drewnoakes · 2022-09-29T03:59:28Z

I recommend reviewing this one commit at a time.

Key changes:

Remove duplicate validation and JTC.Join calls
Ensure validation occurs within joined block to prevent deadlocks
Remove a potentially redundant UI thread switch
Add some API docs
Give a dataflow action block a name to make debugging easier

Microsoft Reviewers: Open in CodeFlow

GetPrimaryWorkspace already performs this validation. We don't need to do it twice.

Determining whether we are enabled or not may involve a switch to the UI thread.

WhenInitialized already joins dataflow work. No need to do this twice.

If `IVsShellServices` has already been initialised, we won't need the UI thread. Avoid the switch before calling it.

davkean · 2022-09-29T07:01:36Z

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs

        using (_joinableTaskCollection.Join())
        {
+            await ValidateEnabledAsync(token);


This commit description for this change does not look correct. Join is about connecting the JoinableTask created here with the caller of this method. Anyone synchronously blocking on the UI thread on this method will happily let ValidateEnabledAsync switch to the UI thread if they are following JTF rules (ie calling JTF.Run).

ie Join solely exists because there's not a traditional async connection between _firstPrimaryWorkspaceSet and that code I pointed to.

So, this ends up calling LanguageServiceHostEnvironment.IsEnabledAsync that no longer needs the UI thread.
Also, the information whether it is running under command line is known upfront, the implementation could be further simplified and eliminate async - by getting this information using ICriticalPackageService - see IVsShellServices.IsInCommandLineMode

moving ValidateEnabledAsync inside the collection join is unnecessary, because the AsyncLazy has its own JTF chain managed inside. it would not break things either.

This commit description for this change does not look correct. Join is about connecting the JoinableTask created here with the caller of this method. Anyone synchronously blocking on the UI thread on this method will happily let ValidateEnabledAsync switch to the UI thread if they are following JTF rules (ie calling JTF.Run).

This JTC has all upstream data sources joined to it. The Join here allows dataflow to access the main thread, even if a caller of this method is on the main thread.

the information whether it is running under command line is known upfront, the implementation could be further simplified and eliminate async - by getting this information using ICriticalPackageService

ICriticalPackageService is not available outside of CPS. I don't know a better way to initialize these variables early enough during solution load to ensure they're populated before we query them.

moving ValidateEnabledAsync inside the collection join is unnecessary, because the AsyncLazy has its own JTF chain managed inside. it would not break things either.

If the join here was removed and the caller was on the main thread, and they trigger the lazy initialisation of the "is command line mode" state, which also require the main thread, but dataflow is running and is blocking the main thread, wouldn't it be able to deadlock?

ICriticalPackageService is not available outside of CPS. I don't know a better way to initialize these variables early enough during solution load to ensure they're populated before we query them.

Oh, I didn't realize that ICriticalPackageService is not public.
Another option would be to expose IVsShellServices.IsInCommandLineMode from CPS as public but that needs some additional thinking.

If the join here was removed and the caller was on the main thread, and they trigger the lazy initialisation of the "is command line mode" state, which also require the main thread, but dataflow is running and is blocking the main thread, wouldn't it be able to deadlock?

Join says "let any tasks added to the JoinableTaskCollection" access the main thread if the caller is blocking it. ValidateEnabledAsync does not add any tasks to the collection, and therefore putting it in the join has no affect. It actually lets dataflow steal the UI thread before the IsCommandLineMode check.

ValidateEnabledAsync does not add any tasks to the collection

I thought tasks in the same collection are implicitly joined. The goal of having ValidateEnabledAsync inside the Join here is so that when it creates its own JoinableTask as part of the AsyncLazy (which uses the default JTF via MEF) that the JoinableTask instances associated with separate collections are considered as joined for the duration of that scope.

Awaiting an AsyncLazy implicitly joins the caller's context with the Func<Task> its executing, so if it needs UI thread, it will get it. All in all, there's probably no real side affect of doing what you are doing (other than allowing dataflow to access UI thread earlier), but its unneeded and the lack of doing this will not cause a hang.

adrianvmsft · 2022-09-29T17:50:50Z

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs

        using (_joinableTaskCollection.Join())
        {
+            await ValidateEnabledAsync(token);


So, this ends up calling LanguageServiceHostEnvironment.IsEnabledAsync that no longer needs the UI thread.
Also, the information whether it is running under command line is known upfront, the implementation could be further simplified and eliminate async - by getting this information using ICriticalPackageService - see IVsShellServices.IsInCommandLineMode

lifengl · 2022-09-29T21:14:49Z

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs

@@ -132,7 +132,8 @@ protected override async Task InitializeCoreAsync(CancellationToken cancellation
                target: DataflowBlockFactory.CreateActionBlock<IProjectVersionedValue<(ConfiguredProject ActiveConfiguredProject, ConfigurationSubscriptionSources Sources)>>(
                    async update => await ExecuteUnderLockAsync(cancellationToken => OnSlicesChanged(update, cancellationToken)),


we can remove both async/await here to reduce extra async state machines.

also, I wonder whether you still need ExecuteUnderLockAsync here, because action block never processes two pieces of input at the same time, and the disposing code is to dispose links, and doesn't have any race condition between the two. The async semaphore is essentially another queue to add more overhead to this.

BTW, if we do have other code path to call this ExecuteUnderLockAsync, to follow the JTF rule, you must call this code inside a JTF.RunAsync:

async update => await _joinableTaskFactory.RunAsync(() => ExecuteUnderLockAsync(...)),

missing this JTF.RunAsync means that we will not join correctly for the async semaphore JTF chain, which can lead into deadlocks. Of course, this will add extra overhead. So I wonder this semaphore is completely unnecssary.

The code in Workspace.cs looks good. I am not sure why we need retain the ExecuteUnderLock/AsyncSemaphore there as well. I cannot find any usage of WriteAsync methods. Those seem to be a part of old design, and can be removed?

BTW, we should consider chaining the fault completion of the action block to abort the task completion source of _firstPrimaryWorkspaceSet. Basically, if we run into any exception inside OnSliceChanged, it will send NFE, but we don't want to hang the solution loading. It just adds a safe net for this code.

I wonder whether you still need ExecuteUnderLockAsync here

Thanks, indeed ExecuteUnderLockAsync here seems redundant. Removed in b63e443.

The code in Workspace.cs looks good. I am not sure why we need retain the ExecuteUnderLock/AsyncSemaphore there as well. I cannot find any usage of WriteAsync methods. Those seem to be a part of old design, and can be removed?

They're used via IWorkspaceWriter.WriteAsync in TempPE, error list, properties providers and code model code. So we cannot remove them, and we must keep the ExecuteUnderLockAsync calls to prevent callers of WriteAsync overlapping with our own updates.

Does your advice around _jtf.RunAsync(() => ExecuteUnderLockAsync(... apply in Workspace.cs too then?

Moved the removal of locking in LanguageServiceHost to a separate PR: #8618

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs

lifengl · 2022-09-29T21:55:16Z

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs

@@ -265,10 +266,10 @@ public Task<bool> IsEnabledAsync(CancellationToken cancellationToken)

    public async Task WhenInitialized(CancellationToken token)
    {
-        await ValidateEnabledAsync(token);
-
        using (_joinableTaskCollection.Join())


i still have questions around JTF chains in this code. One thing interesting is that we create joinableTaskCollection/Factory here instead of reusing ones created in its base class (OnceInitializeOnceDisposeAsync exposes them through protect members, so joins the collection will join initialization tasks as well (of course, it doesn't matter much, because we ensure the initialization in the LoadAsync method.

this code doesn't seem to JoinUpstream for two blocks linked here including _activeConfiguredProjectProvider and _activeConfigurationGroupSubscriptionService. They should be covered.

this code doesn't seem to JoinUpstream for two blocks linked here including _activeConfiguredProjectProvider and _activeConfigurationGroupSubscriptionService. They should be covered.

Thanks. Fixed in a4f2588. Note that I'm joining to my custom JTC/JTF rather than OnceInitializedOnceDisposedAsync.JoinableFactory from the base. We use the custom JTC to join all dataflow work here with callers to IWorkspaceWriter.WriteAsync.

i still have questions around JTF chains in this code. One thing interesting is that we create joinableTaskCollection/Factory here instead of reusing ones created in its base class

When you and I looked at a hang here a while back, I thought we concluded that we'd need our own JTC to sync all this work. I assume the one in OnceInitializedOnceDisposedAsync.JoinableFactory uses a global collection.

OnceInitializedOnceDisposedAsync.JoinableFactory uses its own collection - which is exposed as a property. I think we should use the existing collection and JTF from the base class instead of creating another set and leave those almost unused.

The action block serialises updates, so they won't overlap. There's no race condition with the dispose method either, as the dispose logic just tears down the dataflow links.

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs

MiYanni · 2023-02-17T22:15:52Z

@drewnoakes Howdy. Just checking the status on this PR.

drewnoakes added 8 commits September 29, 2022 11:08

Give action block a name to assist debugging

651e59c

Add API docs to Workspace

1b226ad

Remove duplicate validation

67565b5

GetPrimaryWorkspace already performs this validation. We don't need to do it twice.

Join dataflow when waiting for project load

88a853b

Join dataflow work during validation

3c94e95

Determining whether we are enabled or not may involve a switch to the UI thread.

Remove duplicate join

709f5e2

WhenInitialized already joins dataflow work. No need to do this twice.

Convert method to local function

35f3965

Remove potentially redundant UI switch

3e8ae2f

If `IVsShellServices` has already been initialised, we won't need the UI thread. Avoid the switch before calling it.

drewnoakes added the Feature-Language-Service Populating the Roslyn workspace with references, source files, analyzers, etc label Sep 29, 2022

drewnoakes requested a review from a team as a code owner September 29, 2022 03:59

davkean reviewed Sep 29, 2022

View reviewed changes

adrianvmsft approved these changes Sep 29, 2022

View reviewed changes

lifengl reviewed Sep 29, 2022

View reviewed changes

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs Show resolved Hide resolved

lifengl reviewed Sep 29, 2022

View reviewed changes

drewnoakes added 3 commits September 30, 2022 09:41

Don't lock for dataflow updates

b63e443

The action block serialises updates, so they won't overlap. There's no race condition with the dispose method either, as the dispose logic just tears down the dataflow links.

Ensure we cancel on project unload

7273840

Join upstream data sources

a4f2588

MiYanni reviewed Sep 30, 2022

View reviewed changes

...oft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs Show resolved Hide resolved

This was referenced Oct 18, 2022

Add dataflow block name and some API docs #8617

Merged

Don't lock for dataflow updates #8618

Merged

drewnoakes marked this pull request as draft October 31, 2022 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Language service tidyup #8550

Language service tidyup #8550

drewnoakes commented Sep 29, 2022 •

edited by ghost

davkean Sep 29, 2022

davkean Sep 29, 2022

adrianvmsft Sep 29, 2022

lifengl Sep 29, 2022

drewnoakes Sep 29, 2022

adrianvmsft Sep 29, 2022

davkean Sep 29, 2022 •

edited

drewnoakes Sep 30, 2022

davkean Sep 30, 2022 •

edited

adrianvmsft Sep 29, 2022

lifengl Sep 29, 2022

lifengl Sep 29, 2022 •

edited

lifengl Sep 29, 2022

lifengl Sep 29, 2022

drewnoakes Sep 29, 2022

drewnoakes Oct 18, 2022

lifengl Sep 29, 2022

drewnoakes Sep 29, 2022 •

edited

adrianvmsft Sep 30, 2022 •

edited

MiYanni commented Feb 17, 2023

		@@ -132,7 +132,8 @@ protected override async Task InitializeCoreAsync(CancellationToken cancellation
		target: DataflowBlockFactory.CreateActionBlock<IProjectVersionedValue<(ConfiguredProject ActiveConfiguredProject, ConfigurationSubscriptionSources Sources)>>(
		async update => await ExecuteUnderLockAsync(cancellationToken => OnSlicesChanged(update, cancellationToken)),

Language service tidyup #8550

Are you sure you want to change the base?

Language service tidyup #8550

Conversation

drewnoakes commented Sep 29, 2022 • edited by ghost

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davkean Sep 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davkean Sep 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifengl Sep 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewnoakes Sep 29, 2022 • edited

Choose a reason for hiding this comment

adrianvmsft Sep 30, 2022 • edited

Choose a reason for hiding this comment

MiYanni commented Feb 17, 2023

drewnoakes commented Sep 29, 2022 •

edited by ghost

davkean Sep 29, 2022 •

edited

davkean Sep 30, 2022 •

edited

lifengl Sep 29, 2022 •

edited

drewnoakes Sep 29, 2022 •

edited

adrianvmsft Sep 30, 2022 •

edited