Dispose Hubs from ActivatorUtilities #44

BrennanConroy · 2016-11-21T22:04:56Z

We weren't disposing the hubs from Activator.CreateInstance<THub>() in a couple places

@moozzyk @anurse @davidfowl @mikaelm12 @muratg

dnfclas · 2016-11-21T22:05:01Z

Hi @BrennanConroy, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

moozzyk · 2016-11-21T22:22:01Z

src/Microsoft.AspNetCore.SignalR/HubEndPoint.cs

+
+            return hub;
+        }
+
        private void InitializeHub(Connection connection, THub hub)


Do you still need this method? Why not inline it in CreateHub?

moozzyk · 2016-11-21T22:27:05Z

src/Microsoft.AspNetCore.SignalR/HubEndPoint.cs

@@ -63,9 +63,16 @@ public class HubEndPoint<THub, TClient> : EndPoint where THub : Hub<TClient>

                using (var scope = _serviceScopeFactory.CreateScope())


Can you remind me why we have separate scopes for Connected/Disconnected?

I think it's because we are simulating scoped services per "request" so connected is the first request and disconnected is from a different request.

Hubs are transient and a scope needs to be created whenever we make a hub. We'll need to see how bad the performance is for this.

I understand that for invocations hubs are transient. I was wondering if it has to be the case for onconnected/disconnected but I think you are right - making them non-transient here would be really weird.

muratg · 2016-11-22T01:22:18Z

src/Microsoft.AspNetCore.SignalR/HubEndPoint.cs

@@ -63,9 +63,16 @@ public class HubEndPoint<THub, TClient> : EndPoint where THub : Hub<TClient>

                using (var scope = _serviceScopeFactory.CreateScope())
                {
-                    var hub = scope.ServiceProvider.GetService<THub>() ?? Activator.CreateInstance<THub>();
-                    InitializeHub(connection, hub);
+                    bool created;


var created = false?

Also, I'd like someone to take a look at the name created in this context (and in CreateHub method below). It's set to true only if it's created by Activator, though even if it's coming through a GetService call, someone have "created" it before :)

bool created is fine. In general I find this pattern ugly and it is now across the entire file but honestly, it works.

It's really a design flaw with the pattern. We need a HubActivator with a Create and Dispose like MVC's ControllerActivator (or is it the factory that disposes..)

davidfowl

Add a test

BrennanConroy · 2016-11-29T23:33:37Z

Added a test

davidfowl · 2016-11-30T01:14:47Z

src/Microsoft.AspNetCore.SignalR/HubEndPoint.cs

