Skip to content

Commit 5e2b43c

Browse files
authored
Misc connection/subscription fixes (#2001)
In investigating an issue in #1989, I found a few gaps. Overall: 1. Twemproxy has an out of date CommandMap, which propagated to Envoy. 2. We were expecting both interactive and subscription connections to complete to complete the async handler...but we shouldn't because subscriptions may be disabled. 3. RedisSubscriber changes on the sync path weren't validating the message (asserting the command map has it enabled). This fixes all of the above and adds another test considering all 3.
1 parent 1faa51d commit 5e2b43c

File tree

6 files changed

+25
-12
lines changed

6 files changed

+25
-12
lines changed

docs/ReleaseNotes.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
## Unreleased
44
- Adds bounds checking for `ExponentialRetry` backoff policy (#1921 via gliljas)
5+
- When `SUBSCRIBE` is disabled, give proper errors and connect faster (#2001 via NickCraver)
56

67
## 2.5.27 (prerelease)
78

src/StackExchange.Redis/CommandMap.cs

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -26,25 +26,19 @@ public sealed class CommandMap
2626
{
2727
// see https://github.com/twitter/twemproxy/blob/master/notes/redis.md
2828
RedisCommand.KEYS, RedisCommand.MIGRATE, RedisCommand.MOVE, RedisCommand.OBJECT, RedisCommand.RANDOMKEY,
29-
RedisCommand.RENAME, RedisCommand.RENAMENX, RedisCommand.SORT, RedisCommand.SCAN,
29+
RedisCommand.RENAME, RedisCommand.RENAMENX, RedisCommand.SCAN,
3030

31-
RedisCommand.BITOP, RedisCommand.MSET, RedisCommand.MSETNX,
32-
33-
RedisCommand.HSCAN,
31+
RedisCommand.BITOP, RedisCommand.MSETNX,
3432

3533
RedisCommand.BLPOP, RedisCommand.BRPOP, RedisCommand.BRPOPLPUSH, // yeah, me neither!
3634

37-
RedisCommand.SSCAN,
38-
39-
RedisCommand.ZSCAN,
40-
4135
RedisCommand.PSUBSCRIBE, RedisCommand.PUBLISH, RedisCommand.PUNSUBSCRIBE, RedisCommand.SUBSCRIBE, RedisCommand.UNSUBSCRIBE,
4236

4337
RedisCommand.DISCARD, RedisCommand.EXEC, RedisCommand.MULTI, RedisCommand.UNWATCH, RedisCommand.WATCH,
4438

4539
RedisCommand.SCRIPT,
4640

47-
RedisCommand.ECHO, RedisCommand.PING, RedisCommand.QUIT, RedisCommand.SELECT,
41+
RedisCommand.ECHO, RedisCommand.PING, RedisCommand.SELECT,
4842

4943
RedisCommand.BGREWRITEAOF, RedisCommand.BGSAVE, RedisCommand.CLIENT, RedisCommand.CLUSTER, RedisCommand.CONFIG, RedisCommand.DBSIZE,
5044
RedisCommand.DEBUG, RedisCommand.FLUSHALL, RedisCommand.FLUSHDB, RedisCommand.INFO, RedisCommand.LASTSAVE, RedisCommand.MONITOR, RedisCommand.REPLICAOF,

src/StackExchange.Redis/ConnectionMultiplexer.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1654,7 +1654,7 @@ private void ActivateAllServers(LogProxy log)
16541654
foreach (var server in GetServerSnapshot())
16551655
{
16561656
server.Activate(ConnectionType.Interactive, log);
1657-
if (CommandMap.IsAvailable(RedisCommand.SUBSCRIBE))
1657+
if (server.SupportsSubscriptions)
16581658
{
16591659
// Intentionally not logging the sub connection
16601660
server.Activate(ConnectionType.Subscription, null);

src/StackExchange.Redis/RedisSubscriber.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -378,7 +378,7 @@ internal bool EnsureSubscribedToServer(Subscription sub, RedisChannel channel, C
378378
sub.SetCurrentServer(null); // we're not appropriately connected, so blank it out for eligible reconnection
379379
var message = sub.GetMessage(channel, SubscriptionAction.Subscribe, flags, internalCall);
380380
var selected = multiplexer.SelectServer(message);
381-
return multiplexer.ExecuteSyncImpl(message, sub.Processor, selected);
381+
return ExecuteSync(message, sub.Processor, selected);
382382
}
383383

384384
Task ISubscriber.SubscribeAsync(RedisChannel channel, Action<RedisChannel, RedisValue> handler, CommandFlags flags)

src/StackExchange.Redis/ServerEndPoint.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,8 @@ public ServerEndPoint(ConnectionMultiplexer multiplexer, EndPoint endpoint)
7575
public bool IsConnected => interactive?.IsConnected == true;
7676
public bool IsSubscriberConnected => subscription?.IsConnected == true;
7777

78+
public bool SupportsSubscriptions => Multiplexer.CommandMap.IsAvailable(RedisCommand.SUBSCRIBE);
79+
7880
public bool IsConnecting => interactive?.IsConnecting == true;
7981

8082
private readonly List<TaskCompletionSource<string>> _pendingConnectionMonitors = new List<TaskCompletionSource<string>>();
@@ -629,9 +631,10 @@ internal void OnFullyEstablished(PhysicalConnection connection, string source)
629631
// Since we're issuing commands inside a SetResult path in a message, we'd create a deadlock by waiting.
630632
Multiplexer.EnsureSubscriptions(CommandFlags.FireAndForget);
631633
}
632-
if (IsConnected && IsSubscriberConnected)
634+
if (IsConnected && (IsSubscriberConnected || !SupportsSubscriptions))
633635
{
634636
// Only connect on the second leg - we can accomplish this by checking both
637+
// Or the first leg, if we're only making 1 connection because subscriptions aren't supported
635638
CompletePendingConnectionMonitors(source);
636639
}
637640

tests/StackExchange.Redis.Tests/Config.cs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,21 @@ public void ReadConfigWithConfigDisabled()
263263
}
264264
}
265265

266+
[Fact]
267+
public void ConnectWithSubscribeDisabled()
268+
{
269+
using (var muxer = Create(allowAdmin: true, disabledCommands: new[] { "subscribe" }))
270+
{
271+
Assert.True(muxer.IsConnected);
272+
var servers = muxer.GetServerSnapshot();
273+
Assert.True(servers[0].IsConnected);
274+
Assert.False(servers[0].IsSubscriberConnected);
275+
276+
var ex = Assert.Throws<RedisCommandException>(() => muxer.GetSubscriber().Subscribe(Me(), (_, _) => GC.KeepAlive(this)));
277+
Assert.Equal("This operation has been disabled in the command-map and cannot be used: SUBSCRIBE", ex.Message);
278+
}
279+
}
280+
266281
[Fact]
267282
public void ReadConfig()
268283
{

0 commit comments

Comments
 (0)