[WIP] OwnedDisposable<T> #26321

sharwell · 2018-04-22T03:34:04Z

Implement transferable disposables with OwnedDisposable<T>
Apply OwnedDisposable<T> to RemoteHostClient.Connection

The types and patterns provided in this change work towards a design goal of addressing #25880 (comment) in a world where the following hold:

The IDisposable analyzers are enabled with severity Error
Warnings for the analyzers must not be suppressed

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

sharwell

@mavasani This change resolves all CA2000/CA2013 warnings related to Connection instances without requiring any suppressions. Additional notes provided in the diff.

sharwell · 2018-04-22T03:36:31Z

src/VisualStudio/Core/Def/Implementation/Remote/ServiceHubRemoteHostClient.ConnectionManager.cs

+                }
+                finally
+                {
+                    newConnection.Dispose();


💭 @mavasani Commenting out this line doesn't seem to trigger CA2000/CA2013, but I'm not sure why.

sharwell · 2018-04-22T03:38:29Z

src/VisualStudio/Core/Def/Implementation/Remote/ServiceHubRemoteHostClient.ConnectionManager.cs

-                    return new PooledConnection(this, serviceName, connection);
+                    try
+                    {
+                        return new OwnedDisposable<Connection>(() => new PooledConnection(this, serviceName, ref connection));


💭 @mavasani The factory function avoids the need to transfer an object into this constructor, but it comes with an allocation cost.

having a 'ref' inside a lambda always gives me the heebie jeebies FWIW.

sharwell · 2018-04-22T03:41:06Z

src/Workspaces/Core/Portable/Remote/RemoteHostSessionHelpers.cs

                    WellKnownServiceHubServices.ServiceHubServiceBase_Initialize,
                    new object[] { scope.SolutionInfo },
                    cancellationToken).ConfigureAwait(false);

-                return new SessionWithSolution(connection, scope);
+                return new SessionWithSolution(ref connection.Resource, scope);


📝 The use of a wrapper allows this function to avoid "taking ownership" of connection in error cases. The ownership doesn't transfer prior to this line, which means this method is no longer responsible for releasing the resource on error paths.

shouldn't there be Move somewhere to move ownership?

shouldn't there be Move somewhere to move ownership?

Yes, see #26321 (comment)

sharwell · 2018-04-22T03:41:58Z

src/Workspaces/Core/Portable/Remote/RemoteHostSessionHelpers.cs

        private readonly PinnedRemotableDataScope _scope;

-        public static async Task<SessionWithSolution> CreateAsync(RemoteHostClient.Connection connection, Solution solution, CancellationToken cancellationToken)
+        public static async Task<SessionWithSolution> CreateAsync(OwnedDisposable<RemoteHostClient.Connection>.Boxed connection, Solution solution, CancellationToken cancellationToken)


📝 OwnedDisposable<Connection>.Boxed is functionally equivalent to ref OwnedDisposable<Connection>, but the latter cannot be passed as an argument to an async method.

I see. so it is to be used when ref can't be used.

sharwell · 2018-04-22T03:43:48Z

src/Workspaces/Core/Portable/Utilities/OwnedDisposable`1.cs

+
+namespace Roslyn.Utilities
+{
+    internal struct OwnedDisposable<T> : IDisposable, IEquatable<OwnedDisposable<T>>


📝 This type would need analyzer support to ensure it's treated as non-copyable. It can either be passed by reference or moved to a different instance. For cases where it's used but will not transfer ownership, in can be used.

some questions. why it is struct instead of class? is that to force having ref?

if it require special analyzer support to detect, how about creating well known interface such as IOwnerTrackingDisposable or something, that analyzer knows about, and has well known operation such as Claims, Move and etc below so that analyzer can track ownership more clearly and detect mis-usage such as assigning disposable object to another without transferring ownership?

is the point having wrapper type that can always disposed within same method?

why it is struct instead of class?

Allocation-free fast path. The type should not incur a performance penalty.

if it require special analyzer support to detect, how about creating well known interface such as IOwnerTrackingDisposable or something, that analyzer knows about, and has well known operation such as Claims, Move and etc below so that analyzer can track ownership more clearly and detect mis-usage such as assigning disposable object to another without transferring ownership?

This is intended to be that interface. The use of struct vs interface for it goes back to the previous item.

is the point having wrapper type that can always disposed within same method?

Yes, this is a requirement of the current IDisposable analyzer design.

sharwell · 2018-04-22T03:45:06Z

src/Workspaces/Core/Portable/Utilities/OwnedDisposable`1.cs

+                _resource = resource.Move();
+            }
+
+            public ref OwnedDisposable<T> Resource => ref _resource;


