Fix connection leaking on cancellation by heejaechang · Pull Request #26178 · dotnet/roslyn

heejaechang · 2018-04-16T20:07:07Z

Customer scenario

While users are using VS. Connections to servicehub can be leaked and number of connections get increased overtime.

Bugs this fixes

#26183

Workarounds, if any

there is no workaround

Risk

I don't see any risk

Performance impact

It should remove connection leaks.

Is this a regression from a previous update?

No

Root cause analysis

when we create certain type of connection, if cancellation happens at right time, we didn't dispose connection before exception escapes current call.

How was the bug found?

dogfooding and unit testing.

jasonmalinowski · 2018-04-16T20:09:18Z

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

            }

 #if DEBUG
+            private string _creationCallStack;


Why only debug? still seems useful in release.

CyrusNajmabadi · 2018-04-16T20:16:12Z

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

                if (!Environment.HasShutdownStarted)
                {
-                    Contract.Requires(false, $@"Should have been disposed!");
+                    Contract.Requires(false, $"Should have been disposed!\r\n {_creationCallStack}");


why not NFW here?

…be leaked not disposed. fixed the issue and added test

heejaechang · 2018-04-17T00:06:29Z

@jinujoseph can we bring this to 15.7? @dotnet/roslyn-ide @dotnet/roslyn-analysis can you take a look?

heejaechang · 2018-04-17T00:06:57Z

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

                return null;
            }

-            var scope = await GetPinnedScopeAsync(solution, cancellationToken).ConfigureAwait(false);


issue is this throwing cancellation and connection not get disposed.

heejaechang · 2018-04-17T00:56:45Z

retest windows_debug_vs-integration_prtest please

heejaechang · 2018-04-17T00:57:09Z

LB issue is known one. I will take a look why sometimes LB doesn't show up.

sharwell

Need to take a closer look tomorrow.

sharwell · 2018-04-17T11:02:36Z

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

