Dispose per-instance services created by factories by sharwell · Pull Request #47951 · dotnet/roslyn

sharwell · 2020-09-22T18:23:48Z

This change makes MefWorkspaceServices and MefLanguageServices behave like MEF itself (specifically, its ExportProvider).

(Feature provided by MEF itself) Parts which are exported directly in the MEF catalog and happen to implement IDisposable are disposed when the ExportProvider is disposed. This includes workspace services and language services which are exported directly (as opposed be being exported through a factory)
(Feature provided by this pull request) Parts which are exported through a factory are owned by the workspace that invokes that factory. Any such parts that end up being created and happen to implement IDisposable are disposed automatically when the corresponding workspace is disposed.

The automation of this functionality is primarily valuable because we no longer need to keep track of whether or not a part was created in order to dispose it. Only parts that have already been created are disposed by the workspace.

genlu

The change makes sense to me. I'm curious what problem not disposing those Worksapce/Language services causes today.

sharwell · 2020-09-25T18:27:43Z

I'm curious what problem not disposing those Worksapce/Language services causes today.

It leads to direct coupling of Workspace.Dispose every time we need to handle it:

roslyn/src/Workspaces/Core/Portable/Workspace/Workspace.cs

Line 373 in f67cb8a

this.Services.GetService<IWorkspaceEventListenerService>()?.Stop();

roslyn/src/Workspaces/Core/Portable/Workspace/Workspace.cs

Lines 376 to 377 in f67cb8a

    
           (_optionService as IWorkspaceOptionService)?.OnWorkspaceDisposed(this); 
        
           _optionService.UnregisterWorkspace(this);

There are also two new services that need this support coming in separate pull requests.

CyrusNajmabadi · 2020-09-25T23:14:02Z

On this PR i'm going to defer to @jasonmalinowski . Primarily because i'm not a fan of IDisposable and/or making more parts of our system have that pattern spread through it. I find it a fairly high mental and design tax to take on that now makes it more difficult for all consumptive uses of these APIs. My preference would strongly be that we only do this as a last resort, and only if there aren't other suitable approaches that can avoid this.

HOwever, i'll defer to @jasonmalinowski here if he feels this is in line with standard patterns and practices for Roslyn, Mef and the greater .net ecosystem. If so, in balance, this is perhaps acceptable given that we would not be forging our own path here and we be consistent with a larger whole.

…rvices

…ices

sharwell · 2023-03-03T14:53:40Z

@jasonmalinowski @CyrusNajmabadi This is updated and ready to merge.

CyrusNajmabadi · 2023-03-03T16:38:52Z

I can look on Monday thanks.

sharwell · 2023-03-03T17:34:39Z

@CyrusNajmabadi note that there are no significant functional changes since the last review from you. Just FYI.

jasonmalinowski

This is a nice cleanup for a long-neglected area, so thanks for taking a look at it.