💭 My understanding is you could unbox the value by using boxed.Resource.Move().

sharwell · 2018-04-22T03:46:57Z

src/Workspaces/Core/Portable/Remote/RemoteHostSessionHelpers.cs

@@ -273,12 +270,19 @@ public void Shutdown()
            }

            var connection = await client.TryCreateConnectionAsync(_serviceName, _callbackTarget, cancellationToken).ConfigureAwait(false);
-            if (connection == null)
+            try


📝 Ideally you would just place the previous line in a using statement, but this will not work due to CS1657. See #26313.

/cc @jcouv @OmarTawfik

sharwell · 2018-04-22T03:53:34Z

src/VisualStudio/Core/Test.Next/Services/VisualStudioDiagnosticAnalyzerExecutorTests.cs

                var exception = await Assert.ThrowsAnyAsync<OperationCanceledException>(() => SessionWithSolution.CreateAsync(connection, solution, source.Token));
                Assert.Equal(exception.CancellationToken, source.Token);

                // make sure things that should have been cleaned up are cleaned up
                var service = (RemotableDataServiceFactory.Service)solution.Workspace.Services.GetService<IRemotableDataService>();
                Assert.Null(service.GetRemotableData_TestOnly(solutionChecksum, CancellationToken.None));
-                Assert.True(connection.Disposed);
+                Assert.False(((InvokeThrowsCancellationConnection)connection.Target).Disposed);


@heejaechang This type of situation is what led me to make the comment here: #26178 (comment).

still not sure what you mean. original code was as soon as connection is given to SessionWithSolution.CreateAsync ownership is moved according to your term. and CreateAsync got the responsibility to dispose it.

now it looks like you changed that to not move ownership until the constructor which is after test is designed to throw cancellation. so sure the test will fail. since this PR changed responsibility of disposing the connection to the extension method. previous PR explicitly tried to move the responsibility to the CreateAsync. and this basically reverted that.

(still not sure why there is Box, and then get the resource from it, rather than pass down ref Owner... thing to CreateAsync. if we are going to use this type to mark ownership, then it would be nice if this becomes a bit simpler)

anyway, you can make this test to do SessionWithSolution.CreateAsync(connection.Move(), ..) to have same semantic if you want to.

I like your new pattern better though, connection can always dispose his connection without worrying about ownership. making ownership tracking simpler. but can be done by your refcountingdisposable type as well.

heejaechang · 2018-04-22T07:06:29Z

