From 9cc5d13a40ea4960896a28cee195f470df6874b4 Mon Sep 17 00:00:00 2001
From: BrennanConroy <brecon@microsoft.com>
Date: Fri, 20 Oct 2017 14:29:08 -0700
Subject: [PATCH] [Redis] Adding same group to connection multiple times should
 NOP (#1040)

* Added some additional tests too
---
 .../RedisHubLifetimeManager.cs                |   6 +-
 .../RedisHubLifetimeManagerTests.cs           | 147 ++++++++++++++++++
 2 files changed, 152 insertions(+), 1 deletion(-)

diff --git a/src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs b/src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs
index bfeed38163..cd62c986e1 100644
--- a/src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs
+++ b/src/Microsoft.AspNetCore.SignalR.Redis/RedisHubLifetimeManager.cs
@@ -338,7 +338,11 @@ namespace Microsoft.AspNetCore.SignalR.Redis
 
             lock (groupNames)
             {
-                groupNames.Add(groupName);
+                // Connection already in group
+                if (!groupNames.Add(groupName))
+                {
+                    return;
+                }
             }
 
             var groupChannel = _channelNamePrefix + ".group." + groupName;
diff --git a/test/Microsoft.AspNetCore.SignalR.Redis.Tests/RedisHubLifetimeManagerTests.cs b/test/Microsoft.AspNetCore.SignalR.Redis.Tests/RedisHubLifetimeManagerTests.cs
index b035ead1e6..b8c038946c 100644
--- a/test/Microsoft.AspNetCore.SignalR.Redis.Tests/RedisHubLifetimeManagerTests.cs
+++ b/test/Microsoft.AspNetCore.SignalR.Redis.Tests/RedisHubLifetimeManagerTests.cs
@@ -334,6 +334,153 @@ namespace Microsoft.AspNetCore.SignalR.Redis.Tests
             }
         }
 