+                var connection = new MyConnection(source);
+                try
+                {
+                    await SessionWithSolution.CreateAsync(connection, solution, source.Token);


❕ Remove the try/catch here. Use Assert.ThrowsAsync if the code throws.

sharwell · 2018-04-17T11:05:02Z

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

+                // 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);


❓ why would this be true here? The ownership transfer isn't clear and the test method doesn't dispose of the object, but I'm also reviewing this on my phone so maybe I just need a better view.

whole point of this test is if it throws connection get disposed correctly.

sharwell · 2018-04-17T11:06:51Z

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

                if (!Environment.HasShutdownStarted)
                {
-                    Contract.Requires(false, $@"Should have been disposed!");
+#if DEBUG


📝 If this had wrapped the entire finalizer from the start, this bug would be much less serious.

not sure what you meant.

CyrusNajmabadi · 2018-04-17T18:59:35Z

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

+#if DEBUG
+                    Contract.Fail($"Should have been disposed!\r\n {_creationCallStack}");
+#else
+                    Contract.Fail($"Should have been disposed!");


is there a reason these are not NFWs?

Fail will crash VS.

CyrusNajmabadi · 2018-04-17T18:59:59Z

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

            protected Connection()
            {
+#if DEBUG
+                _creationCallStack = Environment.StackTrace;


why only do this in debug? we create connections rarely, and then pool them, right? so this should be cheap enough to do even in release.

no connection is created many times. and we see this issue again we can make this release later.

heejaechang · 2018-04-17T19:45:12Z

ping? can I get code review?

heejaechang · 2018-04-17T21:03:33Z

@dotnet/roslyn-infrastructure
failed with 13:12:17
C:\Users\runner.nuget\packages\Microsoft.VSSDK.BuildTools\15.8.68-develop-g109a00ff\tools\VSSDK\Microsoft.VsSDK.targets(643,5): error MSB6006: "VsixUtil.exe" exited with code -1073741819. [D:\j\workspace\windows_debug---15a478d3\src\Compilers\Extension\CompilerExtension.csproj

https://ci.dot.net/job/dotnet_roslyn/job/dev15.7.x/job/windows_debug_spanish_unit32_prtest/158/

heejaechang · 2018-04-17T21:03:45Z

retest windows_debug_spanish_unit32_prtest please

heejaechang · 2018-04-17T21:04:17Z

retest windows_release_vs-integration_prtest please

sharwell

The direction here is fine, but there are a few more clean-up items to polish the test.

sharwell · 2018-04-17T22:57:51Z

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

+                _source.Cancel();
+                _source.Token.ThrowIfCancellationRequested();
+
+                return Task.CompletedTask;


💡

throw ExceptionUtilities.Unreachable;

sharwell · 2018-04-17T22:58:32Z

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

            public override Workspace Workspace => _workspace;
        }
+
+        private class MyConnection : RemoteHostClient.Connection


❕ Rename type to unambiguously indicate its test behavior (e.g. InvokeThrowsCancellationConnection).

sharwell · 2018-04-17T22:59:09Z

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

            {
-                sessionWithSolution.Dispose();
+                // make sure disposable objects are disposed when
+                // exceptions are thrown


💡 This comment is unnecessary and can be removed

this tells what this test is trying to do.

sharwell · 2018-04-17T23:01:33Z

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

+
+                var source = new CancellationTokenSource();
+                var connection = new MyConnection(source);
+                await Assert.ThrowsAnyAsync<OperationCanceledException>(() => SessionWithSolution.CreateAsync(connection, solution, source.Token));


❕ This should invoke the entry point for the error RemoteHostClientExtensions.TryCreateSessionAsync rather than using the inner helper methods from SessionWithSolution.

that sure why. the bug is not there. but in CreateAsync.

Testing of TryCreateSessionAsync would ensure that the complete sequence leading to the bug is corrected:

The connection is created

Something happens

The connection fails to be disposed

As-written, the test does not cover the first step of the scenario. I'll unblock because it may be substantially more work to write the more complete test.

I believe I moved (2) down to CreateAsync. and nothing left in TryCreateSessionAsync. but if there is a bug, we still have Contract.Fail.

sharwell · 2018-04-17T23:02:29Z

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

            }
        }

+        [Fact, Trait(Traits.Feature, Traits.Features.RemoteHost)]


❗️ Missing [WorkItem(...)]

heejaechang · 2018-04-17T23:18:01Z

ping? need some code review! @jinujoseph

heejaechang · 2018-04-18T00:37:01Z

retest windows_debug_vs-integration_prtest please

jasonmalinowski · 2018-04-18T01:01:45Z

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

+
+                var source = new CancellationTokenSource();
+                var connection = new InvokeThrowsCancellationConnection(source);
+                await Assert.ThrowsAnyAsync<OperationCanceledException>(() => SessionWithSolution.CreateAsync(connection, solution, source.Token));


Nit: assert that the resulting exception (which is returned so you can assert with it) has OperationCanceledException.CancellationToken equal to source.CancellationToken. Since there's some catching/rethrowing going on we don't want to misassociate the cancellation source.

is there Assert.ThrowXXX that let me get to the exception? let me take a look.

is there Assert.ThrowXXX that let me get to the exception

Yes, they all return the exception instance.

sharwell · 2018-04-18T01:38:00Z

@heejaechang Did you see these?

#26178 (comment)

heejaechang · 2018-04-18T17:45:24Z

@jinujoseph are we going to take this for 15.7?

sharwell · 2018-04-18T18:26:05Z

While users are using VS. Connections to servicehub can be leaked and number of connections get increased overtime.

How is this occurring? I would expect the GC to reclaim the connections when they are no longer referenced.

jinujoseph · 2018-04-18T19:48:07Z

@heejaechang , lets target this for 15.8

heejaechang · 2018-04-18T20:08:40Z

How is this occurring? I would expect the GC to reclaim the connections when they are no longer referenced.

probably. I didn't look through servicehub's stream implementation to see whether it is safe to let go without dispose and let finalizer to take care of disposing it.

regardless, it is resource that is supposed to be disposed and I am not disposing it. so it is certainly leak.

heejaechang · 2018-04-18T20:09:03Z

@jinujoseph do we have 15.8 branch? or master?

jinujoseph · 2018-04-18T20:32:11Z

15.8 - > master

sharwell · 2018-04-18T21:03:40Z

Just waiting on #26178 (comment)

sharwell · 2018-04-18T21:43:18Z

💡 @heejaechang It would be good to revise the Scenario and Performance Impact sections of the original template to more accurately explain the change. Specifically, this is not a performance problem caused by an increasing number of connections, but rather a reliability problem caused by an assertion that the connection is deterministically cleaned up when it is no longer in use. The simple fix of removing the [unnecessary] finalizer would address the reliability problem, but this goes a step further to address the underlying cause of non-deterministic release.

heejaechang · 2018-04-18T21:54:48Z

since I am using external lib, I am not sure how I can guarantee I am not leaking anything when I dont dispose disposable resource. also, It will definitively not decrease ref count on shared connection. so it is not just about debug assert.

jasonmalinowski · 2018-04-18T21:56:54Z

@heejaechang @jinujoseph I'm OK with taking the code fix in 15.8, but if we're going to do that we should get this assert disabled in 15.7 pronto, as this is one of the top flaky issues right now.

heejaechang · 2018-04-18T22:13:11Z

@jinujoseph should I create new PR that simply remove the debug assert for 15.7?

jinujoseph · 2018-04-18T22:17:55Z

yes , lets remove the assert from 15.7

add callstack on debug to make investigation easier

66e6c7c

heejaechang requested a review from a team as a code owner April 16, 2018 20:07

heejaechang changed the base branch from master to dev15.7.x April 16, 2018 20:07

jasonmalinowski reviewed Apr 16, 2018

View reviewed changes

CyrusNajmabadi reviewed Apr 16, 2018

View reviewed changes

if GetPinnedScopeAsync throws cancellation exception, connection can …

5f403d7

…be leaked not disposed. fixed the issue and added test

heejaechang commented Apr 17, 2018

View reviewed changes

heejaechang changed the title ~~add callstack on debug to make investigation easier~~ Fix connection leaking on cancellation Apr 17, 2018

make sure cancellation is thrown in test

29f2787

sharwell suggested changes Apr 17, 2018

View reviewed changes

CyrusNajmabadi reviewed Apr 17, 2018

View reviewed changes

PR feedbacks

feb1912

sharwell suggested changes Apr 17, 2018

View reviewed changes

sharwell reviewed Apr 17, 2018

View reviewed changes

more feedbacks

e770a6c

jasonmalinowski reviewed Apr 18, 2018

View reviewed changes

jasonmalinowski approved these changes Apr 18, 2018

View reviewed changes

address PR feedback

a7e5709

heejaechang changed the base branch from dev15.7.x to master April 18, 2018 21:05

sharwell approved these changes Apr 18, 2018

View reviewed changes

heejaechang merged commit cee5906 into dotnet:master Apr 18, 2018

This was referenced Apr 19, 2018

Remove debug assert that is causing some unit tests to be flaky. #26259

Merged

move back to debug assert. #26260

Merged

sharwell mentioned this pull request Apr 22, 2018

[WIP] OwnedDisposable<T> #26321

Closed

Conversation

heejaechang commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

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!

heejaechang commented Apr 17, 2018

Uh oh!

heejaechang Apr 17, 2018 • 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!

heejaechang commented Apr 17, 2018

Uh oh!

heejaechang commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell left a comment

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!

Choose a reason for hiding this comment

Uh oh!

heejaechang Apr 17, 2018 • 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!

CyrusNajmabadi Apr 17, 2018 • 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!

heejaechang commented Apr 17, 2018

Uh oh!

heejaechang commented Apr 17, 2018

Uh oh!

heejaechang commented Apr 17, 2018

Uh oh!

heejaechang commented Apr 17, 2018

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Apr 17, 2018 • 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

heejaechang commented Apr 16, 2018 •

edited

Loading

heejaechang Apr 17, 2018 •

edited

Loading

heejaechang commented Apr 17, 2018 •

edited

Loading

heejaechang Apr 17, 2018 •

edited

Loading

CyrusNajmabadi Apr 17, 2018 •

edited

Loading

sharwell Apr 17, 2018 •

edited

Loading