src/Workspaces/Core/Portable/Utilities/OwnedDisposable`1.cs

+        }
+
+        public OwnedDisposable(Func<T> resource)
+        {


I am not sure when this ctor should be used compared to the one above.

The factory function avoids the need to transfer an object into this constructor

is there reason you want to avoid transfer ownership? and it is using ref, so does it require to move ownership? a bit hard to know when I should and shouldn't move ownership. it would be nicer if one can more easily know when to call Claim or Move or use ref.

I am not sure when this ctor should be used compared to the one above.

The other constructor avoids a closure allocation, but I haven't found a way to use it without triggering one of the IDisposable analyzer warnings. Per the primary design goal, so far this restricts the constructor usage to the form taking a delegate.

is there reason you want to avoid transfer ownership? and it is using ref, so does it require to move ownership? a bit hard to know when I should and shouldn't move ownership. it would be nicer if one can more easily know when to call Claim or Move or use ref.

These rules are not fully created yet. The answers will depend on how the previous item drives the design.

heejaechang · 2018-04-22T07:32:16Z

src/Workspaces/Core/Portable/Remote/RemoteHostClientExtensions.cs


-            return await SessionWithSolution.CreateAsync(connection, solution, cancellationToken).ConfigureAwait(false);
+                using (var boxed = connection.Box())


can you explain when people use box? is it like if something inside of me take ownership of resource from me then no-op otherwise I will dispose it kind of thing?

➡️ Ownership of an IDisposable resource will exist at one location in memory with this pattern. Normally this location is stored on the evaluation stack for efficiency (within the struct). However, it is not always possible to reference a memory location on the evaluation stack (e.g. when calling an async method), so a boxed location is provided as a fallback to allow explicit ownership transfer in these cases.

heejaechang · 2018-04-22T07:37:03Z

src/Workspaces/Core/Portable/Remote/RemoteHostSessionHelpers.cs

@@ -51,7 +48,7 @@ internal sealed class SessionWithSolution : IDisposable
            }
        }

-        private SessionWithSolution(RemoteHostClient.Connection connection, PinnedRemotableDataScope scope)
+        private SessionWithSolution(ref OwnedDisposable<RemoteHostClient.Connection> connection, PinnedRemotableDataScope scope)
        {
            _connection = connection;


shouldn't this has Move? otherwise, won't using (.. Box()) will dispose the connection?

➡️ Yes, the instance was incorrectly copied by value. Corrected in 3075c31.

heejaechang · 2018-04-22T07:44:53Z

I like the idea of having special type for Disposable. not sure whether we always want to use wrapper for it but I can see some benefit of having those as well. since one can always dispose it without worrying about whether it has ownership or not.

...

one can always dispose it without worrying about whether it has ownership or not.

I think we can also use the referencecounteddispoable type thing for this purpose as well without introducing new wrapper if we want to by the way.

sharwell · 2018-04-23T11:46:12Z

I like the idea of having special type for Disposable. not sure whether we always want to use wrapper for it

I have not found a way to meet the primary design goal without a wrapper type, but we should evaluate any other solution which adheres to the constraints.

I think we can also use the referencecounteddispoable type thing for this purpose as well without introducing new wrapper if we want to by the way.

ReferenceCountedDisposable<T> is a robust wrapper, but using it requires allocation overhead which would be nice to avoid.

CyrusNajmabadi · 2018-04-23T18:51:24Z

src/VisualStudio/Core/Def/Implementation/Remote/ServiceHubRemoteHostClient.ConnectionManager.cs

@@ -146,7 +160,7 @@ private void Free(string serviceName, JsonRpcConnection connection)
                    }

                    // pool the connection
-                    queue.Enqueue(connection);
+                    queue.Enqueue(connection.Move());


Does move mean "extra out value, and release ownership?"

"Transfer ownership from the current instance to the returned value, which may be stored in the location of the new owner"

CyrusNajmabadi · 2018-04-23T18:53:14Z

src/Workspaces/Core/Portable/Utilities/OwnedDisposable`1.cs

+            _resource = resource();
+        }
+
+        public T Target => _resource;


Does this need to be an Interlocked.Read?

CyrusNajmabadi · 2018-04-23T18:54:34Z

src/Workspaces/Core/Portable/Utilities/OwnedDisposable`1.cs

+
+namespace Roslyn.Utilities
+{
+    internal struct OwnedDisposable<T> : IDisposable, IEquatable<OwnedDisposable<T>>


It feels like making this a struct makes some things more painful (like needing to ensure it's not accidentally copied, which can happen so easily). It doesn't look like this i used in hotspots that woudl warrant the need for a struct over a class. Having this be a class seems like it would simplify and ensure invariants better, while having negligible perf overhead.

The issues mentioned in the original design goal make it virtually impossible to use IDisposable objects in any non-trivial setting (e.g. a simple using statement) without a wrapper. I'm concerned the aggregate impact would become non-trivial.

CyrusNajmabadi · 2018-04-23T18:55:27Z

src/Workspaces/Core/Portable/Utilities/OwnedDisposable`1.cs

+        public override int GetHashCode()
+        {
+            return EqualityComparer<T>.Default.GetHashCode(_resource);
+        }


should this type even be hashable? do we need that. given that it can mutate in very significant ways, it seems like possibly no. perhaps better to throw here until there is a need for this?

The type passes through the hash properties of the underlying resource.

except that this tyep is mutable and _resource can become null. That seems very weird to me. Do we have an existing use case where we need to use OwnedDisposables as keys?

except that this tyep is mutable and _resource can become null.

It's not valid to use a resource after it is transferred away. The only defined operation at this point is Dispose() (which becomes a NOP).

CyrusNajmabadi · 2018-04-23T19:15:20Z

