Client Subset - AllExcept #700

mikaelm12 · 2017-08-09T22:08:52Z

Wanted some initial review + wanted to get some discussion on how to do this for the RedisHubLifetimeManager. Discussed this with @moozzyk yesterday a little but I figured it would be good to put it out here.

dnfclas · 2017-08-09T22:08:55Z

@mikaelm12,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

moozzyk

Some nits

moozzyk · 2017-08-09T22:21:58Z

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

@@ -491,7 +491,6 @@ public void HubsCannotHaveOverloadedMethods()
                Task secondEndPointTask = endPoint.OnConnectedAsync(secondClient.Connection);


while you are at it: rename the test class and the file to HubEndPointTests

moozzyk · 2017-08-09T23:35:45Z

src/Microsoft.AspNetCore.SignalR/IHubClients.cs

 namespace Microsoft.AspNetCore.SignalR
 {
    public interface IHubClients
    {
        IClientProxy All { get; }

+        IClientProxy AllExcept(List<string> excludedIds);


params string[] as per: https://github.com/SignalR/SignalR/blob/1fba14fa3437e24c204dfaf8a18db3fce8acad3c/src/Microsoft.AspNet.SignalR.Core/Hubs/IHubConnectionContext.cs#L14

I don't think having params on a public interface API is a great plan actually... I'd rather use IEnumerable or IReadOnlyList and have a params variant implemented in an extension method. params forces you to allocate an array, if you don't already have one (until we get params IEnumerable someday...)

I thought about IEnumerable but there is always a problem with lazy evaluation. IReadOnlyList is a good option. Agree that params is a little weird (you don't know connectionIds upfront but you need to know the count) but it saves you a few keystrokes if you want to exclude just one connectionId you store in a variable (e.g. .Others).

moozzyk · 2017-08-09T23:37:26Z

samples/SocketsSample/Hubs/Chat.cs

@@ -3,6 +3,7 @@

 using System;
 using System.Threading.Tasks;
+using System.Collections.Generic;


Edit: Yeah it is, I was looking at the wrong file when I originally commented.

moozzyk · 2017-08-09T23:37:42Z

samples/ChatSample/PresenceHubLifetimeManager.cs

@@ -141,6 +142,11 @@ public override Task InvokeAllAsync(string methodName, object[] args)
            return _wrappedHubLifetimeManager.InvokeAllAsync(methodName, args);
        }

+        public override Task InvokeAllExceptAsync(List<string> excludedIds, string methodName, object[] args)
+        {
+            return _wrappedHubLifetimeManager.InvokeAllExceptAsync( excludedIds, methodName, args);


moozzyk · 2017-08-09T23:41:48Z

src/Microsoft.AspNetCore.SignalR/DefaultHubLifetimeManager.cs

+        {
+            return InvokeAllWhere(methodName, args, connection =>
+            {
+                return !excludedIds.Contains(connection.ConnectionId);


Not sure if this is for now but this feels slow

Yeah, using a HashSet<string> for excludedIds would be faster, since it has a O(1) contains test

BrennanConroy · 2017-08-10T16:02:08Z

For redis; servers can subscribe to a new channel typeof(THub).FullName + ".AllExcept" and when AllExcept is called we can put the list of connections in the message and then when the servers receive it they could parse the message for the connections...

Or we could create an internal HubMessage wrapper that has a list of connections so the parsers could deal with it instead of us.

BrennanConroy · 2017-08-10T21:12:08Z

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

+        {
+            var serviceProvider = CreateServiceProvider();
+
+            dynamic endPoint = serviceProvider.GetService(GetEndPointType(typeof(MethodHub)));


var endPoint = serviceProvider.GetService<HubEndPoint<MethodHub>>();

Dynamic not needed in this test

KPixel · 2017-08-11T16:34:34Z

Great to see this feature coming.

I think its implementation should be the reverse of InvokeConnectionsAsync(). I discussed that one here: #683. Can you please include it?

I also like the idea of a HubMessage wrapper that takes the list of connectionIds. We can reuse it there too.

Can you change the type of excludedIds to ICollection<string>? It will enable using HashSet, which makes sense here.

analogrelay

Interface all the things!

analogrelay · 2017-08-11T17:34:43Z

src/Microsoft.AspNetCore.SignalR/DefaultHubLifetimeManager.cs

+        {
+            return InvokeAllWhere(methodName, args, connection =>
+            {
+                return !excludedIds.Contains(connection.ConnectionId);


Yeah, using a HashSet<string> for excludedIds would be faster, since it has a O(1) contains test

analogrelay · 2017-08-11T17:35:03Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

@@ -340,6 +339,11 @@ private HubMessage DeserializeMessage(RedisValue data)
            return message;
        }

+        public override Task InvokeAllExceptAsync(List<string> excludedIds, string methodName, object[] args)
+        {
+            throw new NotImplementedException();


Assuming this is part of the WIP-ness of this PR ;)

Yeah lol. Working this out now 😄

analogrelay · 2017-08-11T17:36:56Z

src/Microsoft.AspNetCore.SignalR/HubContext.cs

@@ -20,6 +22,11 @@ public HubContext(HubLifetimeManager<THub> lifetimeManager)

        public virtual IGroupManager Groups { get; }

+        public IClientProxy AllExcept(List<string> excludedIds)


Should use IReadOnlyList<string> or IEnumerable<string> here. We never plan to write to this list, and we don't care exactly how it's implemented. In fact, if our implementations are going to use HashSet<string>, IEnumerable would be better since we're going to need to convert it to a different type of collection anyway.

Also, should have a params string[] excludedIds overload (possibly via an extension method) so you can call .AllExcept(connection1, connection2, connection3)...

analogrelay · 2017-08-11T17:38:04Z

src/Microsoft.AspNetCore.SignalR/HubLifetimeManager.cs

@@ -13,6 +14,8 @@ public abstract class HubLifetimeManager<THub>

        public abstract Task InvokeAllAsync(string methodName, object[] args);

+        public abstract Task InvokeAllExceptAsync(List<string> excludedIds, string methodName, object[] args);


IEnumerable<string>/IReadOnlyList<string>

analogrelay · 2017-08-11T17:39:30Z

src/Microsoft.AspNetCore.SignalR/IHubClients.cs

 namespace Microsoft.AspNetCore.SignalR
 {
    public interface IHubClients
    {
        IClientProxy All { get; }

+        IClientProxy AllExcept(List<string> excludedIds);


I don't think having params on a public interface API is a great plan actually... I'd rather use IEnumerable or IReadOnlyList and have a params variant implemented in an extension method. params forces you to allocate an array, if you don't already have one (until we get params IEnumerable someday...)

analogrelay · 2017-08-11T17:39:41Z

src/Microsoft.AspNetCore.SignalR/Proxies.cs

+        private readonly HubLifetimeManager<THub> _lifetimeManager;
+        private readonly List<string> _excludedIds;
+
+        public AllClientsExceptProxy(HubLifetimeManager<THub> lifetimeManager, List<string> excludedIds)


IEnumerable<string>/IReadOnlyList<string>

Why not ICollection<string>? It is the lowest level interface that offers Contains().

Edit: The dev can choose between convenience (List<string>, or params string[]) and performance (Set<string>) or even some other custom implementation.

We never need to write to the collection, so we should avoid using a writable interface, hence IReadOnlyCollection or IReadOnlyList would be a better option.

In general, we want to lead users towards good performance without them having to know how our code is implemented, so we try to avoid leading users to a path where they have to consider the performance of collection objects they give us. It's our job to make processing this collection fast, not the user's.

IReadOnlyCollection and IReadOnlyList do not have a Contains() method, which is essential to the implementation.

And if we care about performance, we must allow passing an ISet-based collection.

So, I think it is better to take a (potentially) writable interface that performs well than take a read-only collection that SignalR must first convert to something like HashSet.

That interface ends up being ICollection because we also want to allow IList for convenience. Otherwise, the overload method taking params string[] cannot pass its parameter directly.

The point is moot until we have benchmarks to properly understand the performance characteristics in practical scenarios. Given that the most common scenario we've seen for this involves a very small number of entries in the list, we may just spin our wheels here for a while with almost no gain :).

The original comment is just about using an interface over the concrete List<T> type currently used. IReadOnlyCollection<T>/IReadOnlyList<T> are the "correct" expressions of what this public API requires, which is generally our first stop when building an API. Performance decisions to compromise that have to be data-driven and based on actual usage.

mikaelm12 · 2017-08-16T22:31:49Z

Tested this functionality with the presence chat sample and it looks pretty good 😄

mikaelm12 · 2017-08-17T21:31:13Z

⬆️ 📅

BrennanConroy · 2017-08-17T21:48:34Z

src/Microsoft.AspNetCore.SignalR/DefaultHubLifetimeManager.cs

+        {
+            return InvokeAllWhere(methodName, args, connection =>
+            {
+                var excludedIdsSet = new HashSet<string>(excludedIds);


Move this outside of the callback, right now you allocate a HashSet for every connection

BrennanConroy · 2017-08-17T21:50:52Z

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

+        {
+            var serviceProvider = CreateServiceProvider();
+
+            dynamic endPoint = serviceProvider.GetService(GetEndPointType(typeof(MethodHub)));


Dynamic not needed in this test

BrennanConroy · 2017-08-17T21:51:50Z

src/Microsoft.AspNetCore.SignalR/HubContext.cs

@@ -20,6 +22,11 @@ public HubContext(HubLifetimeManager<THub> lifetimeManager)

        public virtual IGroupManager Groups { get; }

+        public IClientProxy AllExcept(IReadOnlyList<string> excludedIds)


Some public APIs use IReadOnlyList some use IReadOnlyCollection. Normalize them

Should all be IReadOnlyList now

BrennanConroy · 2017-08-17T21:51:59Z

src/Microsoft.AspNetCore.SignalR/IHubClients.cs

 namespace Microsoft.AspNetCore.SignalR
 {
    public interface IHubClients
    {
        IClientProxy All { get; }

+        IClientProxy AllExcept(IReadOnlyList<string> excludedIds);
+        //IClientProxy AllExcept(params string[] excludedIds);


BrennanConroy · 2017-08-17T21:52:39Z

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

@@ -124,8 +124,9 @@ public Task<string> SendInvocationAsync(string methodName, params object[] args)
        public async Task<string> SendInvocationAsync(string methodName, bool nonBlocking, params object[] args)
        {
            var invocationId = GetInvocationId();
-
-            var payload = _protocolReaderWriter.WriteMessage(new InvocationMessage(invocationId, nonBlocking, methodName, args));
+            var invocationMessage = new InvocationMessage(invocationId, nonBlocking, methodName, args);


BrennanConroy · 2017-08-18T15:20:10Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

+        public override Task InvokeAllExceptAsync(string methodName, object[] args, IReadOnlyCollection<string> excludedIds)
+        {
+            var message = new InvocationMessage(GetInvocationId(), nonBlocking: true, target: methodName, arguments: args);
+            var data = new object[] { message, excludedIds };


This looks... What was wrong with making a class that wraps InvocationMessage and adds a IReadOnlyCollection?

This was a WIP hack that I didn't update 😄

mikaelm12 · 2017-08-18T20:01:37Z

🆙 📅

BrennanConroy · 2017-08-18T20:34:44Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

+                var message = DeserializeMessage<RedisExcludeClientMessage>(data);
+                var invocationMessage = (InvocationMessage)message.HubMessage;
+                var excludedIds = message.ExcludedIds;
+                var excludedIdsSet = new HashSet<string>(excludedIds);


This isn't used

BrennanConroy · 2017-08-18T20:37:28Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

        }

-        private async Task PublishAsync(string channel, HubMessage hubMessage)
+        private async Task PublishAsync(string channel, object args)


BrennanConroy · 2017-08-18T20:39:35Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

@@ -362,6 +394,17 @@ public override void WriteLine(string value)
            }
        }

+        private class RedisExcludeClientMessage


This should be able to inherit from InvocationMessage

Would it make more sense to inherit from HubMessage?

BrennanConroy · 2017-08-18T20:40:51Z

src/Microsoft.AspNetCore.SignalR/DefaultHubLifetimeManager.cs

+        {
+            return InvokeAllWhere(methodName, args, connection =>
+            {
+                var excludedIdsSet = new HashSet<string>(excludedIds);


Should just get rid of this and call .Contains on excludedIds

Yeah, this will create a new HashSet for every check. If we decide we need a HashSet for optimization, we would want to initialize it outside the lambda so there's only one instance.

BrennanConroy · 2017-08-21T15:32:19Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

+        public override Task InvokeAllExceptAsync(string methodName, object[] args, IReadOnlyList<string> excludedIds)
+        {
+            var message = new InvocationMessage(GetInvocationId(), nonBlocking: true, target: methodName, arguments: args);
+            var redisExcludeMessage = new RedisExcludeClientMessage(message, excludedIds);


The point of making it inherit from InvocationMessage was that you could avoid newing up two objects, and just do a single RedisExcludeClientMessage

Hrm... feels like a a premature optimization, but I guess we can do it.

BrennanConroy · 2017-08-21T21:44:58Z

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

+                Assert.Equal("Send", invocation.Target);
+                Assert.Equal("To second", invocation.Arguments[0]);
+
+                var thirdClientResult = await thirdClient.ReadAsync().OrTimeout(); ;


extra semi-colon

BrennanConroy · 2017-08-21T21:45:21Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

+        {
+            public IReadOnlyList<string> ExcludedIds;
+
+            public RedisExcludeClientsMessage(string invocationId, bool nonBlocking, string target, IReadOnlyList<string> excludedIds, params object[] arguments) : base(invocationId, nonBlocking, target, arguments)


Should probably add a newline

BrennanConroy · 2017-08-21T21:45:50Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

+
+        public override Task InvokeAllExceptAsync(string methodName, object[] args, IReadOnlyList<string> excludedIds)
+        {
+            var message = new RedisExcludeClientsMessage(GetInvocationId(), nonBlocking: true, target: methodName, excludedIds: excludedIds ,arguments: args);


formatting excludedIds ,arguments:

analogrelay · 2017-08-22T17:31:17Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

+                var invocationMessage = (InvocationMessage)message.HubMessage;
+                var excludedIds = message.ExcludedIds;
+
+                //// TODO: This isn't going to work when we allow JsonSerializer customization or add Protobuf


How is it not going to work? If we're just writing serialized data directly to the connection, then we're broken for MsgPack too, if one node is using JSON and the other is using MsgPack. If we're serializing separately for every connection, then it should work fine with JsonSerializer customization and Protobuf because each send uses the client's configured serializers.

I derped and commented on outdated stuff. Disregard

analogrelay · 2017-08-22T17:32:54Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

+        public override Task InvokeAllExceptAsync(string methodName, object[] args, IReadOnlyList<string> excludedIds)
+        {
+            var message = new InvocationMessage(GetInvocationId(), nonBlocking: true, target: methodName, arguments: args);
+            var redisExcludeMessage = new RedisExcludeClientMessage(message, excludedIds);


Hrm... feels like a a premature optimization, but I guess we can do it.

analogrelay · 2017-08-22T17:33:21Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

+                HubMessage = message;
+                ExcludedIds = ids;
+            }
+        }


need an extra line

analogrelay · 2017-08-22T17:34:09Z

src/Microsoft.AspNetCore.SignalR/DefaultHubLifetimeManager.cs

+        {
+            return InvokeAllWhere(methodName, args, connection =>
+            {
+                var excludedIdsSet = new HashSet<string>(excludedIds);


Yeah, this will create a new HashSet for every check. If we decide we need a HashSet for optimization, we would want to initialize it outside the lambda so there's only one instance.

analogrelay · 2017-08-22T17:45:26Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

+                var message = DeserializeMessage<RedisExcludeClientsMessage>(data);
+                var excludedIds = message.ExcludedIds;
+
+                //// TODO: This isn't going to work when we allow JsonSerializer customization or add Protobuf


Can you clarify how this is broken with JsonSerializer customization and Protobuf? I'd think it'd already be broken for MsgPack if that was the case. We should be re-serializing for each client in the current implementation, right? It's slower, but guarantees it's correct and should work fine with serializer customization.

This comment is a copy pasta result. Not sure tbh

super nit: the comment has too many ////

moozzyk · 2017-08-22T17:56:46Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

+            _logger.LogInformation("Subscribing to channel: {channel}", channelName);
+            _bus.Subscribe(channelName, async (c, data) =>
+            {
+                await previousBroadcastTask;


Is this correct to use the same task as broadcast?

Oh yeah. This doesn't look good.

moozzyk · 2017-08-22T17:59:12Z

src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs

+                    }
+                }
+
+                previousBroadcastTask = Task.WhenAll(tasks);


I think this is wrong/racey - you can replace a task that has not been finished with another task that is not finished. Sharing the same task between broadcast and .allexcept makes it even more racey

mikaelm12 · 2017-08-22T23:07:45Z

If there's no further discussion I'm gonna merge this eod

mikaelm12 requested review from analogrelay, davidfowl, moozzyk and BrennanConroy August 9, 2017 22:08

dnfclas added the cla-already-signed label Aug 9, 2017

moozzyk reviewed Aug 9, 2017

View reviewed changes

BrennanConroy reviewed Aug 10, 2017

View reviewed changes

analogrelay suggested changes Aug 11, 2017

View reviewed changes

mikaelm12 changed the title ~~WIP Client Subset - AllExcept~~ Client Subset - AllExcept Aug 17, 2017

mikaelm12 added 2 commits August 17, 2017 12:07

IHubClients Property: AllExcept

13985c8

Rebase on dev - react to ReadAsync method name change

2b7b869

mikaelm12 force-pushed the mikaelm12/ClientSubsets branch from 233374a to 2b7b869 Compare August 17, 2017 19:19

BrennanConroy suggested changes Aug 18, 2017

View reviewed changes

mikaelm12 added 2 commits August 18, 2017 12:12

RedisExcludeClientMessage

6c2b74f

PR comments

f710817

BrennanConroy reviewed Aug 18, 2017

View reviewed changes

PR feedback

3287a6c

BrennanConroy reviewed Aug 21, 2017

View reviewed changes

PR feedback

6f14009

BrennanConroy approved these changes Aug 21, 2017

View reviewed changes

PR comments

20a9a49

mikaelm12 mentioned this pull request Aug 21, 2017

AllExcept #746

Closed

analogrelay reviewed Aug 22, 2017

View reviewed changes

moozzyk suggested changes Aug 22, 2017

View reviewed changes

Pr comments

534406c

Pr comment

0c0c154

mikaelm12 merged commit d469cc3 into dev Aug 23, 2017

mikaelm12 deleted the mikaelm12/ClientSubsets branch September 29, 2017 23:51

moozzyk mentioned this pull request Oct 11, 2017

AllExcept params #1007

Closed

		@@ -491,7 +491,6 @@ public void HubsCannotHaveOverloadedMethods()
		Task secondEndPointTask = endPoint.OnConnectedAsync(secondClient.Connection);

		@@ -20,6 +22,11 @@ public HubContext(HubLifetimeManager<THub> lifetimeManager)

		public virtual IGroupManager Groups { get; }

		public IClientProxy AllExcept(List<string> excludedIds)

		@@ -13,6 +14,8 @@ public abstract class HubLifetimeManager<THub>

		public abstract Task InvokeAllAsync(string methodName, object[] args);

		public abstract Task InvokeAllExceptAsync(List<string> excludedIds, string methodName, object[] args);

		@@ -20,6 +22,11 @@ public HubContext(HubLifetimeManager<THub> lifetimeManager)

		public virtual IGroupManager Groups { get; }

		public IClientProxy AllExcept(IReadOnlyList<string> excludedIds)

Client Subset - AllExcept #700

Client Subset - AllExcept #700

Conversation

mikaelm12 commented Aug 9, 2017

dnfclas commented Aug 9, 2017

moozzyk 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

mikaelm12 Aug 16, 2017 • edited Loading

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

BrennanConroy commented Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPixel commented Aug 11, 2017

analogrelay 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

KPixel Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

analogrelay Aug 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelm12 commented Aug 16, 2017

mikaelm12 commented Aug 17, 2017

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

mikaelm12 commented Aug 18, 2017

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

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

mikaelm12 commented Aug 22, 2017

mikaelm12 Aug 16, 2017 •

edited

Loading

BrennanConroy commented Aug 10, 2017 •

edited

Loading

KPixel Aug 11, 2017 •

edited

Loading

analogrelay Aug 12, 2017 •

edited

Loading