+        [Fact]
+        public async Task AddGroupAsyncForConnectionOnDifferentServerWorks()
+        {
+            var manager1 = new RedisHubLifetimeManager<MyHub>(new LoggerFactory().CreateLogger<RedisHubLifetimeManager<MyHub>>(), Options.Create(new RedisOptions()
+            {
+                Factory = t => new TestConnectionMultiplexer()
+            }));
+            var manager2 = new RedisHubLifetimeManager<MyHub>(new LoggerFactory().CreateLogger<RedisHubLifetimeManager<MyHub>>(), Options.Create(new RedisOptions()
+            {
+                Factory = t => new TestConnectionMultiplexer()
+            }));
+
+            using (var client = new TestClient())
+            {
+                var output = Channel.CreateUnbounded<HubMessage>();
+
+                var connection = new HubConnectionContext(output, client.Connection);
+
+                await manager1.OnConnectedAsync(connection).OrTimeout();
+
+                await manager2.AddGroupAsync(connection.ConnectionId, "name").OrTimeout();
+
+                await manager2.InvokeGroupAsync("name", "Hello", new object[] { "World" }).OrTimeout();
+
+                AssertMessage(output);
+            }
+        }
+
+        [Fact]
+        public async Task AddGroupAsyncForLocalConnectionAlreadyInGroupDoesNothing()
+        {
+            var manager = new RedisHubLifetimeManager<MyHub>(new LoggerFactory().CreateLogger<RedisHubLifetimeManager<MyHub>>(), Options.Create(new RedisOptions()
+            {
+                Factory = t => new TestConnectionMultiplexer()
+            }));
+
+            using (var client = new TestClient())
+            {
+                var output = Channel.CreateUnbounded<HubMessage>();
+
+                var connection = new HubConnectionContext(output, client.Connection);
+
+                await manager.OnConnectedAsync(connection).OrTimeout();
+
+                await manager.AddGroupAsync(connection.ConnectionId, "name").OrTimeout();
+                await manager.AddGroupAsync(connection.ConnectionId, "name").OrTimeout();
+
+                await manager.InvokeGroupAsync("name", "Hello", new object[] { "World" }).OrTimeout();
+
+                AssertMessage(output);
+                Assert.False(output.In.TryRead(out var item));
+            }
+        }
+
+        [Fact]
+        public async Task AddGroupAsyncForConnectionOnDifferentServerAlreadyInGroupDoesNothing()
+        {
+            var manager1 = new RedisHubLifetimeManager<MyHub>(new LoggerFactory().CreateLogger<RedisHubLifetimeManager<MyHub>>(), Options.Create(new RedisOptions()
+            {
+                Factory = t => new TestConnectionMultiplexer()
+            }));
+            var manager2 = new RedisHubLifetimeManager<MyHub>(new LoggerFactory().CreateLogger<RedisHubLifetimeManager<MyHub>>(), Options.Create(new RedisOptions()
+            {
+                Factory = t => new TestConnectionMultiplexer()
+            }));
+
+            using (var client = new TestClient())
+            {
+                var output = Channel.CreateUnbounded<HubMessage>();
+
+                var connection = new HubConnectionContext(output, client.Connection);
+
+                await manager1.OnConnectedAsync(connection).OrTimeout();
+
+                await manager1.AddGroupAsync(connection.ConnectionId, "name").OrTimeout();
+                await manager2.AddGroupAsync(connection.ConnectionId, "name").OrTimeout();
+
+                await manager2.InvokeGroupAsync("name", "Hello", new object[] { "World" }).OrTimeout();
+
+                AssertMessage(output);
+                Assert.False(output.In.TryRead(out var item));
+            }
+        }
+
+        [Fact]
+        public async Task RemoveGroupAsyncForConnectionOnDifferentServerWorks()
+        {
+            var manager1 = new RedisHubLifetimeManager<MyHub>(new LoggerFactory().CreateLogger<RedisHubLifetimeManager<MyHub>>(), Options.Create(new RedisOptions()
+            {
+                Factory = t => new TestConnectionMultiplexer()
+            }));
+            var manager2 = new RedisHubLifetimeManager<MyHub>(new LoggerFactory().CreateLogger<RedisHubLifetimeManager<MyHub>>(), Options.Create(new RedisOptions()
+            {
+                Factory = t => new TestConnectionMultiplexer()
+            }));
+
+            using (var client = new TestClient())
+            {
+                var output = Channel.CreateUnbounded<HubMessage>();
+
+                var connection = new HubConnectionContext(output, client.Connection);
+
+                await manager1.OnConnectedAsync(connection).OrTimeout();
+
+                await manager1.AddGroupAsync(connection.ConnectionId, "name").OrTimeout();
+
+                await manager2.InvokeGroupAsync("name", "Hello", new object[] { "World" }).OrTimeout();
+
+                AssertMessage(output);
+
+                await manager2.RemoveGroupAsync(connection.ConnectionId, "name").OrTimeout();
+
+                await manager2.InvokeGroupAsync("name", "Hello", new object[] { "World" }).OrTimeout();
+
+                Assert.False(output.In.TryRead(out var item));
+            }
+        }
+
+        [Fact]
+        public async Task InvokeConnectionAsyncForLocalConnectionDoesNotPublishToRedis()
+        {
+            var manager1 = new RedisHubLifetimeManager<MyHub>(new LoggerFactory().CreateLogger<RedisHubLifetimeManager<MyHub>>(), Options.Create(new RedisOptions()
+            {
+                Factory = t => new TestConnectionMultiplexer()
+            }));
+            var manager2 = new RedisHubLifetimeManager<MyHub>(new LoggerFactory().CreateLogger<RedisHubLifetimeManager<MyHub>>(), Options.Create(new RedisOptions()
+            {
+                Factory = t => new TestConnectionMultiplexer()
+            }));
+
+            using (var client = new TestClient())
+            {
+                var output = Channel.CreateUnbounded<HubMessage>();
+
+                var connection = new HubConnectionContext(output, client.Connection);
+
+                // Add connection to both "servers" to see if connection receives message twice
+                await manager1.OnConnectedAsync(connection).OrTimeout();
+                await manager2.OnConnectedAsync(connection).OrTimeout();
+
+                await manager1.InvokeConnectionAsync(connection.ConnectionId, "Hello", new object[] { "World" }).OrTimeout();
+
+                AssertMessage(output);
+                Assert.False(output.In.TryRead(out var item));
+            }
+        }
+
         private void AssertMessage(Channel<HubMessage> channel)
         {
             Assert.True(channel.In.TryRead(out var item));