Am i correct in that this appears to be trying to provide a .net version of unique_ptr? Or is there another type out there (in the C++/rust/midori space) that this more closely emulates? Thanks!

mavasani · 2018-04-23T19:19:13Z

@sharwell - This is an interesting approach to tackling the dispose ownership problem, and probably can be recommended as a possible solution for projects that value the disposable rules but are bothered by the high false positive rate because the rule itself does not track dispose ownership transfer.

sharwell · 2018-04-23T19:21:36Z

Am i correct in that this appears to be trying to provide a .net version of unique_ptr?

This is almost exactly what it is.

sharwell · 2018-04-23T19:23:43Z

@mavasani It seems that new DisposableStruct(value) does not trigger the code analysis warnings when the type is a struct. Is this a known limitation/bug?

CyrusNajmabadi · 2018-04-23T19:29:54Z

This is almost exactly what it is.

Ok, that's what i though, and that seems very useful. My only critique is what i listed above, that i think makign this a struct actually does us little favor (though i totally can commiserate with the desire to have a zero cost abstraction). I think being a struct simply makes the usage too unweildy and confusing.

In this case, this is one of those: having a mutable struct that you have to handle with utmost care just seems too onerous. I would far prefer a small-cost class-based version of this that was much simpelr to deal with, had far less niggles about how it was used, but which only cost us an allocation in a non-hot path it seems.

mavasani · 2018-04-23T19:31:50Z

It seems that new DisposableStruct(value) does not trigger the code analysis warnings when the type is a struct. Is this a known limitation/bug?

Yes, FXCop rule implementation did not flag value types, and I retained the semantics. I agree we should fix it, even if this is a breaking change, it is a good one.

sharwell · 2018-04-23T21:02:29Z

I would far prefer a small-cost class-based version of this that was much simpelr to deal with, had far less niggles about how it was used, but which only cost us an allocation in a non-hot path it seems.

For this, the suggestions by @heejaechang come into play - ReferenceCountedDisposable<T> may be a suitable type for general use. In addition to already being hardened against the cases needed for OwnedDisposable<T>, it can be used in robust ways with asynchronous operations and other cases where the "safe point to dispose" is not easily defined or verified in code.

sharwell added 3 commits April 21, 2018 22:06

Add helper class OwnedDisposable<T>

857240b

Use OwnedDisposable<T> for connections

552604e

Use a factory method to avoid warnings about IDisposable use

dfc039a

sharwell requested a review from a team as a code owner April 22, 2018 03:34

sharwell commented Apr 22, 2018

View reviewed changes

sharwell added Area-Analyzers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Apr 22, 2018

heejaechang reviewed Apr 22, 2018

View reviewed changes

Fix incorrect copy of owned resource by value

3075c31

CyrusNajmabadi reviewed Apr 23, 2018

View reviewed changes

sharwell closed this May 11, 2018

sharwell mentioned this pull request May 17, 2018

Warning for struct copy of selected types #26937

Open


		return await SessionWithSolution.CreateAsync(connection, solution, cancellationToken).ConfigureAwait(false);
		using (var boxed = connection.Box())

[WIP] OwnedDisposable<T> #26321

[WIP] OwnedDisposable<T> #26321

Conversation

sharwell commented Apr 22, 2018 • edited Loading

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

sharwell left a 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

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

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

heejaechang Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

heejaechang Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

heejaechang Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

sharwell Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

heejaechang Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

heejaechang commented Apr 22, 2018 • edited Loading

sharwell commented Apr 23, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Apr 23, 2018

mavasani commented Apr 23, 2018

sharwell commented Apr 23, 2018

sharwell commented Apr 23, 2018

CyrusNajmabadi commented Apr 23, 2018 • edited Loading

mavasani commented Apr 23, 2018

sharwell commented Apr 23, 2018

sharwell commented Apr 22, 2018 •

edited

Loading

heejaechang Apr 22, 2018 •

edited

Loading

heejaechang Apr 22, 2018 •

edited

Loading

heejaechang Apr 22, 2018 •

edited

Loading

sharwell Apr 23, 2018 •

edited

Loading

heejaechang Apr 22, 2018 •

edited

Loading

sharwell Apr 23, 2018 •

edited

Loading

heejaechang commented Apr 22, 2018 •

edited

Loading

CyrusNajmabadi commented Apr 23, 2018 •

edited

Loading