+                    if (created)
+                    {
+                        // Dispose the object if it's disposable and we created it
+                        (hub as IDisposable)?.Dispose();


Why the cast?

Odd, I thought I copied it from somewhere...

davidfowl · 2016-11-30T01:15:35Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+            {
+                var endPointTask = Task.Run(() => endPoint.OnConnectedAsync(connectionWrapper.Connection));
+
+                Thread.Sleep(100);


I wasn't sure what to do here, don't want to await endPoint.OnConnectedAsync(...) because it wont complete, and need to wait a small amount of time for endPoint.OnConnectedAsync(...) to actually run and get to DispatchMessagesAsync

Since you have a handle on the Pipeline that was created and passed into OnConnectedAsync, you can get access to the PipelineReaderWriter and await ReadingStarted.

davidfowl · 2016-11-30T01:15:52Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+
+        private class TestHub : Hub
+        {
+            static public int DisposedCount = 0;


Why is this static?

So we can Assert that dispose was called on connect and on disconnect.

I think you should pass in an object that has the state and not use a static for this.

BrennanConroy · 2016-11-30T17:55:30Z

src/Microsoft.AspNetCore.SignalR/HubEndPoint.cs

+
+            if (hub == null)
+            {
+                hub = ActivatorUtilities.CreateInstance<THub>(provider);


FYI: Changed this so that hubs with non-default constructors work without adding the hub to DI

davidfowl · 2016-11-30T18:14:53Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+            {
+                if (dispose)
+                {
+                    Interlocked.Increment(ref _trackDispose.DisposeCount);


Why is this interlocked?

davidfowl · 2016-11-30T18:15:47Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+            {
+                var endPointTask = Task.Run(() => endPoint.OnConnectedAsync(connectionWrapper.Connection));
+
+                await ((HttpConnection)connectionWrapper.Connection.Channel).Input.ReadingStarted;


Expose the HttpConnection directly on connectionWrapper so you can avoid the cast

davidfowl · 2016-11-30T18:16:18Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+
+                await endPointTask;
+
+                Assert.Equal(2, trackDispose.DisposeCount);


Why is it 2?

OnConnectedAsync + OnDisconnectedAsync

mikaelm12 · 2016-11-30T18:57:30Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

@@ -0,0 +1,105 @@
+using System;


License and copyright header

moozzyk · 2016-11-30T19:07:50Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+        public async Task HubsAreDisposed()
+        {
+            var trackDispose = new TrackDispose();
+            var serviceProvider = CreateServiceProvider((s) => s.AddSingleton(trackDispose));


nit: parens not needed in (s)

moozzyk · 2016-11-30T19:09:17Z

test/Microsoft.AspNetCore.SignalR.Tests/project.json

+    "dotnet-test-xunit": "2.2.0-*",
+    "Microsoft.AspNetCore.Hosting": "1.2.0-*",
+    "Microsoft.AspNetCore.Sockets": "0.1.0-*",
+    "Microsoft.AspNetCore.SignalR": "1.0.0-*",


should be project reference

moozzyk · 2016-11-30T19:10:46Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+                var user = new ClaimsPrincipal(new ClaimsIdentity());
+                Connection = connectionManager.AddNewConnection(_httpConnection).Connection;
+                Connection.Metadata["formatType"] = format;
+                Connection.User = user;


golfing: initialize directly (i.e. without the user variable)

moozzyk · 2016-11-30T19:28:51Z

test/Microsoft.AspNetCore.SignalR.Tests/HubEndpointTests.cs

+
+            using (var connectionWrapper = new ConnectionWrapper())
+            {
+                var endPointTask = Task.Run(() => endPoint.OnConnectedAsync(connectionWrapper.Connection));


can't it be just var endPointTask = endPoint.OnConnectedAsync(connectionWrapper.Connection); ?

Hmm... good catch

davidfowl · 2016-12-03T09:15:45Z

Next, this logic needs to move into a hub activator like component.

davidfowl · 2016-12-03T09:16:12Z

test/Microsoft.AspNetCore.SignalR.Tests/Microsoft.AspNetCore.SignalR.Tests.xproj

+  <ItemGroup>
+    <Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />
+  </ItemGroup>
+  <Import Project="$(VSToolsPath)\DNX\Microsoft.DNX.targets" Condition="'$(VSToolsPath)' != ''" />


DNX? What version of VS do you have??

dnfclas added the cla-already-signed label Nov 21, 2016

moozzyk reviewed Nov 21, 2016

View reviewed changes

analogrelay approved these changes Nov 21, 2016

View reviewed changes

muratg reviewed Nov 22, 2016

View reviewed changes

davidfowl suggested changes Nov 22, 2016

View reviewed changes

BrennanConroy force-pushed the brecon/hub branch from d0eb0a6 to 08e3cde Compare November 29, 2016 23:24

davidfowl reviewed Nov 30, 2016

View reviewed changes

BrennanConroy commented Nov 30, 2016

View reviewed changes

davidfowl reviewed Nov 30, 2016

View reviewed changes

mikaelm12 reviewed Nov 30, 2016

View reviewed changes

moozzyk reviewed Nov 30, 2016

View reviewed changes

davidfowl reviewed Dec 3, 2016

View reviewed changes

davidfowl approved these changes Dec 3, 2016

View reviewed changes

Dispose Hubs from ActivatorUtilities

28e3c83

BrennanConroy force-pushed the brecon/hub branch from 623722e to 28e3c83 Compare December 5, 2016 23:41

BrennanConroy mentioned this pull request Dec 5, 2016

Make a HubActivator #60

Closed

BrennanConroy merged commit 28e3c83 into dev Dec 6, 2016

BrennanConroy deleted the brecon/hub branch December 6, 2016 00:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dispose Hubs from ActivatorUtilities #44

Dispose Hubs from ActivatorUtilities #44

BrennanConroy commented Nov 21, 2016

dnfclas commented Nov 21, 2016

moozzyk Nov 21, 2016

moozzyk Nov 21, 2016

BrennanConroy Nov 21, 2016

davidfowl Nov 22, 2016

moozzyk Nov 22, 2016

muratg Nov 22, 2016

moozzyk Nov 22, 2016

davidfowl Nov 22, 2016

davidfowl left a comment

BrennanConroy commented Nov 29, 2016

davidfowl Nov 30, 2016

BrennanConroy Nov 30, 2016

davidfowl Nov 30, 2016

BrennanConroy Nov 30, 2016

davidfowl Nov 30, 2016

davidfowl Nov 30, 2016

BrennanConroy Nov 30, 2016

davidfowl Nov 30, 2016

BrennanConroy Nov 30, 2016

davidfowl Nov 30, 2016

davidfowl Nov 30, 2016

davidfowl Nov 30, 2016

BrennanConroy Nov 30, 2016

davidfowl Dec 3, 2016

mikaelm12 Nov 30, 2016

moozzyk Nov 30, 2016

moozzyk Nov 30, 2016

moozzyk Nov 30, 2016

moozzyk Nov 30, 2016 •

edited

Loading

BrennanConroy Nov 30, 2016

davidfowl commented Dec 3, 2016

davidfowl Dec 3, 2016

		@@ -63,9 +63,16 @@ public class HubEndPoint<THub, TClient> : EndPoint where THub : Hub<TClient>

		using (var scope = _serviceScopeFactory.CreateScope())


		await endPointTask;

		Assert.Equal(2, trackDispose.DisposeCount);

Dispose Hubs from ActivatorUtilities #44

Dispose Hubs from ActivatorUtilities #44

Conversation

BrennanConroy commented Nov 21, 2016

dnfclas commented Nov 21, 2016

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

davidfowl left a comment

Choose a reason for hiding this comment

BrennanConroy commented Nov 29, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moozzyk Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Dec 3, 2016

Choose a reason for hiding this comment

moozzyk Nov 30, 2016 •

edited

Loading