If I have one "concern", I hope we don't have cases where people "helpfully" see the services they might get back from GetService are disposable, and stick it in a using statement and now they're disposing objects underneath other people. Consider any of the disposable lifetime tracking analyzers that might be out there. It could be mitigated by instead introducing something like IDisposableWorkspaceServiceFactory that has a second method that takes the service object (and it's up to the implementer to under the covers call some explicit cleanup method), so that way the service objects themselves aren't disposable. But that's kinda icky though.

Maybe add a remark on GetService that if the object returned is disposable, don't dispose it?

jasonmalinowski · 2023-03-04T01:45:47Z

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Workspace/Mef/MefUtilities.cs

+{
+    internal static class MefUtilities
+    {
+        public static void DisposeWithExceptionTracking<T>(T instance, [NotNullIfNotNull("exceptions")] ref List<Exception>? exceptions)


The nullable annotation there on exceptions is intentional and not a tautology?

➡️ Without this attribute, the method would be allowed to reassign null to a non-null storage location.

jasonmalinowski · 2023-03-04T01:51:54Z

src/Workspaces/Core/Portable/Workspace/Host/HostWorkspaceServices.cs

+    ///   <strong><see cref="IWorkspaceServiceFactory"/> instance constructed externally (e.g. MEF):</strong> Owned by
+    ///   the external source, and will not be automatically disposed when <see cref="Workspace"/> is disposed.


This line confused me a bit, since the IWorkspaceServiceFactory is "definitely" not owned by the workspace, i.e. this bullet might cause more confusion than good.

⏱️ I'll see if I can clarify that the only option for IWorkspaceServiceFactory is that it's owned outside the workspace.

jasonmalinowski · 2023-03-04T01:54:07Z

src/Workspaces/Core/Portable/Workspace/Host/HostWorkspaceServices.cs

+    /// <para>Workspace services which implement <see cref="IDisposable"/> are considered ownable, in which case the
+    /// owner is responsible for disposing of owned instances when they are no longer in use. When
+    /// <see cref="IWorkspaceService"/> or <see cref="IWorkspaceServiceFactory"/> instances are provided directly to the
+    /// <see cref="HostWorkspaceServices"/>, the owner of the instances is the type or container (e.g. a MEF export
+    /// provider) which created the instances. For the specific case of ownable workspace services created by a factory
+    /// (i.e. instances returned by <see cref="IWorkspaceServiceFactory.CreateService"/>), the <see cref="Workspace"/>
+    /// is considered the owner of the resulting instance and is expected to be disposed during the call to
+    /// <see cref="Dispose"/>.</para>
+    ///
+    /// <para><strong>Summary of lifetime rules</strong></para>
+    ///
+    /// <list type="bullet">
+    /// <item><description>
+    ///   <strong><see cref="IWorkspaceService"/> instance constructed externally (e.g. MEF):</strong> Owned by the
+    ///   external source, and will not be automatically disposed when <see cref="Workspace"/> is disposed.
+    /// </description></item>
+    /// <item><description>
+    ///   <strong><see cref="IWorkspaceServiceFactory"/> instance constructed externally (e.g. MEF):</strong> Owned by
+    ///   the external source, and will not be automatically disposed when <see cref="Workspace"/> is disposed.
+    /// </description></item>
+    /// <item><description>
+    ///   <strong><see cref="IWorkspaceService"/> instance constructed by <see cref="IWorkspaceServiceFactory"/> within
+    ///   the context of <see cref="HostWorkspaceServices"/>:</strong> Owned by <see cref="Workspace"/>, and
+    ///   <strong>will</strong> be automatically disposed when <see cref="Workspace"/> is disposed.
+    /// </description></item>
+    /// </list>
+    /// </remarks>


It feels like this can really be summarized as:

Since this object created them, this object owns disposable IWorkspaceServices that are created from IWorkspaceFactoryServices, and will dispose them.

Workspace services not created from factories and the factories themselves are not disposed.

jasonmalinowski · 2023-03-04T01:56:25Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

-            // Directly dispose IRemoteHostClientProvider if necessary. This is a test hook to ensure RemoteWorkspace
-            // gets disposed in unit tests as soon as TestWorkspace gets disposed. This would be superseded by direct
-            // support for IDisposable in https://github.com/dotnet/roslyn/pull/47951.
-            if (Services.GetService<IRemoteHostClientProvider>() is IDisposable disposableService)


I love the implication here that we'll potentially create the service only to immediately dispose it if it hadn't already been created.

➡️ Yes, and I tried to call out the benefits of tracking object creation in the top-level pull request comment as well.

jasonmalinowski · 2023-03-04T02:00:10Z

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Workspace/Mef/MefLanguageServices.cs

+            List<Exception> exceptions = null;
+            foreach (var service in disposableServices)
+            {
+                MefUtilities.DisposeWithExceptionTracking(service, ref exceptions);
+            }
+
+            if (exceptions is not null)
+            {
+                throw new AggregateException(CompilerExtensionsResources.Instantiated_parts_threw_exceptions_from_IDisposable_Dispose, exceptions);
+            }


Consider refactoring MefUtilities.DisposeWithExceptionTracking to just do this entire pattern of looping and throwing, since we'll need the same thing in the other place too.

jasonmalinowski · 2023-03-04T02:11:48Z

...Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Workspace/Mef/MefWorkspaceServices.cs

+                // MEF workspace service instances created by a factory are not owned by the MEF catalog or disposed
+                // when the MEF catalog is disposed. Whenever we are potentially going to create an instance of a
+                // service provided by a factory, we need to check if the resulting service implements IDisposable. The
+                // specific conditions here are:
+                //
+                // * usesFactory: This is true when the workspace service is provided by a factory. Services provided
+                //   directly are owned by the MEF catalog so they do not need to be tracked by the workspace.
+                // * IsValueCreated: This will be false at least once prior to accessing the lazy value. Once the value
+                //   is known to be created, we no longer need to try adding it to _ownedDisposableServices, so we use a
+                //   lock-free fast path.
+                var checkAddDisposable = service.usesFactory && !service.lazyService.IsValueCreated;
+
+                var serviceInstance = (TWorkspaceService)service.lazyService.Value;
+                if (checkAddDisposable && serviceInstance is IDisposable disposable)
+                {
+                    lock (_gate)
+                    {
+                        _ownedDisposableServices.Add(disposable);
+                    }
+                }


Consider extracting and putting this into MefUtilities since it's the same as the other use.

(although I guess it's a strange to pass the lock out...)

jasonmalinowski · 2023-03-04T02:13:36Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

@@ -523,13 +522,9 @@ protected virtual void Dispose(bool finalize)



GitHub won't let me leave a comment the line above this, but an easy follow up to this PR is the this.Services.GetService()?.Stop(); line and making that also be disposable based.

➡️ This is already part of follow-up #48068.

sharwell · 2023-03-05T03:59:10Z

If I have one "concern", I hope we don't have cases where people "helpfully" see the services they might get back from GetService are disposable, and stick it in a using statement and now they're disposing objects underneath other people. Consider any of the disposable lifetime tracking analyzers that might be out there. It could be mitigated by instead introducing something like IDisposableWorkspaceServiceFactory that has a second method that takes the service object (and it's up to the implementer to under the covers call some explicit cleanup method), so that way the service objects themselves aren't disposable. But that's kinda icky though.

Typically workspace services are defined with an interface, and exported with an implementation. For disposable service instances, the interface would not inherit IDisposable, but the implementation would. These patterns match the long-standing patterns used by MEF without alteration.

…ices

sharwell added 3 commits September 25, 2020 07:09

Dispose per-instance services created by factories

af5bce5

Dispose per-instance language services created by factories

1bca6dd

Document lifetime rules for disposable services

dab1986

sharwell changed the title ~~Dispose per-instance language services created by factories~~ Dispose per-instance services created by factories Sep 25, 2020

sharwell force-pushed the dispose-language-services branch from 22db5a3 to dab1986 Compare September 25, 2020 14:48

sharwell marked this pull request as ready for review September 25, 2020 16:42

sharwell requested a review from a team as a code owner September 25, 2020 16:42

genlu approved these changes Sep 25, 2020

View reviewed changes

jinujoseph added the Area-IDE label Sep 25, 2020

This was referenced Sep 25, 2020

Decouple Workspace.Dispose #48068

Draft

Fix failure to dispose RemoteWorkspace in tests #48069

Closed

sharwell mentioned this pull request Sep 26, 2020

Fix failure to dispose RemoteWorkspace in tests #48077

Merged

Merge remote-tracking branch 'dotnet/master' into dispose-language-se…

0424e43

…rvices

This was referenced Sep 28, 2020

Test failures via timeline in Roslyn jaredpar/runfo#37

Closed

Test failures via timeline in Roslyn jaredpar/runfo#38

Closed

Base automatically changed from master to main March 3, 2021 23:52

sharwell mentioned this pull request Sep 8, 2021

Workspace disposal during MEF shutdown may try to create more stuff and throw exceptions #56177

Closed

sharwell added 2 commits September 10, 2021 09:07

Merge remote-tracking branch 'dotnet/main' into dispose-language-serv…

e5e7000

…ices

Merge remote-tracking branch 'dotnet/main' into dispose-language-serv…

cf7f748

…ices

sharwell requested a review from a team as a code owner May 16, 2022 23:30

Merge remote-tracking branch 'dotnet/main' into dispose-language-serv…

be3d184

…ices

jasonmalinowski approved these changes Mar 4, 2023

View reviewed changes

Merge remote-tracking branch 'dotnet/main' into dispose-language-serv…

c621992

…ices

dotnet deleted a comment from azure-pipelines bot Sep 12, 2023

dotnet deleted a comment from azure-pipelines bot Oct 12, 2023

dotnet deleted a comment from azure-pipelines bot Oct 23, 2023

Merge remote-tracking branch 'dotnet/main' into dispose-language-serv…

607dab5

…ices

sharwell merged commit 33e4c03 into dotnet:main Jul 19, 2024

sharwell deleted the dispose-language-services branch July 19, 2024 14:49

dotnet-policy-service bot added this to the Next milestone Jul 19, 2024

This was referenced Jul 23, 2024

[Automated] PRs inserted in VS build main-35122.268 #74496

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35124.136 #74546

Closed

RikkiGibson modified the milestones: Next, 17.12 P1 Jul 29, 2024

dotnet-bot mentioned this pull request Aug 2, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35201.55 #74635

Closed

		/// <strong><see cref="IWorkspaceServiceFactory"/> instance constructed externally (e.g. MEF):</strong> Owned by
		/// the external source, and will not be automatically disposed when <see cref="Workspace"/> is disposed.

		@@ -523,13 +522,9 @@ protected virtual void Dispose(bool finalize)

Conversation

sharwell commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

genlu left a comment

Choose a reason for hiding this comment

Uh oh!

sharwell commented Sep 25, 2020

Uh oh!

CyrusNajmabadi commented Sep 25, 2020

Uh oh!

sharwell commented Mar 3, 2023

Uh oh!

CyrusNajmabadi commented Mar 3, 2023

Uh oh!

sharwell commented Mar 3, 2023

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Mar 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Mar 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sharwell commented Sep 22, 2020 •

edited

Loading

sharwell Mar 5, 2023 •

edited

Loading

sharwell Mar 5, 2023 •

edited

Loading

jasonmalinowski Mar 4, 2023 •

edited

Loading

sharwell Mar 5, 2023 •

edited

Loading