Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Language service tidyup #8550

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Language service tidyup #8550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
651e59c
Give action block a name to assist debugging
drewnoakes Sep 29, 2022
1b226ad
Add API docs to Workspace
drewnoakes Sep 29, 2022
67565b5
Remove duplicate validation
drewnoakes Sep 29, 2022
88a853b
Join dataflow when waiting for project load
drewnoakes Sep 29, 2022
3c94e95
Join dataflow work during validation
drewnoakes Sep 29, 2022
709f5e2
Remove duplicate join
drewnoakes Sep 29, 2022
35f3965
Convert method to local function
drewnoakes Sep 29, 2022
3e8ae2f
Remove potentially redundant UI switch
drewnoakes Sep 29, 2022
b63e443
Don't lock for dataflow updates
drewnoakes Sep 29, 2022
7273840
Ensure we cancel on project unload
drewnoakes Sep 29, 2022
a4f2588
Join upstream data sources
drewnoakes Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions ...jectSystem.Managed.VS/ProjectSystem/VS/LanguageServices/LanguageServiceHostEnvironment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ public LanguageServiceHostEnvironment(IVsShellServices vsShell, JoinableTaskCont
_isEnabled = new(
async () =>
{
await joinableTaskContext.Factory.SwitchToMainThreadAsync();

// If VS is running in command line mode (e.g. "devenv.exe /build my.sln"),
// the language service host is not enabled. The one exception to this is
// when we're populating a solution cache via "/populateSolutionCache".
Expand Down
56 changes: 29 additions & 27 deletions ....VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/LanguageServiceHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ namespace Microsoft.VisualStudio.ProjectSystem.LanguageServices;
[Export(typeof(IWorkspaceWriter))]
[Export(ExportContractNames.Scopes.UnconfiguredProject, typeof(IProjectDynamicLoadComponent))]
[AppliesTo(ProjectCapability.DotNetLanguageService)]
internal sealed class LanguageServiceHost : OnceInitializedOnceDisposedUnderLockAsync, IProjectDynamicLoadComponent, IWorkspaceWriter
internal sealed class LanguageServiceHost : OnceInitializedOnceDisposedAsync, IProjectDynamicLoadComponent, IWorkspaceWriter
{
private readonly TaskCompletionSource _firstPrimaryWorkspaceSet = new();

Expand Down Expand Up @@ -106,6 +106,9 @@ protected override async Task InitializeCoreAsync(CancellationToken cancellation
return;
}

// Ensure we also cancel on project unload
cancellationToken = _tasksService.LinkUnload(cancellationToken);

// We have one "workspace" per "slice".
//
// - A "workspace" models the project state that Roslyn needs for a specific configuration.
Expand All @@ -130,12 +133,15 @@ protected override async Task InitializeCoreAsync(CancellationToken cancellation
// We track per-slice data via this source.
_activeConfigurationGroupSubscriptionService.SourceBlock.SyncLinkOptions(),
target: DataflowBlockFactory.CreateActionBlock<IProjectVersionedValue<(ConfiguredProject ActiveConfiguredProject, ConfigurationSubscriptionSources Sources)>>(
async update => await ExecuteUnderLockAsync(cancellationToken => OnSlicesChanged(update, cancellationToken)),
update => OnSlicesChanged(update, cancellationToken),
_unconfiguredProject,
ProjectFaultSeverity.LimitedFunctionality),
ProjectFaultSeverity.LimitedFunctionality,
"LanguageServiceHostSlices {0}"),
linkOptions: DataflowOption.PropagateCompletion,
cancellationToken: cancellationToken),

ProjectDataSources.JoinUpstreamDataSources(_joinableTaskFactory, _projectFaultHandler, _activeConfiguredProjectProvider, _activeConfigurationGroupSubscriptionService),

new DisposableDelegate(() =>
{
// Dispose all workspaces. Note that this happens within a lock, so we will not race with project updates.
Expand Down Expand Up @@ -265,10 +271,10 @@ public Task<bool> IsEnabledAsync(CancellationToken cancellationToken)

public async Task WhenInitialized(CancellationToken token)
{
await ValidateEnabledAsync(token);

using (_joinableTaskCollection.Join())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@drewnoakes drewnoakes Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@adrianvmsft adrianvmsft Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
await ValidateEnabledAsync(token);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@davkean davkean Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@davkean davkean Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


await _firstPrimaryWorkspaceSet.Task.WithCancellation(token);
}
}
Expand All @@ -277,8 +283,6 @@ public async Task WriteAsync(Func<IWorkspace, Task> action, CancellationToken to
{
token = _tasksService.LinkUnload(token);

await ValidateEnabledAsync(token);

Workspace workspace = await GetPrimaryWorkspaceAsync(token);

await workspace.WriteAsync(action, token);
Expand All @@ -288,32 +292,33 @@ public async Task<T> WriteAsync<T>(Func<IWorkspace, Task<T>> action, Cancellatio
{
token = _tasksService.LinkUnload(token);

await ValidateEnabledAsync(token);

Workspace workspace = await GetPrimaryWorkspaceAsync(token);

return await workspace.WriteAsync(action, token);
}

private async Task<Workspace> GetPrimaryWorkspaceAsync(CancellationToken cancellationToken)
{
await ValidateEnabledAsync(cancellationToken);
using (_joinableTaskCollection.Join())
{
await ValidateEnabledAsync(cancellationToken);

await WhenProjectLoaded(cancellationToken);
await WhenProjectLoaded();
}

return _primaryWorkspace ?? throw Assumes.Fail("Primary workspace unknown.");
}

private async Task WhenProjectLoaded(CancellationToken cancellationToken)
{
// The active configuration can change multiple times during initialization in cases where we've incorrectly
// guessed the configuration via our IProjectConfigurationDimensionsProvider3 implementation.
// Wait until that has been determined before we publish the wrong configuration.
await _tasksService.PrioritizedProjectLoadedInHost.WithCancellation(cancellationToken);

// We block project load on initialization of the primary workspace.
// Therefore by this point we must have set the primary workspace.
System.Diagnostics.Debug.Assert(_firstPrimaryWorkspaceSet.Task is { IsCompleted: true, IsFaulted: false });
async Task WhenProjectLoaded()
{
// The active configuration can change multiple times during initialization in cases where we've incorrectly
// guessed the configuration via our IProjectConfigurationDimensionsProvider3 implementation.
// Wait until that has been determined before we publish the wrong configuration.
await _tasksService.PrioritizedProjectLoadedInHost.WithCancellation(cancellationToken);

// We block project load on initialization of the primary workspace.
// Therefore by this point we must have set the primary workspace.
System.Diagnostics.Debug.Assert(_firstPrimaryWorkspaceSet.Task is { IsCompleted: true, IsFaulted: false });
drewnoakes marked this conversation as resolved.
Show resolved Hide resolved
}
}

#endregion
Expand All @@ -333,18 +338,15 @@ public async Task AfterLoadInitialConfigurationAsync()
// Ensure the project is not considered loaded until our first publication.
Task result = _tasksService.PrioritizedProjectLoadedInHostAsync(async () =>
{
using (_joinableTaskCollection.Join())
{
await WhenInitialized(_tasksService.UnloadCancellationToken);
}
await WhenInitialized(_tasksService.UnloadCancellationToken);
drewnoakes marked this conversation as resolved.
Show resolved Hide resolved
});

// While we want make sure it's loaded before PrioritizedProjectLoadedInHost,
// we don't want to block project factory completion on its load, so fire and forget.
_projectFaultHandler.Forget(result, _unconfiguredProject, ProjectFaultSeverity.LimitedFunctionality);
}

protected override Task DisposeCoreUnderLockAsync(bool initialized)
protected override Task DisposeCoreAsync(bool initialized)
{
_firstPrimaryWorkspaceSet.TrySetCanceled();

Expand Down
21 changes: 21 additions & 0 deletions src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/Workspace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,34 @@ protected override Task DisposeCoreUnderLockAsync(bool initialized)
return Task.CompletedTask;
}

/// <summary>
/// Adds an object that will be disposed along with this <see cref="Workspace"/> instance.
/// </summary>
/// <param name="disposable">The object to dispose when this object is disposed.</param>
public void ChainDisposal(IDisposable disposable)
{
Verify.NotDisposed(this);

_disposableBag.Add(disposable);
}

/// <summary>
/// Integrates project updates into the workspace.
/// </summary>
/// <remarks>
/// <para>
/// This method must always receive an evaluation update first. After that point,
/// both evaluation and build updates may arrive in any order, so long as values
/// of each type are ordered correctly.
/// </para>
/// <para>
/// Calls must not overlap. This method is not thread-safe. This method is designed
/// to be called from a dataflow ActionBlock, which will serialize calls, so we
/// needn't perform any locking or protection here.
/// </para>
/// </remarks>
/// <param name="update">The project update to integrate.</param>
/// <returns>A task that completes when the update has been integrated.</returns>
internal async Task OnWorkspaceUpdateAsync(IProjectVersionedValue<WorkspaceUpdate> update)
{
Verify.NotDisposed(this);
Expand Down