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.

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

Properly dispose the SymbolSearchEngine #73464

Merged
merged 2 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ internal sealed class SymbolSearchUpdateNoOpEngine : ISymbolSearchUpdateEngine
{
public static readonly SymbolSearchUpdateNoOpEngine Instance = new();

public void Dispose()
{
// Nothing to do for the no-op version.
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: the actual Dispose we care about already exists, but is currently not called through any codepaths. It is here:

What thsi PR does is hook things up to ensure that it is called. To ensure that we actually dispose the RemoteServiceConnection instance we're holding onto.


public ValueTask<ImmutableArray<PackageWithAssemblyResult>> FindPackagesWithAssemblyAsync(string source, string assemblyName, CancellationToken cancellationToken)
=> ValueTaskFactory.FromResult(ImmutableArray<PackageWithAssemblyResult>.Empty);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ internal SymbolSearchUpdateEngine(
_reportAndSwallowExceptionUnlessCanceled = reportAndSwallowExceptionUnlessCanceled;
}

public void Dispose()
{
// Nothing to do for the core symbol search engine.
}

public ValueTask<ImmutableArray<PackageWithTypeResult>> FindPackagesWithTypeAsync(
string source, string name, int arity, CancellationToken cancellationToken)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,10 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Composition;
using System.IO;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
Expand All @@ -25,8 +22,8 @@
using Microsoft.VisualStudio.LanguageServices.Storage;
using Microsoft.VisualStudio.Settings;
using Microsoft.VisualStudio.Shell;
using Microsoft.VisualStudio.Shell.Interop;
using Microsoft.VisualStudio.Shell.Settings;
using Microsoft.VisualStudio.Threading;
using Roslyn.Utilities;
using VSShell = Microsoft.VisualStudio.Shell;

Expand All @@ -40,18 +37,18 @@ namespace Microsoft.VisualStudio.LanguageServices.SymbolSearch;
/// date by downloading patches on a daily basis.
/// </summary>
[ExportWorkspaceService(typeof(ISymbolSearchService), ServiceLayer.Host), Shared]
internal partial class VisualStudioSymbolSearchService : AbstractDelayStartedService, ISymbolSearchService
internal partial class VisualStudioSymbolSearchService : AbstractDelayStartedService, ISymbolSearchService, IDisposable
{
private readonly SemaphoreSlim _gate = new(initialCount: 1);

// Note: A remote engine is disposable as it maintains a connection with ServiceHub,
// but we want to keep it alive until the VS is closed, so we don't dispose it.
private ISymbolSearchUpdateEngine _lazyUpdateEngine;
private ISymbolSearchUpdateEngine? _lazyUpdateEngine;

private readonly SVsServiceProvider _serviceProvider;
private readonly IPackageInstallerService _installerService;

private string _localSettingsDirectory;
private string? _localSettingsDirectory;

[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
Expand All @@ -69,7 +66,32 @@ public VisualStudioSymbolSearchService(
[SymbolSearchOptionsStorage.SearchReferenceAssemblies, SymbolSearchOptionsStorage.SearchNuGetPackages])
{
_serviceProvider = serviceProvider;
_installerService = workspace.Services.GetService<IPackageInstallerService>();
_installerService = workspace.Services.GetRequiredService<IPackageInstallerService>();
}

public void Dispose()
{
// Once we're disposed, swap out our engine with a no-op one so we don't try to do any more work, and dispose of
// our connection to the OOP server so it can be cleaned up.
//
// Kick off a Task for this so we don't block MEF from proceeding (as it will be calling us on the UI thread).
_ = DisposeAsync();
return;

async Task DisposeAsync()
{
// Make sure we get off the UI thread so that Dispose can return immediately.
await TaskScheduler.Default;

ISymbolSearchUpdateEngine? updateEngine;
using (await _gate.DisposableWaitAsync().ConfigureAwait(false))
{
updateEngine = _lazyUpdateEngine;
_lazyUpdateEngine = SymbolSearchUpdateNoOpEngine.Instance;
}

updateEngine?.Dispose();
}
}

protected override async Task EnableServiceAsync(CancellationToken cancellationToken)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#nullable disable

using System;
using System.Collections.Immutable;
using System.Threading;
using System.Threading.Tasks;
Expand All @@ -14,7 +15,7 @@ namespace Microsoft.CodeAnalysis.SymbolSearch;
/// Service that allows you to query the SymbolSearch database and which keeps
/// the database up to date.
/// </summary>
internal interface ISymbolSearchUpdateEngine
internal interface ISymbolSearchUpdateEngine : IDisposable
{
ValueTask UpdateContinuouslyAsync(string sourceName, string localSettingsDirectory, CancellationToken cancellationToken);

Expand Down
Loading