From 990f5e867c33eb5221ff0a5ea2e6a9422512232a Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 10 Jan 2022 08:59:46 -0500 Subject: [PATCH 01/56] WIP: Pub/Sub portion of #1912 We're working on pub/sub - breaking it out explicitly. --- src/StackExchange.Redis/RedisSubscriber.cs | 57 ++++++------- src/StackExchange.Redis/ServerEndPoint.cs | 16 ++-- tests/StackExchange.Redis.Tests/PubSub.cs | 93 +++++++++++++++++++--- 3 files changed, 120 insertions(+), 46 deletions(-) diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 22b9664b7..3acf60fde 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -13,6 +13,14 @@ public partial class ConnectionMultiplexer { private readonly Dictionary subscriptions = new Dictionary(); + internal int GetSubscriptionsCount() + { + lock (subscriptions) + { + return subscriptions.Count; + } + } + internal static void CompleteAsWorker(ICompletable completable) { if (completable != null) ThreadPool.QueueUserWorkItem(s_CompleteAsWorker, completable); @@ -166,11 +174,10 @@ internal void ResendSubscriptions(ServerEndPoint server) internal bool SubscriberConnected(in RedisChannel channel = default(RedisChannel)) { - var server = GetSubscribedServer(channel); - if (server != null) return server.IsConnected; + // TODO: default(RedisKey) is incorrect here - should shard based on the channel in cluster + var server = GetSubscribedServer(channel) ?? SelectServer(RedisCommand.SUBSCRIBE, CommandFlags.DemandMaster, default(RedisKey)); - server = SelectServer(RedisCommand.SUBSCRIBE, CommandFlags.DemandMaster, default(RedisKey)); - return server?.IsConnected == true; + return server?.IsConnected == true && server.IsSubscriberConnected; } internal long ValidateSubscriptions() @@ -221,6 +228,7 @@ public bool Remove(Action handler, ChannelMessageQueue [System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "RCS1210:Return completed task instead of returning null.", Justification = "Intentional for efficient success check")] public Task SubscribeToServer(ConnectionMultiplexer multiplexer, in RedisChannel channel, CommandFlags flags, object asyncState, bool internalCall) { + // TODO: default(RedisKey) is incorrect here - should shard based on the channel in cluster var selected = multiplexer.SelectServer(RedisCommand.SUBSCRIBE, flags, default(RedisKey)); var bridge = selected?.GetBridge(ConnectionType.Subscription, true); if (bridge == null) return null; @@ -299,14 +307,13 @@ private PendingSubscriptionState(object asyncState, RedisChannel channel, Subscr internal void Resubscribe(in RedisChannel channel, ServerEndPoint server) { - if (server != null && Interlocked.CompareExchange(ref owner, server, server) == server) + // Only re-subscribe to the original server + if (server != null && GetOwner() == server) { var cmd = channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE; var msg = Message.Create(-1, CommandFlags.FireAndForget, cmd, channel); msg.SetInternalCall(); -#pragma warning disable CS0618 - server.WriteDirectFireAndForgetSync(msg, ResultProcessor.TrackSubscriptions); -#pragma warning restore CS0618 + server.Multiplexer.ExecuteSyncImpl(msg, ResultProcessor.TrackSubscriptions, server); } } @@ -422,36 +429,24 @@ public Task IdentifyEndpointAsync(RedisChannel channel, CommandFlags f public override TimeSpan Ping(CommandFlags flags = CommandFlags.None) { - var msg = CreatePingMessage(flags, out var server); - return ExecuteSync(msg, ResultProcessor.ResponseTimer, server); + var msg = CreatePingMessage(flags); + return ExecuteSync(msg, ResultProcessor.ResponseTimer); } public override Task PingAsync(CommandFlags flags = CommandFlags.None) { - var msg = CreatePingMessage(flags, out var server); - return ExecuteAsync(msg, ResultProcessor.ResponseTimer, server); + var msg = CreatePingMessage(flags); + return ExecuteAsync(msg, ResultProcessor.ResponseTimer); } - private Message CreatePingMessage(CommandFlags flags, out ServerEndPoint server) + private Message CreatePingMessage(CommandFlags flags) { - bool usePing = false; - server = null; - if (multiplexer.CommandMap.IsAvailable(RedisCommand.PING)) - { - try { usePing = GetFeatures(default, flags, out server).PingOnSubscriber; } - catch { } - } - - if (usePing) - { - return ResultProcessor.TimingProcessor.CreateMessage(-1, flags, RedisCommand.PING); - } - else - { - // can't use regular PING, but we can unsubscribe from something random that we weren't even subscribed to... - RedisValue channel = multiplexer.UniqueId; - return ResultProcessor.TimingProcessor.CreateMessage(-1, flags, RedisCommand.UNSUBSCRIBE, channel); - } + // We're explicitly NOT using PING here because GetBridge() would send this over the interactive connection + // rather than the subscription connection we intend. + RedisValue channel = multiplexer.UniqueId; + var message = ResultProcessor.TimingProcessor.CreateMessage(-1, flags, RedisCommand.UNSUBSCRIBE, channel); + message.SetInternalCall(); + return message; } public long Publish(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None) diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs index f41e360a3..b24d43065 100755 --- a/src/StackExchange.Redis/ServerEndPoint.cs +++ b/src/StackExchange.Redis/ServerEndPoint.cs @@ -74,6 +74,8 @@ public ServerEndPoint(ConnectionMultiplexer multiplexer, EndPoint endpoint) public bool IsConnected => interactive?.IsConnected == true; + public bool IsSubscriberConnected => subscription?.IsConnected == true; + public bool IsConnecting => interactive?.IsConnecting == true; private readonly List> _pendingConnectionMonitors = new List>(); @@ -92,7 +94,7 @@ async Task IfConnectedAsync(LogProxy log, bool sendTracerIfConnected, bo } if (sendTracerIfConnected) { - await SendTracer(log).ForAwait(); + await SendTracerAsync(log).ForAwait(); } log?.WriteLine($"{Format.ToString(this)}: OnConnectedAsync already connected end"); return "Already connected"; @@ -686,7 +688,7 @@ internal void OnHeartbeat() } } - internal Task WriteDirectAsync(Message message, ResultProcessor processor, object asyncState = null, PhysicalBridge bridge = null) + internal Task WriteDirectAsync(Message message, ResultProcessor processor, PhysicalBridge bridge = null) { static async Task Awaited(ServerEndPoint @this, Message message, ValueTask write, TaskCompletionSource tcs) { @@ -699,7 +701,7 @@ static async Task Awaited(ServerEndPoint @this, Message message, ValueTask.Create(out var tcs, asyncState); + var source = TaskResultBox.Create(out var tcs, null); message.SetSource(processor, source); if (bridge == null) bridge = GetBridge(message.Command); @@ -743,7 +745,7 @@ internal void ReportNextFailure() subscription?.ReportNextFailure(); } - internal Task SendTracer(LogProxy log = null) + internal Task SendTracerAsync(LogProxy log = null) { var msg = GetTracerMessage(false); msg = LoggingMessage.Create(log, msg); @@ -783,6 +785,9 @@ internal string Summary() return sb.ToString(); } + /// + /// Write the message directly to the pipe or fail...will not queue. + /// internal ValueTask WriteDirectOrQueueFireAndForgetAsync(PhysicalConnection connection, Message message, ResultProcessor processor) { static async ValueTask Awaited(ValueTask l_result) => await l_result.ForAwait(); @@ -886,7 +891,7 @@ private async Task HandshakeAsync(PhysicalConnection connection, LogProxy log) log?.WriteLine($"{Format.ToString(this)}: Sending critical tracer (handshake): {tracer.CommandAndKey}"); await WriteDirectOrQueueFireAndForgetAsync(connection, tracer, ResultProcessor.EstablishConnection).ForAwait(); - // note: this **must** be the last thing on the subscription handshake, because after this + // Note: this **must** be the last thing on the subscription handshake, because after this // we will be in subscriber mode: regular commands cannot be sent if (connType == ConnectionType.Subscription) { @@ -894,6 +899,7 @@ private async Task HandshakeAsync(PhysicalConnection connection, LogProxy log) if (configChannel != null) { msg = Message.Create(-1, CommandFlags.FireAndForget, RedisCommand.SUBSCRIBE, (RedisChannel)configChannel); + // Note: this is NOT internal, we want it to queue in a backlog for sending when ready if necessary await WriteDirectOrQueueFireAndForgetAsync(connection, msg, ResultProcessor.TrackSubscriptions).ForAwait(); } } diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 0e4131913..9c3d264da 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -1,11 +1,13 @@ using System; using System.Collections.Generic; using System.Diagnostics; +using System.Linq; using System.Text; using System.Threading; using System.Threading.Channels; using System.Threading.Tasks; using StackExchange.Redis.Maintenance; +using StackExchange.Redis.Profiling; using Xunit; using Xunit.Abstractions; // ReSharper disable AccessToModifiedClosure @@ -520,6 +522,9 @@ public async Task PubSubGetAllCorrectOrder_OnMessage_Async() }); await sub.PingAsync().ForAwait(); + // Give a delay between subscriptions and when we try to publish to be safe + await Task.Delay(1000).ForAwait(); + lock (syncLock) { for (int i = 0; i < count; i++) @@ -743,8 +748,10 @@ public async Task AzureRedisEventsAutomaticSubscribe() [Fact] public async Task SubscriptionsSurviveConnectionFailureAsync() { - using (var muxer = Create(allowAdmin: true, shared: false)) + var session = new ProfilingSession(); + using (var muxer = Create(allowAdmin: true, shared: false, syncTimeout: 1000) as ConnectionMultiplexer) { + muxer.RegisterProfiler(() => session); RedisChannel channel = Me(); var sub = muxer.GetSubscriber(); int counter = 0; @@ -752,23 +759,89 @@ await sub.SubscribeAsync(channel, delegate { Interlocked.Increment(ref counter); }).ConfigureAwait(false); + + var profile1 = session.FinishProfiling(); + foreach (var command in profile1) + { + Log($"{command.EndPoint}: {command}"); + } + // We shouldn't see the initial connection here + Assert.Equal(0, profile1.Count(p => p.Command == nameof(RedisCommand.SUBSCRIBE))); + + Assert.Equal(1, muxer.GetSubscriptionsCount()); + await Task.Delay(200).ConfigureAwait(false); + await sub.PublishAsync(channel, "abc").ConfigureAwait(false); sub.Ping(); await Task.Delay(200).ConfigureAwait(false); - Assert.Equal(1, Thread.VolatileRead(ref counter)); + + var counter1 = Thread.VolatileRead(ref counter); + Log($"Expecting 1 messsage, got {counter1}"); + Assert.Equal(1, counter1); + var server = GetServer(muxer); - Assert.Equal(1, server.GetCounters().Subscription.SocketCount); + var socketCount = server.GetCounters().Subscription.SocketCount; + Log($"Expecting 1 socket, got {socketCount}"); + Assert.Equal(1, socketCount); + // We might fail both connections or just the primary in the time period + SetExpectedAmbientFailureCount(-1); + + // Make sure we fail all the way + muxer.AllowConnect = false; + Log("Failing connection"); + // Fail all connections server.SimulateConnectionFailure(SimulatedFailureType.All); - SetExpectedAmbientFailureCount(2); - await Task.Delay(200).ConfigureAwait(false); - sub.Ping(); - Assert.Equal(2, server.GetCounters().Subscription.SocketCount); - await sub.PublishAsync(channel, "abc").ConfigureAwait(false); - await Task.Delay(200).ConfigureAwait(false); + // Trigger failure + Assert.Throws(() => sub.Ping()); + Assert.False(sub.IsConnected(channel)); + + // Now reconnect... + muxer.AllowConnect = true; + Log("Waiting on reconnect"); + // Wait until we're reconnected + await UntilCondition(TimeSpan.FromSeconds(10), () => sub.IsConnected(channel)); + Log("Reconnected"); + // Ensure we're reconnected + Assert.True(sub.IsConnected(channel)); + + // And time to resubscribe... + await Task.Delay(1000).ConfigureAwait(false); + + // Ensure we've sent the subscribe command after reconnecting + var profile2 = session.FinishProfiling(); + foreach (var command in profile2) + { + Log($"{command.EndPoint}: {command}"); + } + //Assert.Equal(1, profile2.Count(p => p.Command == nameof(RedisCommand.SUBSCRIBE))); + + Log($"Issuing ping after reconnected"); sub.Ping(); - Assert.Equal(2, Thread.VolatileRead(ref counter)); + Assert.Equal(1, muxer.GetSubscriptionsCount()); + + Log("Publishing"); + var published = await sub.PublishAsync(channel, "abc").ConfigureAwait(false); + + Log($"Published to {published} subscriber(s)."); + Assert.Equal(1, published); + + // Give it a few seconds to get our messages + Log("Waiting for 2 messages"); + await UntilCondition(TimeSpan.FromSeconds(5), () => Thread.VolatileRead(ref counter) == 2); + + var counter2 = Thread.VolatileRead(ref counter); + Log($"Expecting 2 messsages, got {counter2}"); + Assert.Equal(2, counter2); + + // Log all commands at the end + Log("All commands since connecting:"); + var profile3 = session.FinishProfiling(); + foreach (var command in profile3) + { + Log($"{command.EndPoint}: {command}"); + } } } } From d552097e39465e6776293d7958c5116bc96f6b1c Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 10 Jan 2022 10:45:05 -0500 Subject: [PATCH 02/56] Lots of things - need to writeup in PR --- .../ConfigurationOptions.cs | 2 +- .../ConnectionMultiplexer.cs | 2 +- src/StackExchange.Redis/Enums/CommandFlags.cs | 2 + src/StackExchange.Redis/ExceptionFactory.cs | 4 +- src/StackExchange.Redis/Message.cs | 12 +++++- src/StackExchange.Redis/RedisBatch.cs | 2 +- src/StackExchange.Redis/RedisSubscriber.cs | 27 ++++++++++--- src/StackExchange.Redis/ServerEndPoint.cs | 40 ++++++++++++++----- .../ConnectingFailDetection.cs | 2 +- tests/StackExchange.Redis.Tests/PubSub.cs | 2 +- tests/StackExchange.Redis.Tests/TestBase.cs | 5 +++ 11 files changed, 77 insertions(+), 23 deletions(-) diff --git a/src/StackExchange.Redis/ConfigurationOptions.cs b/src/StackExchange.Redis/ConfigurationOptions.cs index abc12b579..ddc53f1a8 100644 --- a/src/StackExchange.Redis/ConfigurationOptions.cs +++ b/src/StackExchange.Redis/ConfigurationOptions.cs @@ -280,7 +280,7 @@ public int ConnectTimeout /// /// The server version to assume /// - public Version DefaultVersion { get { return defaultVersion ?? (IsAzureEndpoint() ? RedisFeatures.v4_0_0 : RedisFeatures.v2_8_0); } set { defaultVersion = value; } } + public Version DefaultVersion { get { return defaultVersion ?? (IsAzureEndpoint() ? RedisFeatures.v4_0_0 : RedisFeatures.v3_0_0); } set { defaultVersion = value; } } /// /// The endpoints defined for this configuration diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.cs b/src/StackExchange.Redis/ConnectionMultiplexer.cs index 055ea3d12..34591b465 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.cs @@ -1773,7 +1773,7 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, LogP { var server = servers[i]; var task = available[i]; - var bs = server.GetBridgeStatus(RedisCommand.PING); + var bs = server.GetBridgeStatus(ConnectionType.Interactive); log?.WriteLine($" Server[{i}] ({Format.ToString(server)}) Status: {task.Status} (inst: {bs.MessagesSinceLastHeartbeat}, qs: {bs.Connection.MessagesSentAwaitingResponse}, in: {bs.Connection.BytesAvailableOnSocket}, qu: {bs.MessagesSinceLastHeartbeat}, aw: {bs.IsWriterActive}, in-pipe: {bs.Connection.BytesInReadPipe}, out-pipe: {bs.Connection.BytesInWritePipe}, bw: {bs.BacklogStatus}, rs: {bs.Connection.ReadStatus}. ws: {bs.Connection.WriteStatus})"); } diff --git a/src/StackExchange.Redis/Enums/CommandFlags.cs b/src/StackExchange.Redis/Enums/CommandFlags.cs index f0a670d76..c1efc65c1 100644 --- a/src/StackExchange.Redis/Enums/CommandFlags.cs +++ b/src/StackExchange.Redis/Enums/CommandFlags.cs @@ -82,5 +82,7 @@ public enum CommandFlags NoScriptCache = 512, // 1024: used for timed-out; never user-specified, so not visible on the public API + + // 2048: Use subscription connection type; never user-specified, so not visible on the public API } } diff --git a/src/StackExchange.Redis/ExceptionFactory.cs b/src/StackExchange.Redis/ExceptionFactory.cs index 4cc274d24..fe7aabc3c 100644 --- a/src/StackExchange.Redis/ExceptionFactory.cs +++ b/src/StackExchange.Redis/ExceptionFactory.cs @@ -312,7 +312,7 @@ ServerEndPoint server // Add server data, if we have it if (server != null && message != null) { - var bs = server.GetBridgeStatus(message.Command); + var bs = server.GetBridgeStatus(message.IsForSubscriptionBridge ? ConnectionType.Subscription: ConnectionType.Interactive); switch (bs.Connection.ReadStatus) { @@ -338,7 +338,7 @@ ServerEndPoint server if (multiplexer.StormLogThreshold >= 0 && bs.Connection.MessagesSentAwaitingResponse >= multiplexer.StormLogThreshold && Interlocked.CompareExchange(ref multiplexer.haveStormLog, 1, 0) == 0) { - var log = server.GetStormLog(message.Command); + var log = server.GetStormLog(message); if (string.IsNullOrWhiteSpace(log)) Interlocked.Exchange(ref multiplexer.haveStormLog, 0); else Interlocked.Exchange(ref multiplexer.stormLogSnapshot, log); } diff --git a/src/StackExchange.Redis/Message.cs b/src/StackExchange.Redis/Message.cs index c8fdf54f8..05c1f56fb 100644 --- a/src/StackExchange.Redis/Message.cs +++ b/src/StackExchange.Redis/Message.cs @@ -74,7 +74,8 @@ internal void SetBacklogState(int position, PhysicalConnection physical) private const CommandFlags AskingFlag = (CommandFlags)32, ScriptUnavailableFlag = (CommandFlags)256, - NeedsAsyncTimeoutCheckFlag = (CommandFlags)1024; + NeedsAsyncTimeoutCheckFlag = (CommandFlags)1024, + DemandSubscriptionConnection = (CommandFlags)2048; private const CommandFlags MaskMasterServerPreference = CommandFlags.DemandMaster | CommandFlags.DemandReplica @@ -705,6 +706,15 @@ internal void SetWriteTime() private int _writeTickCount; public int GetWriteTime() => Volatile.Read(ref _writeTickCount); + /// + /// Gets if this command should be sent over the subscription bridge. + /// + internal bool IsForSubscriptionBridge => (Flags & DemandSubscriptionConnection) != 0; + /// + /// Sends this command to the subscription connection rather than the interactive. + /// + internal void SetForSubscriptionBridge() => Flags |= DemandSubscriptionConnection; + private void SetNeedsTimeoutCheck() => Flags |= NeedsAsyncTimeoutCheckFlag; internal bool HasAsyncTimedOut(int now, int timeoutMilliseconds, out int millisecondsTaken) { diff --git a/src/StackExchange.Redis/RedisBatch.cs b/src/StackExchange.Redis/RedisBatch.cs index 6f4d70700..7abe234c5 100644 --- a/src/StackExchange.Redis/RedisBatch.cs +++ b/src/StackExchange.Redis/RedisBatch.cs @@ -30,7 +30,7 @@ public void Execute() FailNoServer(snapshot); throw ExceptionFactory.NoConnectionAvailable(multiplexer, message, server); } - var bridge = server.GetBridge(message.Command); + var bridge = server.GetBridge(message); if (bridge == null) { FailNoServer(snapshot); diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 3acf60fde..850a10d3b 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -441,12 +441,27 @@ public override Task PingAsync(CommandFlags flags = CommandFlags.None) private Message CreatePingMessage(CommandFlags flags) { - // We're explicitly NOT using PING here because GetBridge() would send this over the interactive connection - // rather than the subscription connection we intend. - RedisValue channel = multiplexer.UniqueId; - var message = ResultProcessor.TimingProcessor.CreateMessage(-1, flags, RedisCommand.UNSUBSCRIBE, channel); - message.SetInternalCall(); - return message; + bool usePing = false; + if (multiplexer.CommandMap.IsAvailable(RedisCommand.PING)) + { + try { usePing = GetFeatures(default, flags, out _).PingOnSubscriber; } + catch { } + } + + Message msg; + if (usePing) + { + msg = ResultProcessor.TimingProcessor.CreateMessage(-1, flags, RedisCommand.PING); + } + else + { + // can't use regular PING, but we can unsubscribe from something random that we weren't even subscribed to... + RedisValue channel = multiplexer.UniqueId; + msg = ResultProcessor.TimingProcessor.CreateMessage(-1, flags, RedisCommand.UNSUBSCRIBE, channel); + } + // Ensure the ping is sent over the intended subscriver connection, which wouldn't happen in GetBridge() by default with PING; + msg.SetForSubscriptionBridge(); + return msg; } public long Publish(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None) diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs index b24d43065..ab2889e65 100755 --- a/src/StackExchange.Redis/ServerEndPoint.cs +++ b/src/StackExchange.Redis/ServerEndPoint.cs @@ -211,6 +211,28 @@ public PhysicalBridge GetBridge(ConnectionType type, bool create = true, LogProx }; } + public PhysicalBridge GetBridge(Message message, bool create = true) + { + if (isDisposed) return null; + + // Subscription commands go to a specific bridge - so we need to set that up. + // There are other commands we need to send to the right connection (e.g. subscriber PING with an explicit SetForSubscriptionBridge call), + // but these always go subscriber. + switch (message.Command) + { + case RedisCommand.SUBSCRIBE: + case RedisCommand.UNSUBSCRIBE: + case RedisCommand.PSUBSCRIBE: + case RedisCommand.PUNSUBSCRIBE: + message.SetForSubscriptionBridge(); + break; + } + + return message.IsForSubscriptionBridge + ? subscription ?? (create ? subscription = CreateBridge(ConnectionType.Subscription, null) : null) + : interactive ?? (create ? interactive = CreateBridge(ConnectionType.Interactive, null) : null); + } + public PhysicalBridge GetBridge(RedisCommand command, bool create = true) { if (isDisposed) return null; @@ -283,9 +305,9 @@ public void SetUnselectable(UnselectableFlags flags) public override string ToString() => Format.ToString(EndPoint); [Obsolete("prefer async")] - public WriteResult TryWriteSync(Message message) => GetBridge(message.Command)?.TryWriteSync(message, isReplica) ?? WriteResult.NoConnectionAvailable; + public WriteResult TryWriteSync(Message message) => GetBridge(message)?.TryWriteSync(message, isReplica) ?? WriteResult.NoConnectionAvailable; - public ValueTask TryWriteAsync(Message message) => GetBridge(message.Command)?.TryWriteAsync(message, isReplica) ?? new ValueTask(WriteResult.NoConnectionAvailable); + public ValueTask TryWriteAsync(Message message) => GetBridge(message)?.TryWriteAsync(message, isReplica) ?? new ValueTask(WriteResult.NoConnectionAvailable); internal void Activate(ConnectionType type, LogProxy log) { @@ -447,11 +469,11 @@ internal ServerCounters GetCounters() return counters; } - internal BridgeStatus GetBridgeStatus(RedisCommand command) + internal BridgeStatus GetBridgeStatus(ConnectionType connectionType) { try { - return GetBridge(command, false)?.GetStatus() ?? BridgeStatus.Zero; + return GetBridge(connectionType, false)?.GetStatus() ?? BridgeStatus.Zero; } catch (Exception ex) { // only needs to be best efforts @@ -486,9 +508,9 @@ internal byte[] GetScriptHash(string script, RedisCommand command) return found; } - internal string GetStormLog(RedisCommand command) + internal string GetStormLog(Message message) { - var bridge = GetBridge(command); + var bridge = GetBridge(message); return bridge?.GetStormLog(); } @@ -703,7 +725,7 @@ static async Task Awaited(ServerEndPoint @this, Message message, ValueTask.Create(out var tcs, null); message.SetSource(processor, source); - if (bridge == null) bridge = GetBridge(message.Command); + if (bridge == null) bridge = GetBridge(message); WriteResult result; if (bridge == null) @@ -735,7 +757,7 @@ internal void WriteDirectFireAndForgetSync(Message message, ResultProcessor(PhysicalConnection co if (connection == null) { Multiplexer.Trace($"{Format.ToString(this)}: Enqueue (async): " + message); - result = GetBridge(message.Command).TryWriteAsync(message, isReplica); + result = GetBridge(message).TryWriteAsync(message, isReplica); } else { diff --git a/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs b/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs index 7b33df000..926948150 100644 --- a/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs +++ b/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs @@ -115,7 +115,7 @@ public async Task Issue922_ReconnectRaised() muxer.ConnectionRestored += (s, e) => { Interlocked.Increment(ref restoreCount); - Log($"Connection Failed ({e.ConnectionType},{e.FailureType}): {e.Exception}"); + Log($"Connection Restored ({e.ConnectionType},{e.FailureType}): {e.Exception}"); }; muxer.GetDatabase(); diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 9c3d264da..27976e811 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -794,7 +794,7 @@ await sub.SubscribeAsync(channel, delegate // Fail all connections server.SimulateConnectionFailure(SimulatedFailureType.All); // Trigger failure - Assert.Throws(() => sub.Ping()); + Assert.Throws(() => sub.Ping()); Assert.False(sub.IsConnected(channel)); // Now reconnect... diff --git a/tests/StackExchange.Redis.Tests/TestBase.cs b/tests/StackExchange.Redis.Tests/TestBase.cs index d1d2ef408..449f001ea 100644 --- a/tests/StackExchange.Redis.Tests/TestBase.cs +++ b/tests/StackExchange.Redis.Tests/TestBase.cs @@ -128,6 +128,7 @@ protected void OnConnectionFailed(object sender, ConnectionFailedEventArgs e) { privateExceptions.Add($"{Time()}: Connection failed ({e.FailureType}): {EndPointCollection.ToString(e.EndPoint)}/{e.ConnectionType}: {e.Exception}"); } + Log($"Connection Failed ({e.ConnectionType},{e.FailureType}): {e.Exception}"); } protected void OnInternalError(object sender, InternalErrorEventArgs e) @@ -284,6 +285,10 @@ internal virtual IInternalConnectionMultiplexer Create( caller); muxer.InternalError += OnInternalError; muxer.ConnectionFailed += OnConnectionFailed; + muxer.ConnectionRestored += (s, e) => + { + Log($"Connection Restored ({e.ConnectionType},{e.FailureType}): {e.Exception}"); + }; return muxer; } From fac5a1b2bfd39a69a89e7bc88e65975e25cda66e Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 10 Jan 2022 10:47:56 -0500 Subject: [PATCH 03/56] Fix KeepAlive on PhysicalBridge --- src/StackExchange.Redis/PhysicalBridge.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/StackExchange.Redis/PhysicalBridge.cs b/src/StackExchange.Redis/PhysicalBridge.cs index f14cf7b10..b9567ab0a 100644 --- a/src/StackExchange.Redis/PhysicalBridge.cs +++ b/src/StackExchange.Redis/PhysicalBridge.cs @@ -356,6 +356,7 @@ internal void KeepAlive() if (commandMap.IsAvailable(RedisCommand.PING) && features.PingOnSubscriber) { msg = Message.Create(-1, CommandFlags.FireAndForget, RedisCommand.PING); + msg.SetForSubscriptionBridge(); msg.SetSource(ResultProcessor.Tracer, null); } else if (commandMap.IsAvailable(RedisCommand.UNSUBSCRIBE)) From 1cb00ffd593e9a4ab070651cc6d1424628e87bc0 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 10 Jan 2022 10:50:51 -0500 Subject: [PATCH 04/56] Fix default version tests --- tests/StackExchange.Redis.Tests/Config.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/Config.cs b/tests/StackExchange.Redis.Tests/Config.cs index a2d0d7034..2a5cf6625 100644 --- a/tests/StackExchange.Redis.Tests/Config.cs +++ b/tests/StackExchange.Redis.Tests/Config.cs @@ -14,7 +14,7 @@ namespace StackExchange.Redis.Tests { public class Config : TestBase { - public Version DefaultVersion = new (2, 8, 0); + public Version DefaultVersion = new (3, 0, 0); public Version DefaultAzureVersion = new (4, 0, 0); public Config(ITestOutputHelper output) : base(output) { } From 7fdb45af68edac792b31c6fa5f66cb46a40d499a Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 10 Jan 2022 20:59:24 -0500 Subject: [PATCH 05/56] Fix up Isue922 test now that we ping the right things *Now* this should be stable killing and restoring both connections with proper PING routing in place. --- tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs b/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs index 926948150..e1d388012 100644 --- a/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs +++ b/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs @@ -123,7 +123,7 @@ public async Task Issue922_ReconnectRaised() Assert.Equal(0, Volatile.Read(ref restoreCount)); var server = muxer.GetServer(TestConfig.Current.MasterServerAndPort); - server.SimulateConnectionFailure(SimulatedFailureType.InteractiveInbound | SimulatedFailureType.InteractiveOutbound); + server.SimulateConnectionFailure(SimulatedFailureType.All); await UntilCondition(TimeSpan.FromSeconds(10), () => Volatile.Read(ref failCount) + Volatile.Read(ref restoreCount) == 4); // interactive+subscriber = 2 From 85c5a4dcbf6def6f4c1ea95e93566ae32e0a8d49 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 10 Jan 2022 21:17:19 -0500 Subject: [PATCH 06/56] Migrate PubSub tests off sync threads --- tests/StackExchange.Redis.Tests/PubSub.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 27976e811..224c4bef8 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -157,6 +157,7 @@ public async Task TestBasicPubSubFireAndForget() var count = sub.Publish(key, "def", CommandFlags.FireAndForget); await PingAsync(muxer, pub, sub).ForAwait(); + await UntilCondition(TimeSpan.FromSeconds(5), () => received.Count == 1); lock (received) { Assert.Single(received); @@ -184,9 +185,7 @@ private static async Task PingAsync(IConnectionMultiplexer muxer, IServer pub, I // way to prove that is to use TPL objects var t1 = sub.PingAsync(); var t2 = pub.PingAsync(); - await Task.Delay(100).ForAwait(); // especially useful when testing any-order mode - - if (!Task.WaitAll(new[] { t1, t2 }, muxer.TimeoutMilliseconds * 2)) throw new TimeoutException(); + await Task.WhenAll(t1, t2).ForAwait(); } } From 98701c9d3fd2f9f797fac4a89550bec416724bf3 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 10 Jan 2022 21:33:45 -0500 Subject: [PATCH 07/56] Fix shared connections with simulated failures (cross-test noise) --- tests/StackExchange.Redis.Tests/AsyncTests.cs | 2 +- tests/StackExchange.Redis.Tests/ConnectFailTimeout.cs | 2 +- tests/StackExchange.Redis.Tests/ConnectionShutdown.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/AsyncTests.cs b/tests/StackExchange.Redis.Tests/AsyncTests.cs index 5ee26f815..4dd36670b 100644 --- a/tests/StackExchange.Redis.Tests/AsyncTests.cs +++ b/tests/StackExchange.Redis.Tests/AsyncTests.cs @@ -19,7 +19,7 @@ public void AsyncTasksReportFailureIfServerUnavailable() { SetExpectedAmbientFailureCount(-1); // this will get messy - using (var conn = Create(allowAdmin: true)) + using (var conn = Create(allowAdmin: true, shared: false)) { var server = conn.GetServer(TestConfig.Current.MasterServer, TestConfig.Current.MasterPort); diff --git a/tests/StackExchange.Redis.Tests/ConnectFailTimeout.cs b/tests/StackExchange.Redis.Tests/ConnectFailTimeout.cs index c52082d12..73af84fa4 100644 --- a/tests/StackExchange.Redis.Tests/ConnectFailTimeout.cs +++ b/tests/StackExchange.Redis.Tests/ConnectFailTimeout.cs @@ -13,7 +13,7 @@ public ConnectFailTimeout(ITestOutputHelper output) : base (output) { } public async Task NoticesConnectFail() { SetExpectedAmbientFailureCount(-1); - using (var conn = Create(allowAdmin: true)) + using (var conn = Create(allowAdmin: true, shared: false)) { var server = conn.GetServer(conn.GetEndPoints()[0]); diff --git a/tests/StackExchange.Redis.Tests/ConnectionShutdown.cs b/tests/StackExchange.Redis.Tests/ConnectionShutdown.cs index a4e720772..d75054ca4 100644 --- a/tests/StackExchange.Redis.Tests/ConnectionShutdown.cs +++ b/tests/StackExchange.Redis.Tests/ConnectionShutdown.cs @@ -14,7 +14,7 @@ public ConnectionShutdown(ITestOutputHelper output) : base(output) { } [Fact(Skip = "Unfriendly")] public async Task ShutdownRaisesConnectionFailedAndRestore() { - using (var conn = Create(allowAdmin: true)) + using (var conn = Create(allowAdmin: true, shared: false)) { int failed = 0, restored = 0; Stopwatch watch = Stopwatch.StartNew(); From 377c813ebd2108add69f225b5469b42b4029f8d5 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 10 Jan 2022 21:34:29 -0500 Subject: [PATCH 08/56] Compensate for delay removal This awaits the condition, rather than a magical delay previously. --- tests/StackExchange.Redis.Tests/PubSub.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 224c4bef8..a96275254 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -93,6 +93,7 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br await PingAsync(muxer, pub, sub, 3).ForAwait(); + await UntilCondition(TimeSpan.FromSeconds(5), () => received.Count == 1); lock (received) { Assert.Single(received); @@ -221,6 +222,7 @@ public async Task TestPatternPubSub() var count = sub.Publish("abc", "def"); await PingAsync(muxer, pub, sub).ForAwait(); + await UntilCondition(TimeSpan.FromSeconds(5), () => received.Count == 1); lock (received) { Assert.Single(received); From d9c68e1fc5080b835d8ff7b9fc318795410a6df7 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 10 Jan 2022 21:44:43 -0500 Subject: [PATCH 09/56] Add logging to pubsub methods --- tests/StackExchange.Redis.Tests/PubSub.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index a96275254..cbf80b851 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -57,7 +57,7 @@ await UntilCondition(TimeSpan.FromSeconds(10), [InlineData("Foo:", true, "f")] public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string breaker) { - using (var muxer = Create(channelPrefix: channelPrefix)) + using (var muxer = Create(channelPrefix: channelPrefix, log: Writer)) { var pub = GetAnyMaster(muxer); var sub = muxer.GetSubscriber(); @@ -127,7 +127,7 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br [Fact] public async Task TestBasicPubSubFireAndForget() { - using (var muxer = Create()) + using (var muxer = Create(log: Writer)) { var pub = GetAnyMaster(muxer); var sub = muxer.GetSubscriber(); From 3f6e03043f37483a23fe15c535d115283e0d6855 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 10 Jan 2022 22:07:47 -0500 Subject: [PATCH 10/56] Add logging to PubSubGetAllCorrectOrder --- tests/StackExchange.Redis.Tests/PubSub.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index cbf80b851..646041055 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -351,7 +351,7 @@ await sub.SubscribeAsync(channel, (_, val) => [Fact] public async Task PubSubGetAllCorrectOrder() { - using (var muxer = Create(configuration: TestConfig.Current.RemoteServerAndPort, syncTimeout: 20000)) + using (var muxer = Create(configuration: TestConfig.Current.RemoteServerAndPort, syncTimeout: 20000, log: Writer)) { var sub = muxer.GetSubscriber(); RedisChannel channel = Me(); From b63648aa686ce30d6299455652c70d948fe279b5 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 10 Jan 2022 22:38:09 -0500 Subject: [PATCH 11/56] Tidy exception messages --- tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs b/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs index e1d388012..bbe3a0892 100644 --- a/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs +++ b/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs @@ -110,12 +110,12 @@ public async Task Issue922_ReconnectRaised() muxer.ConnectionFailed += (s, e) => { Interlocked.Increment(ref failCount); - Log($"Connection Failed ({e.ConnectionType},{e.FailureType}): {e.Exception}"); + Log($"Connection Failed ({e.ConnectionType}, {e.FailureType}): {e.Exception}"); }; muxer.ConnectionRestored += (s, e) => { Interlocked.Increment(ref restoreCount); - Log($"Connection Restored ({e.ConnectionType},{e.FailureType}): {e.Exception}"); + Log($"Connection Restored ({e.ConnectionType}, {e.FailureType})"); }; muxer.GetDatabase(); From 148c9752ea4a5fe55f8a0e41c8ca19990952429e Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Wed, 12 Jan 2022 16:15:45 -0500 Subject: [PATCH 12/56] Eliminate writer here --- tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs b/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs index bbe3a0892..5042d51a8 100644 --- a/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs +++ b/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs @@ -105,7 +105,7 @@ public async Task Issue922_ReconnectRaised() int failCount = 0, restoreCount = 0; - using (var muxer = ConnectionMultiplexer.Connect(config, log: Writer)) + using (var muxer = ConnectionMultiplexer.Connect(config)) { muxer.ConnectionFailed += (s, e) => { From 92cecfcbbf214138445f212c47b6bae8e5c516a4 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Wed, 19 Jan 2022 23:14:43 -0500 Subject: [PATCH 13/56] WIP: This could all be a bad idea --- .../ConnectionMultiplexer.cs | 2 +- .../Interfaces/ISubscriber.cs | 8 +- src/StackExchange.Redis/PhysicalBridge.cs | 67 ----- src/StackExchange.Redis/RedisSubscriber.cs | 253 +++++++----------- 4 files changed, 109 insertions(+), 221 deletions(-) diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.cs b/src/StackExchange.Redis/ConnectionMultiplexer.cs index 6e0d71d74..b652aff34 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.cs @@ -1921,7 +1921,7 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, LogP } if (!first) { - long subscriptionChanges = ValidateSubscriptions(); + long subscriptionChanges = await EnsureSubscriptionsAsync(); if (subscriptionChanges == 0) { log?.WriteLine("No subscription changes necessary"); diff --git a/src/StackExchange.Redis/Interfaces/ISubscriber.cs b/src/StackExchange.Redis/Interfaces/ISubscriber.cs index b479a8d8d..11e985a0b 100644 --- a/src/StackExchange.Redis/Interfaces/ISubscriber.cs +++ b/src/StackExchange.Redis/Interfaces/ISubscriber.cs @@ -100,8 +100,8 @@ public interface ISubscriber : IRedis EndPoint SubscribedEndpoint(RedisChannel channel); /// - /// Unsubscribe from a specified message channel; note; if no handler is specified, the subscription is cancelled regardless - /// of the subscribers; if a handler is specified, the subscription is only cancelled if this handler is the + /// Unsubscribe from a specified message channel; note; if no handler is specified, the subscription is canceled regardless + /// of the subscribers; if a handler is specified, the subscription is only canceled if this handler is the /// last handler remaining against the channel /// /// The channel that was subscribed to. @@ -128,8 +128,8 @@ public interface ISubscriber : IRedis Task UnsubscribeAllAsync(CommandFlags flags = CommandFlags.None); /// - /// Unsubscribe from a specified message channel; note; if no handler is specified, the subscription is cancelled regardless - /// of the subscribers; if a handler is specified, the subscription is only cancelled if this handler is the + /// Unsubscribe from a specified message channel; note; if no handler is specified, the subscription is canceled regardless + /// of the subscribers; if a handler is specified, the subscription is only canceled if this handler is the /// last handler remaining against the channel /// /// The channel that was subscribed to. diff --git a/src/StackExchange.Redis/PhysicalBridge.cs b/src/StackExchange.Redis/PhysicalBridge.cs index 38abeb672..51abb2996 100644 --- a/src/StackExchange.Redis/PhysicalBridge.cs +++ b/src/StackExchange.Redis/PhysicalBridge.cs @@ -8,7 +8,6 @@ using System.Threading.Channels; using System.Threading.Tasks; using static StackExchange.Redis.ConnectionMultiplexer; -using PendingSubscriptionState = global::StackExchange.Redis.ConnectionMultiplexer.Subscription.PendingSubscriptionState; #if !NETCOREAPP using Pipelines.Sockets.Unofficial.Threading; using static Pipelines.Sockets.Unofficial.Threading.MutexSlim; @@ -102,7 +101,6 @@ public enum State : byte public void Dispose() { isDisposed = true; - ShutdownSubscriptionQueue(); using (var tmp = physical) { physical = null; @@ -221,71 +219,6 @@ internal void GetCounters(ConnectionCounters counters) physical?.GetCounters(counters); } - private Channel _subscriptionBackgroundQueue; - private static readonly UnboundedChannelOptions s_subscriptionQueueOptions = new UnboundedChannelOptions - { - AllowSynchronousContinuations = false, // we do *not* want the async work to end up on the caller's thread - SingleReader = true, // only one reader will be started per channel - SingleWriter = true, // writes will be synchronized, because order matters - }; - - private Channel GetSubscriptionQueue() - { - var queue = _subscriptionBackgroundQueue; - if (queue == null) - { - queue = Channel.CreateUnbounded(s_subscriptionQueueOptions); - var existing = Interlocked.CompareExchange(ref _subscriptionBackgroundQueue, queue, null); - - if (existing != null) return existing; // we didn't win, but that's fine - - // we won (_subqueue is now queue) - // this means we have a new channel without a reader; let's fix that! - Task.Run(() => ExecuteSubscriptionLoop()); - } - return queue; - } - - private void ShutdownSubscriptionQueue() - { - try - { - Interlocked.CompareExchange(ref _subscriptionBackgroundQueue, null, null)?.Writer.TryComplete(); - } - catch { } - } - - private async Task ExecuteSubscriptionLoop() // pushes items that have been enqueued over the bridge - { - // note: this will execute on the default pool rather than our dedicated pool; I'm... OK with this - var queue = _subscriptionBackgroundQueue ?? Interlocked.CompareExchange(ref _subscriptionBackgroundQueue, null, null); // just to be sure we can read it! - try - { - while (await queue.Reader.WaitToReadAsync().ForAwait() && queue.Reader.TryRead(out var next)) - { - try - { - // Treat these commands as background/handshake and do not allow queueing to backlog - if ((await TryWriteAsync(next.Message, next.IsReplica).ForAwait()) != WriteResult.Success) - { - next.Abort(); - } - } - catch (Exception ex) - { - next.Fail(ex); - } - } - } - catch (Exception ex) - { - Multiplexer.OnInternalError(ex, ServerEndPoint?.EndPoint, ConnectionType); - } - } - - internal bool TryEnqueueBackgroundSubscriptionWrite(in PendingSubscriptionState state) - => !isDisposed && (_subscriptionBackgroundQueue ?? GetSubscriptionQueue()).Writer.TryWrite(state); - internal readonly struct BridgeStatus { /// diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 850a10d3b..d8b7dd3ca 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -1,5 +1,5 @@ using System; -using System.Collections.Generic; +using System.Collections.Concurrent; using System.Diagnostics; using System.Net; using System.Runtime.CompilerServices; @@ -11,15 +11,10 @@ namespace StackExchange.Redis { public partial class ConnectionMultiplexer { - private readonly Dictionary subscriptions = new Dictionary(); + private readonly SemaphoreSlim subscriptionsLock = new SemaphoreSlim(1, 1); + private readonly ConcurrentDictionary subscriptions = new ConcurrentDictionary(); - internal int GetSubscriptionsCount() - { - lock (subscriptions) - { - return subscriptions.Count; - } - } + internal int GetSubscriptionsCount() => subscriptions.Count; internal static void CompleteAsWorker(ICompletable completable) { @@ -54,12 +49,7 @@ internal static bool TryCompleteHandler(EventHandler handler, object sende internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out int queues) { - Subscription sub; - lock (subscriptions) - { - if (!subscriptions.TryGetValue(channel, out sub)) sub = null; - } - if (sub != null) + if (subscriptions.TryGetValue(channel, out var sub)) { sub.GetSubscriberCounts(out handlers, out queues); return true; @@ -68,36 +58,27 @@ internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out return false; } - internal Task AddSubscription(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags, object asyncState) + internal Task AddSubscriptionAsync(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags, object asyncState) { Task task = null; if (handler != null | queue != null) { - lock (subscriptions) + if (!subscriptions.TryGetValue(channel, out Subscription sub)) { - if (!subscriptions.TryGetValue(channel, out Subscription sub)) - { - sub = new Subscription(); - subscriptions.Add(channel, sub); - task = sub.SubscribeToServer(this, channel, flags, asyncState, false); - } - sub.Add(handler, queue); + sub = new Subscription(flags); + subscriptions.TryAdd(channel, sub); + task = sub.SubscribeToServerAsync(this, channel, flags, asyncState, false); } + sub.Add(handler, queue); } return task ?? CompletedTask.Default(asyncState); } internal ServerEndPoint GetSubscribedServer(in RedisChannel channel) { - if (!channel.IsNullOrEmpty) + if (!channel.IsNullOrEmpty && subscriptions.TryGetValue(channel, out Subscription sub)) { - lock (subscriptions) - { - if (subscriptions.TryGetValue(channel, out Subscription sub)) - { - return sub.GetOwner(); - } - } + return sub.GetCurrentServer(); } return null; } @@ -106,12 +87,9 @@ internal void OnMessage(in RedisChannel subscription, in RedisChannel channel, i { ICompletable completable = null; ChannelMessageQueue queues = null; - lock (subscriptions) + if (subscriptions.TryGetValue(subscription, out Subscription sub)) { - if (subscriptions.TryGetValue(subscription, out Subscription sub)) - { - completable = sub.ForInvoke(channel, payload, out queues); - } + completable = sub.ForInvoke(channel, payload, out queues); } if (queues != null) ChannelMessageQueue.WriteAll(ref queues, channel, payload); if (completable != null && !completable.TryComplete(false)) ConnectionMultiplexer.CompleteAsWorker(completable); @@ -120,15 +98,13 @@ internal void OnMessage(in RedisChannel subscription, in RedisChannel channel, i internal Task RemoveAllSubscriptions(CommandFlags flags, object asyncState) { Task last = null; - lock (subscriptions) + foreach (var pair in subscriptions) { - foreach (var pair in subscriptions) + if (subscriptions.TryRemove(pair.Key, out var sub)) { pair.Value.MarkCompleted(); - var task = pair.Value.UnsubscribeFromServer(pair.Key, flags, asyncState, false); - if (task != null) last = task; + last = pair.Value.UnsubscribeFromServerAsync(pair.Key, asyncState, false); } - subscriptions.Clear(); } return last ?? CompletedTask.Default(asyncState); } @@ -136,25 +112,23 @@ internal Task RemoveAllSubscriptions(CommandFlags flags, object asyncState) internal Task RemoveSubscription(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags, object asyncState) { Task task = null; - lock (subscriptions) + if (subscriptions.TryGetValue(channel, out Subscription sub)) { - if (subscriptions.TryGetValue(channel, out Subscription sub)) + bool removeChannel; + if (handler == null & queue == null) // blanket wipe { - bool remove; - if (handler == null & queue == null) // blanket wipe - { - sub.MarkCompleted(); - remove = true; - } - else - { - remove = sub.Remove(handler, queue); - } - if (remove) - { - subscriptions.Remove(channel); - task = sub.UnsubscribeFromServer(channel, flags, asyncState, false); - } + sub.MarkCompleted(); + removeChannel = true; + } + else + { + removeChannel = sub.Remove(handler, queue); + } + // If it was the last handler or a blanket wipe, remove it. + if (removeChannel) + { + subscriptions.TryRemove(channel, out _); + task = sub.UnsubscribeFromServerAsync(channel, asyncState, false); } } return task ?? CompletedTask.Default(asyncState); @@ -163,12 +137,9 @@ internal Task RemoveSubscription(in RedisChannel channel, Action EnsureSubscriptionsAsync() { - lock (subscriptions) + long count = 0; + foreach (var pair in subscriptions) { - long count = 0; - foreach (var pair in subscriptions) + if (await pair.Value.EnsureSubscribedAsync(this, pair.Key)) { - if (pair.Value.Validate(this, pair.Key)) count++; + count++; } - return count; } + return count; } internal sealed class Subscription { private Action _handlers; private ChannelMessageQueue _queues; - private ServerEndPoint owner; + private ServerEndPoint CurrentServer; + public CommandFlags Flags { get; } + + public Subscription(CommandFlags flags) + { + Flags = flags; + } + + private Message GetMessage( + RedisChannel channel, + RedisCommand command, + object asyncState, + bool internalCall, + out TaskCompletionSource taskSource) + { + var msg = Message.Create(-1, Flags, command, channel); + if (internalCall) msg.SetInternalCall(); + + var source = TaskResultBox.Create(out taskSource, asyncState); + msg.SetSource(ResultProcessor.TrackSubscriptions, source); + return msg; + } public void Add(Action handler, ChannelMessageQueue queue) { @@ -225,90 +217,58 @@ public bool Remove(Action handler, ChannelMessageQueue return _handlers == null & _queues == null; } - [System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "RCS1210:Return completed task instead of returning null.", Justification = "Intentional for efficient success check")] - public Task SubscribeToServer(ConnectionMultiplexer multiplexer, in RedisChannel channel, CommandFlags flags, object asyncState, bool internalCall) + public async Task SubscribeToServerAsync(ConnectionMultiplexer multiplexer, RedisChannel channel, CommandFlags flags, object asyncState, bool internalCall) { + var command = channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE; // TODO: default(RedisKey) is incorrect here - should shard based on the channel in cluster - var selected = multiplexer.SelectServer(RedisCommand.SUBSCRIBE, flags, default(RedisKey)); - var bridge = selected?.GetBridge(ConnectionType.Subscription, true); - if (bridge == null) return null; - - // note: check we can create the message validly *before* we swap the owner over (Interlocked) - var state = PendingSubscriptionState.Create(channel, this, flags, true, internalCall, asyncState, selected.IsReplica); + var selected = multiplexer.SelectServer(command, flags, default(RedisKey)); - if (Interlocked.CompareExchange(ref owner, selected, null) != null) return null; + if (Interlocked.CompareExchange(ref CurrentServer, selected, null) != null) + { + // Abort + return false; + } try { - if (!bridge.TryEnqueueBackgroundSubscriptionWrite(state)) + var message = GetMessage(channel, command, asyncState, internalCall, out var taskSource); + var success = await multiplexer.ExecuteAsyncImpl(message, ResultProcessor.TrackSubscriptions, null, selected); + if (!success) { - state.Abort(); - return null; + taskSource.SetCanceled(); } - return state.Task; + return await taskSource.Task; } catch { // clear the owner if it is still us - Interlocked.CompareExchange(ref owner, null, selected); + Interlocked.CompareExchange(ref CurrentServer, null, selected); throw; } } - [System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "RCS1210:Return completed task instead of returning null.", Justification = "Intentional for efficient success check")] - public Task UnsubscribeFromServer(in RedisChannel channel, CommandFlags flags, object asyncState, bool internalCall) - { - var oldOwner = Interlocked.Exchange(ref owner, null); - var bridge = oldOwner?.GetBridge(ConnectionType.Subscription, false); - if (bridge == null) return null; - - var state = PendingSubscriptionState.Create(channel, this, flags, false, internalCall, asyncState, oldOwner.IsReplica); - - if (!bridge.TryEnqueueBackgroundSubscriptionWrite(state)) - { - state.Abort(); - return null; - } - return state.Task; - } - - internal readonly struct PendingSubscriptionState + public async Task UnsubscribeFromServerAsync(RedisChannel channel, object asyncState, bool internalCall) { - public override string ToString() => Message.ToString(); - public Subscription Subscription { get; } - public Message Message { get; } - public bool IsReplica { get; } - public Task Task => _taskSource.Task; - private readonly TaskCompletionSource _taskSource; - - public static PendingSubscriptionState Create(RedisChannel channel, Subscription subscription, CommandFlags flags, bool subscribe, bool internalCall, object asyncState, bool isReplica) - => new PendingSubscriptionState(asyncState, channel, subscription, flags, subscribe, internalCall, isReplica); - - public void Abort() => _taskSource.TrySetCanceled(); - public void Fail(Exception ex) => _taskSource.TrySetException(ex); - - private PendingSubscriptionState(object asyncState, RedisChannel channel, Subscription subscription, CommandFlags flags, bool subscribe, bool internalCall, bool isReplica) + var command = channel.IsPatternBased ? RedisCommand.PUNSUBSCRIBE : RedisCommand.UNSUBSCRIBE; + var oldOwner = Interlocked.Exchange(ref CurrentServer, null); + if (oldOwner != null) { - var cmd = subscribe - ? (channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE) - : (channel.IsPatternBased ? RedisCommand.PUNSUBSCRIBE : RedisCommand.UNSUBSCRIBE); - var msg = Message.Create(-1, flags, cmd, channel); - if (internalCall) msg.SetInternalCall(); - - var source = TaskResultBox.Create(out _taskSource, asyncState); - msg.SetSource(ResultProcessor.TrackSubscriptions, source); - - Subscription = subscription; - Message = msg; - IsReplica = isReplica; + var message = GetMessage(channel, command, asyncState, internalCall, out var taskSource); + var success = await oldOwner.Multiplexer.ExecuteAsyncImpl(message, ResultProcessor.TrackSubscriptions, null, oldOwner); + if (!success) + { + taskSource.SetCanceled(); + } + return await taskSource.Task; } + return false; } - internal ServerEndPoint GetOwner() => Volatile.Read(ref owner); + internal ServerEndPoint GetCurrentServer() => Volatile.Read(ref CurrentServer); internal void Resubscribe(in RedisChannel channel, ServerEndPoint server) { // Only re-subscribe to the original server - if (server != null && GetOwner() == server) + if (server != null && GetCurrentServer() == server) { var cmd = channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE; var msg = Message.Create(-1, CommandFlags.FireAndForget, cmd, channel); @@ -317,21 +277,20 @@ internal void Resubscribe(in RedisChannel channel, ServerEndPoint server) } } - internal bool Validate(ConnectionMultiplexer multiplexer, in RedisChannel channel) + internal async ValueTask EnsureSubscribedAsync(ConnectionMultiplexer multiplexer, RedisChannel channel) { bool changed = false; - var oldOwner = Volatile.Read(ref owner); + var oldOwner = Volatile.Read(ref CurrentServer); + // If the old server is bad, unsubscribe if (oldOwner != null && !oldOwner.IsSelectable(RedisCommand.PSUBSCRIBE)) { - if (UnsubscribeFromServer(channel, CommandFlags.FireAndForget, null, true) != null) - { - changed = true; - } + changed = await UnsubscribeFromServerAsync(channel, null, true); oldOwner = null; } - if (oldOwner == null && SubscribeToServer(multiplexer, channel, CommandFlags.FireAndForget, null, true) != null) + // If we didn't have an owner or just cleared one, subscribe + if (oldOwner == null) { - changed = true; + changed = await SubscribeToServerAsync(multiplexer, channel, CommandFlags.FireAndForget, null, true); } return changed; } @@ -422,10 +381,7 @@ public Task IdentifyEndpointAsync(RedisChannel channel, CommandFlags f return ExecuteAsync(msg, ResultProcessor.ConnectionIdentity); } - public bool IsConnected(RedisChannel channel = default(RedisChannel)) - { - return multiplexer.SubscriberConnected(channel); - } + public bool IsConnected(RedisChannel channel = default(RedisChannel)) => multiplexer.SubscriberConnected(channel); public override TimeSpan Ping(CommandFlags flags = CommandFlags.None) { @@ -459,7 +415,7 @@ private Message CreatePingMessage(CommandFlags flags) RedisValue channel = multiplexer.UniqueId; msg = ResultProcessor.TimingProcessor.CreateMessage(-1, flags, RedisCommand.UNSUBSCRIBE, channel); } - // Ensure the ping is sent over the intended subscriver connection, which wouldn't happen in GetBridge() by default with PING; + // Ensure the ping is sent over the intended subscriber connection, which wouldn't happen in GetBridge() by default with PING; msg.SetForSubscriptionBridge(); return msg; } @@ -500,7 +456,7 @@ Task ISubscriber.SubscribeAsync(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) { if (channel.IsNullOrEmpty) throw new ArgumentNullException(nameof(channel)); - return multiplexer.AddSubscription(channel, handler, queue, flags, asyncState); + return multiplexer.AddSubscriptionAsync(channel, handler, queue, flags, asyncState); } internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out int queues) @@ -521,6 +477,7 @@ public EndPoint SubscribedEndpoint(RedisChannel channel) void ISubscriber.Unsubscribe(RedisChannel channel, Action handler, CommandFlags flags) => Unsubscribe(channel, handler, null, flags); + public void Unsubscribe(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) { var task = UnsubscribeAsync(channel, handler, queue, flags); @@ -533,10 +490,8 @@ public void UnsubscribeAll(CommandFlags flags = CommandFlags.None) if ((flags & CommandFlags.FireAndForget) == 0) Wait(task); } - public Task UnsubscribeAllAsync(CommandFlags flags = CommandFlags.None) - { - return multiplexer.RemoveAllSubscriptions(flags, asyncState); - } + public Task UnsubscribeAllAsync(CommandFlags flags = CommandFlags.None) => + multiplexer.RemoveAllSubscriptions(flags, asyncState); Task ISubscriber.UnsubscribeAsync(RedisChannel channel, Action handler, CommandFlags flags) => UnsubscribeAsync(channel, handler, null, flags); From 7d7f0207edb36c9222f5bacbb9cd342f76c908fc Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Thu, 20 Jan 2022 10:53:53 -0500 Subject: [PATCH 14/56] Gap commit Want to yank some of this into another PR ahead of time, getting files in. --- .../ConnectionMultiplexer.Threading.cs | 50 +++++ .../ConnectionMultiplexer.Verbose.cs | 59 ++++++ .../ConnectionMultiplexer.cs | 43 +--- src/StackExchange.Redis/RedisSubscriber.cs | 190 +++++++----------- .../ServerSelectionStrategy.cs | 25 ++- 5 files changed, 205 insertions(+), 162 deletions(-) create mode 100644 src/StackExchange.Redis/ConnectionMultiplexer.Threading.cs create mode 100644 src/StackExchange.Redis/ConnectionMultiplexer.Verbose.cs diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.Threading.cs b/src/StackExchange.Redis/ConnectionMultiplexer.Threading.cs new file mode 100644 index 000000000..f23d010cc --- /dev/null +++ b/src/StackExchange.Redis/ConnectionMultiplexer.Threading.cs @@ -0,0 +1,50 @@ +using System; +using System.Threading; +using Pipelines.Sockets.Unofficial; + +namespace StackExchange.Redis +{ + public partial class ConnectionMultiplexer + { + private static readonly WaitCallback s_CompleteAsWorker = s => ((ICompletable)s).TryComplete(true); + internal static void CompleteAsWorker(ICompletable completable) + { + if (completable != null) + { + ThreadPool.QueueUserWorkItem(s_CompleteAsWorker, completable); + } + } + + internal static bool TryCompleteHandler(EventHandler handler, object sender, T args, bool isAsync) where T : EventArgs, ICompletable + { + if (handler == null) return true; + if (isAsync) + { + if (handler.IsSingle()) + { + try + { + handler(sender, args); + } + catch { } + } + else + { + foreach (EventHandler sub in handler.AsEnumerable()) + { + try + { + sub(sender, args); + } + catch { } + } + } + return true; + } + else + { + return false; + } + } + } +} diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.Verbose.cs b/src/StackExchange.Redis/ConnectionMultiplexer.Verbose.cs new file mode 100644 index 000000000..7a61096b6 --- /dev/null +++ b/src/StackExchange.Redis/ConnectionMultiplexer.Verbose.cs @@ -0,0 +1,59 @@ +using System; +using System.Diagnostics; +using System.Net; +using System.Runtime.CompilerServices; + +namespace StackExchange.Redis +{ + public partial class ConnectionMultiplexer + { + internal event Action MessageFaulted; + internal event Action Closing; + internal event Action PreTransactionExec, TransactionLog, InfoMessage; + internal event Action Connecting; + internal event Action Resurrecting; + + partial void OnTrace(string message, string category); + static partial void OnTraceWithoutContext(string message, string category); + + [Conditional("VERBOSE")] + internal void Trace(string message, [CallerMemberName] string category = null) => OnTrace(message, category); + + [Conditional("VERBOSE")] + internal void Trace(bool condition, string message, [CallerMemberName] string category = null) + { + if (condition) OnTrace(message, category); + } + + [Conditional("VERBOSE")] + internal static void TraceWithoutContext(string message, [CallerMemberName] string category = null) => OnTraceWithoutContext(message, category); + + [Conditional("VERBOSE")] + internal static void TraceWithoutContext(bool condition, string message, [CallerMemberName] string category = null) + { + if (condition) OnTraceWithoutContext(message, category); + } + + [Conditional("VERBOSE")] + internal void OnMessageFaulted(Message msg, Exception fault, [CallerMemberName] string origin = default, [CallerFilePath] string path = default, [CallerLineNumber] int lineNumber = default) => + MessageFaulted?.Invoke(msg?.CommandAndKey, fault, $"{origin} ({path}#{lineNumber})"); + + [Conditional("VERBOSE")] + internal void OnInfoMessage(string message) => InfoMessage?.Invoke(message); + + [Conditional("VERBOSE")] + internal void OnClosing(bool complete) => Closing?.Invoke(complete); + + [Conditional("VERBOSE")] + internal void OnConnecting(EndPoint endpoint, ConnectionType connectionType) => Connecting?.Invoke(endpoint, connectionType); + + [Conditional("VERBOSE")] + internal void OnResurrecting(EndPoint endpoint, ConnectionType connectionType) => Resurrecting.Invoke(endpoint, connectionType); + + [Conditional("VERBOSE")] + internal void OnPreTransactionExec(Message message) => PreTransactionExec?.Invoke(message.CommandAndKey); + + [Conditional("VERBOSE")] + internal void OnTransactionLog(string message) => TransactionLog?.Invoke(message); + } +} diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.cs b/src/StackExchange.Redis/ConnectionMultiplexer.cs index b652aff34..2254cf50a 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.cs @@ -1504,33 +1504,6 @@ public IServer GetServer(EndPoint endpoint, object asyncState = null) return new RedisServer(this, server, asyncState); } - [Conditional("VERBOSE")] - internal void Trace(string message, [CallerMemberName] string category = null) - { - OnTrace(message, category); - } - - [Conditional("VERBOSE")] - internal void Trace(bool condition, string message, [CallerMemberName] string category = null) - { - if (condition) OnTrace(message, category); - } - - partial void OnTrace(string message, string category); - static partial void OnTraceWithoutContext(string message, string category); - - [Conditional("VERBOSE")] - internal static void TraceWithoutContext(string message, [CallerMemberName] string category = null) - { - OnTraceWithoutContext(message, category); - } - - [Conditional("VERBOSE")] - internal static void TraceWithoutContext(bool condition, string message, [CallerMemberName] string category = null) - { - if (condition) OnTraceWithoutContext(message, category); - } - /// /// The number of operations that have been performed on all connections /// @@ -2174,16 +2147,14 @@ internal void UpdateClusterRange(ClusterConfiguration configuration) private IDisposable pulse; - internal ServerEndPoint SelectServer(Message message) - { - if (message == null) return null; - return ServerSelectionStrategy.Select(message); - } + internal ServerEndPoint SelectServer(Message message) => + message == null ? null : ServerSelectionStrategy.Select(message); - internal ServerEndPoint SelectServer(RedisCommand command, CommandFlags flags, in RedisKey key) - { - return ServerSelectionStrategy.Select(command, key, flags); - } + internal ServerEndPoint SelectServer(RedisCommand command, CommandFlags flags, in RedisKey key) => + ServerSelectionStrategy.Select(command, key, flags); + + internal ServerEndPoint SelectServer(RedisCommand command, CommandFlags flags, in RedisChannel channel) => + ServerSelectionStrategy.Select(command, channel, flags); private bool PrepareToPushMessageToBridge(Message message, ResultProcessor processor, IResultBox resultBox, ref ServerEndPoint server) { diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index d8b7dd3ca..6ded6952f 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -1,8 +1,6 @@ using System; using System.Collections.Concurrent; -using System.Diagnostics; using System.Net; -using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using Pipelines.Sockets.Unofficial; @@ -11,42 +9,11 @@ namespace StackExchange.Redis { public partial class ConnectionMultiplexer { - private readonly SemaphoreSlim subscriptionsLock = new SemaphoreSlim(1, 1); - private readonly ConcurrentDictionary subscriptions = new ConcurrentDictionary(); + private readonly SemaphoreSlim subscriptionsAddLock = new SemaphoreSlim(1, 1); + private readonly ConcurrentDictionary subscriptions = new(); internal int GetSubscriptionsCount() => subscriptions.Count; - internal static void CompleteAsWorker(ICompletable completable) - { - if (completable != null) ThreadPool.QueueUserWorkItem(s_CompleteAsWorker, completable); - } - - private static readonly WaitCallback s_CompleteAsWorker = s => ((ICompletable)s).TryComplete(true); - - internal static bool TryCompleteHandler(EventHandler handler, object sender, T args, bool isAsync) where T : EventArgs, ICompletable - { - if (handler == null) return true; - if (isAsync) - { - if (handler.IsSingle()) - { - try { handler(sender, args); } catch { } - } - else - { - foreach (EventHandler sub in handler.AsEnumerable()) - { - try { sub(sender, args); } catch { } - } - } - return true; - } - else - { - return false; - } - } - internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out int queues) { if (subscriptions.TryGetValue(channel, out var sub)) @@ -58,20 +25,22 @@ internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out return false; } - internal Task AddSubscriptionAsync(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags, object asyncState) + internal async Task AddSubscriptionAsync(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags, object asyncState) { - Task task = null; if (handler != null | queue != null) { if (!subscriptions.TryGetValue(channel, out Subscription sub)) { sub = new Subscription(flags); subscriptions.TryAdd(channel, sub); - task = sub.SubscribeToServerAsync(this, channel, flags, asyncState, false); + if (!(await sub.SubscribeToServerAsync(this, channel, flags, asyncState, false))) + { + return false; + } } sub.Add(handler, queue); } - return task ?? CompletedTask.Default(asyncState); + return true; } internal ServerEndPoint GetSubscribedServer(in RedisChannel channel) @@ -91,11 +60,17 @@ internal void OnMessage(in RedisChannel subscription, in RedisChannel channel, i { completable = sub.ForInvoke(channel, payload, out queues); } - if (queues != null) ChannelMessageQueue.WriteAll(ref queues, channel, payload); - if (completable != null && !completable.TryComplete(false)) ConnectionMultiplexer.CompleteAsWorker(completable); + if (queues != null) + { + ChannelMessageQueue.WriteAll(ref queues, channel, payload); + } + if (completable != null && !completable.TryComplete(false)) + { + CompleteAsWorker(completable); + } } - internal Task RemoveAllSubscriptions(CommandFlags flags, object asyncState) + internal Task RemoveAllSubscriptionsAsync(CommandFlags flags, object asyncState) { Task last = null; foreach (var pair in subscriptions) @@ -109,7 +84,7 @@ internal Task RemoveAllSubscriptions(CommandFlags flags, object asyncState) return last ?? CompletedTask.Default(asyncState); } - internal Task RemoveSubscription(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags, object asyncState) + internal Task RemoveSubscriptionAsync(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags, object asyncState) { Task task = null; if (subscriptions.TryGetValue(channel, out Subscription sub)) @@ -145,9 +120,7 @@ internal void ResendSubscriptions(ServerEndPoint server) internal bool SubscriberConnected(in RedisChannel channel = default(RedisChannel)) { - // TODO: default(RedisKey) is incorrect here - should shard based on the channel in cluster - var server = GetSubscribedServer(channel) ?? SelectServer(RedisCommand.SUBSCRIBE, CommandFlags.DemandMaster, default(RedisKey)); - + var server = GetSubscribedServer(channel) ?? SelectServer(RedisCommand.SUBSCRIBE, CommandFlags.DemandMaster, channel); return server?.IsConnected == true && server.IsSubscriberConnected; } @@ -171,10 +144,7 @@ internal sealed class Subscription private ServerEndPoint CurrentServer; public CommandFlags Flags { get; } - public Subscription(CommandFlags flags) - { - Flags = flags; - } + public Subscription(CommandFlags flags) => Flags = flags; private Message GetMessage( RedisChannel channel, @@ -184,7 +154,11 @@ private Message GetMessage( out TaskCompletionSource taskSource) { var msg = Message.Create(-1, Flags, command, channel); - if (internalCall) msg.SetInternalCall(); + msg.SetForSubscriptionBridge(); + if (internalCall) + { + msg.SetInternalCall(); + } var source = TaskResultBox.Create(out taskSource, asyncState); msg.SetSource(ResultProcessor.TrackSubscriptions, source); @@ -193,8 +167,14 @@ private Message GetMessage( public void Add(Action handler, ChannelMessageQueue queue) { - if (handler != null) _handlers += handler; - if (queue != null) ChannelMessageQueue.Combine(ref _queues, queue); + if (handler != null) + { + _handlers += handler; + } + if (queue != null) + { + ChannelMessageQueue.Combine(ref _queues, queue); + } } public ICompletable ForInvoke(in RedisChannel channel, in RedisValue message, out ChannelMessageQueue queues) @@ -212,35 +192,42 @@ internal void MarkCompleted() public bool Remove(Action handler, ChannelMessageQueue queue) { - if (handler != null) _handlers -= handler; - if (queue != null) ChannelMessageQueue.Remove(ref _queues, queue); + if (handler != null) + { + _handlers -= handler; + } + if (queue != null) + { + ChannelMessageQueue.Remove(ref _queues, queue); + } return _handlers == null & _queues == null; } public async Task SubscribeToServerAsync(ConnectionMultiplexer multiplexer, RedisChannel channel, CommandFlags flags, object asyncState, bool internalCall) { var command = channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE; - // TODO: default(RedisKey) is incorrect here - should shard based on the channel in cluster - var selected = multiplexer.SelectServer(command, flags, default(RedisKey)); + var selected = multiplexer.SelectServer(command, flags, channel); - if (Interlocked.CompareExchange(ref CurrentServer, selected, null) != null) + // Do we have a server already? And is it connected? Then bail out. + if (CurrentServer?.IsSubscriberConnected == true) { - // Abort return false; } + // Otherwise try and subscribe on the server side try { var message = GetMessage(channel, command, asyncState, internalCall, out var taskSource); - var success = await multiplexer.ExecuteAsyncImpl(message, ResultProcessor.TrackSubscriptions, null, selected); + // TODO: Could move this entirely into a processor, e.g. the CurrentServer removal we need below + var success = await multiplexer.ExecuteAsyncImpl(message, ResultProcessor.TrackSubscriptions, asyncState, selected); if (!success) { - taskSource.SetCanceled(); + taskSource.SetResult(false); } return await taskSource.Task; } catch { - // clear the owner if it is still us + // If there was an exception, clear the owner Interlocked.CompareExchange(ref CurrentServer, null, selected); throw; } @@ -253,7 +240,7 @@ public async Task UnsubscribeFromServerAsync(RedisChannel channel, object if (oldOwner != null) { var message = GetMessage(channel, command, asyncState, internalCall, out var taskSource); - var success = await oldOwner.Multiplexer.ExecuteAsyncImpl(message, ResultProcessor.TrackSubscriptions, null, oldOwner); + var success = await oldOwner.Multiplexer.ExecuteAsyncImpl(message, ResultProcessor.TrackSubscriptions, asyncState, oldOwner); if (!success) { taskSource.SetCanceled(); @@ -314,51 +301,6 @@ internal void GetSubscriberCounts(out int handlers, out int queues) } } } - - internal string GetConnectionName(EndPoint endPoint, ConnectionType connectionType) - => GetServerEndPoint(endPoint)?.GetBridge(connectionType, false)?.PhysicalName; - - internal event Action MessageFaulted; - internal event Action Closing; - internal event Action PreTransactionExec, TransactionLog, InfoMessage; - internal event Action Connecting; - internal event Action Resurrecting; - - [Conditional("VERBOSE")] - internal void OnMessageFaulted(Message msg, Exception fault, [CallerMemberName] string origin = default, [CallerFilePath] string path = default, [CallerLineNumber] int lineNumber = default) - { - MessageFaulted?.Invoke(msg?.CommandAndKey, fault, $"{origin} ({path}#{lineNumber})"); - } - [Conditional("VERBOSE")] - internal void OnInfoMessage(string message) - { - InfoMessage?.Invoke(message); - } - [Conditional("VERBOSE")] - internal void OnClosing(bool complete) - { - Closing?.Invoke(complete); - } - [Conditional("VERBOSE")] - internal void OnConnecting(EndPoint endpoint, ConnectionType connectionType) - { - Connecting?.Invoke(endpoint, connectionType); - } - [Conditional("VERBOSE")] - internal void OnResurrecting(EndPoint endpoint, ConnectionType connectionType) - { - Resurrecting.Invoke(endpoint, connectionType); - } - [Conditional("VERBOSE")] - internal void OnPreTransactionExec(Message message) - { - PreTransactionExec?.Invoke(message.CommandAndKey); - } - [Conditional("VERBOSE")] - internal void OnTransactionLog(string message) - { - TransactionLog?.Invoke(message); - } } internal sealed class RedisSubscriber : RedisBase, ISubscriber @@ -422,14 +364,20 @@ private Message CreatePingMessage(CommandFlags flags) public long Publish(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None) { - if (channel.IsNullOrEmpty) throw new ArgumentNullException(nameof(channel)); + if (channel.IsNullOrEmpty) + { + throw new ArgumentNullException(nameof(channel)); + } var msg = Message.Create(-1, flags, RedisCommand.PUBLISH, channel, message); return ExecuteSync(msg, ResultProcessor.Int64); } public Task PublishAsync(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None) { - if (channel.IsNullOrEmpty) throw new ArgumentNullException(nameof(channel)); + if (channel.IsNullOrEmpty) + { + throw new ArgumentNullException(nameof(channel)); + } var msg = Message.Create(-1, flags, RedisCommand.PUBLISH, channel, message); return ExecuteAsync(msg, ResultProcessor.Int64); } @@ -455,7 +403,10 @@ Task ISubscriber.SubscribeAsync(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) { - if (channel.IsNullOrEmpty) throw new ArgumentNullException(nameof(channel)); + if (channel.IsNullOrEmpty) + { + throw new ArgumentNullException(nameof(channel)); + } return multiplexer.AddSubscriptionAsync(channel, handler, queue, flags, asyncState); } @@ -469,11 +420,7 @@ public async Task SubscribeAsync(RedisChannel channel, Comm return queue; } - public EndPoint SubscribedEndpoint(RedisChannel channel) - { - var server = multiplexer.GetSubscribedServer(channel); - return server?.EndPoint; - } + public EndPoint SubscribedEndpoint(RedisChannel channel) => multiplexer.GetSubscribedServer(channel)?.EndPoint; void ISubscriber.Unsubscribe(RedisChannel channel, Action handler, CommandFlags flags) => Unsubscribe(channel, handler, null, flags); @@ -490,15 +437,18 @@ public void UnsubscribeAll(CommandFlags flags = CommandFlags.None) if ((flags & CommandFlags.FireAndForget) == 0) Wait(task); } - public Task UnsubscribeAllAsync(CommandFlags flags = CommandFlags.None) => - multiplexer.RemoveAllSubscriptions(flags, asyncState); + public Task UnsubscribeAllAsync(CommandFlags flags = CommandFlags.None) => multiplexer.RemoveAllSubscriptionsAsync(flags, asyncState); Task ISubscriber.UnsubscribeAsync(RedisChannel channel, Action handler, CommandFlags flags) => UnsubscribeAsync(channel, handler, null, flags); + public Task UnsubscribeAsync(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) { - if (channel.IsNullOrEmpty) throw new ArgumentNullException(nameof(channel)); - return multiplexer.RemoveSubscription(channel, handler, queue, flags, asyncState); + if (channel.IsNullOrEmpty) + { + throw new ArgumentNullException(nameof(channel)); + } + return multiplexer.RemoveSubscriptionAsync(channel, handler, queue, flags, asyncState); } } } diff --git a/src/StackExchange.Redis/ServerSelectionStrategy.cs b/src/StackExchange.Redis/ServerSelectionStrategy.cs index ac9e664ca..cb4bb954c 100644 --- a/src/StackExchange.Redis/ServerSelectionStrategy.cs +++ b/src/StackExchange.Redis/ServerSelectionStrategy.cs @@ -58,19 +58,26 @@ public ServerSelectionStrategy(ConnectionMultiplexer multiplexer) internal static int TotalSlots => RedisClusterSlotCount; /// - /// Computes the hash-slot that would be used by the given key + /// Computes the hash-slot that would be used by the given key. /// /// The to determine a slot ID for. public int HashSlot(in RedisKey key) - => ServerType == ServerType.Standalone ? NoSlot : GetClusterSlot(key); + => ServerType == ServerType.Standalone || key.IsNull ? NoSlot : GetClusterSlot((byte[])key); - private static unsafe int GetClusterSlot(in RedisKey key) + /// + /// Computes the hash-slot that would be used by the given channel. + /// + /// The to determine a slot ID for. + public int HashSlot(in RedisChannel channel) + => ServerType == ServerType.Standalone || channel.IsNull ? NoSlot : GetClusterSlot((byte[])channel); + + /// + /// HASH_SLOT = CRC16(key) mod 16384 + /// + private static unsafe int GetClusterSlot(byte[] blob) { - //HASH_SLOT = CRC16(key) mod 16384 - if (key.IsNull) return NoSlot; unchecked { - var blob = (byte[])key; fixed (byte* ptr = blob) { fixed (ushort* crc16tab = s_crc16tab) @@ -116,6 +123,12 @@ public ServerEndPoint Select(RedisCommand command, in RedisKey key, CommandFlags return Select(slot, command, flags); } + public ServerEndPoint Select(RedisCommand command, in RedisChannel channel, CommandFlags flags) + { + int slot = ServerType == ServerType.Cluster ? HashSlot(channel) : NoSlot; + return Select(slot, command, flags); + } + public bool TryResend(int hashSlot, Message message, EndPoint endpoint, bool isMoved) { try From f24798094c4093bf155329ac80388233731fff84 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Thu, 20 Jan 2022 11:06:31 -0500 Subject: [PATCH 15/56] Pub/Sub: default to 3.0, fix PING, fix server selection in cluster, and cleanup In prep for changes to how we handle subscriptions internally, this does several things: - Upgrades default Redis server assumption to 3.x - Routes PING on Subscription keepalives over the subscription bridge appropriately - Fixes cluster sharding from default(RedisKey) to shared logic for RedisChannel as well (both in byte[] form) - General code cleanup in the area (getting a lot of DEBUG/VERBOSE noise into isolated files) --- .../ConfigurationOptions.cs | 2 +- .../ConnectionMultiplexer.Threading.cs | 50 +++++++++ .../ConnectionMultiplexer.Verbose.cs | 59 ++++++++++ .../ConnectionMultiplexer.cs | 45 ++------ src/StackExchange.Redis/Enums/CommandFlags.cs | 2 + src/StackExchange.Redis/ExceptionFactory.cs | 4 +- .../Interfaces/ISubscriber.cs | 8 +- src/StackExchange.Redis/Message.cs | 12 +- src/StackExchange.Redis/PhysicalBridge.cs | 1 + src/StackExchange.Redis/RedisBatch.cs | 2 +- src/StackExchange.Redis/RedisSubscriber.cs | 82 +------------- src/StackExchange.Redis/ServerEndPoint.cs | 56 ++++++--- .../ServerSelectionStrategy.cs | 25 ++++- tests/StackExchange.Redis.Tests/AsyncTests.cs | 2 +- tests/StackExchange.Redis.Tests/Config.cs | 2 +- .../ConnectFailTimeout.cs | 2 +- .../ConnectingFailDetection.cs | 6 +- .../ConnectionShutdown.cs | 2 +- tests/StackExchange.Redis.Tests/PubSub.cs | 106 +++++++++++++++--- tests/StackExchange.Redis.Tests/TestBase.cs | 5 + 20 files changed, 308 insertions(+), 165 deletions(-) create mode 100644 src/StackExchange.Redis/ConnectionMultiplexer.Threading.cs create mode 100644 src/StackExchange.Redis/ConnectionMultiplexer.Verbose.cs diff --git a/src/StackExchange.Redis/ConfigurationOptions.cs b/src/StackExchange.Redis/ConfigurationOptions.cs index d733353cc..138096120 100644 --- a/src/StackExchange.Redis/ConfigurationOptions.cs +++ b/src/StackExchange.Redis/ConfigurationOptions.cs @@ -306,7 +306,7 @@ public int ConnectTimeout /// public Version DefaultVersion { - get => defaultVersion ?? (IsAzureEndpoint() ? RedisFeatures.v4_0_0 : RedisFeatures.v2_8_0); + get => defaultVersion ?? (IsAzureEndpoint() ? RedisFeatures.v4_0_0 : RedisFeatures.v3_0_0); set => defaultVersion = value; } diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.Threading.cs b/src/StackExchange.Redis/ConnectionMultiplexer.Threading.cs new file mode 100644 index 000000000..f23d010cc --- /dev/null +++ b/src/StackExchange.Redis/ConnectionMultiplexer.Threading.cs @@ -0,0 +1,50 @@ +using System; +using System.Threading; +using Pipelines.Sockets.Unofficial; + +namespace StackExchange.Redis +{ + public partial class ConnectionMultiplexer + { + private static readonly WaitCallback s_CompleteAsWorker = s => ((ICompletable)s).TryComplete(true); + internal static void CompleteAsWorker(ICompletable completable) + { + if (completable != null) + { + ThreadPool.QueueUserWorkItem(s_CompleteAsWorker, completable); + } + } + + internal static bool TryCompleteHandler(EventHandler handler, object sender, T args, bool isAsync) where T : EventArgs, ICompletable + { + if (handler == null) return true; + if (isAsync) + { + if (handler.IsSingle()) + { + try + { + handler(sender, args); + } + catch { } + } + else + { + foreach (EventHandler sub in handler.AsEnumerable()) + { + try + { + sub(sender, args); + } + catch { } + } + } + return true; + } + else + { + return false; + } + } + } +} diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.Verbose.cs b/src/StackExchange.Redis/ConnectionMultiplexer.Verbose.cs new file mode 100644 index 000000000..7a61096b6 --- /dev/null +++ b/src/StackExchange.Redis/ConnectionMultiplexer.Verbose.cs @@ -0,0 +1,59 @@ +using System; +using System.Diagnostics; +using System.Net; +using System.Runtime.CompilerServices; + +namespace StackExchange.Redis +{ + public partial class ConnectionMultiplexer + { + internal event Action MessageFaulted; + internal event Action Closing; + internal event Action PreTransactionExec, TransactionLog, InfoMessage; + internal event Action Connecting; + internal event Action Resurrecting; + + partial void OnTrace(string message, string category); + static partial void OnTraceWithoutContext(string message, string category); + + [Conditional("VERBOSE")] + internal void Trace(string message, [CallerMemberName] string category = null) => OnTrace(message, category); + + [Conditional("VERBOSE")] + internal void Trace(bool condition, string message, [CallerMemberName] string category = null) + { + if (condition) OnTrace(message, category); + } + + [Conditional("VERBOSE")] + internal static void TraceWithoutContext(string message, [CallerMemberName] string category = null) => OnTraceWithoutContext(message, category); + + [Conditional("VERBOSE")] + internal static void TraceWithoutContext(bool condition, string message, [CallerMemberName] string category = null) + { + if (condition) OnTraceWithoutContext(message, category); + } + + [Conditional("VERBOSE")] + internal void OnMessageFaulted(Message msg, Exception fault, [CallerMemberName] string origin = default, [CallerFilePath] string path = default, [CallerLineNumber] int lineNumber = default) => + MessageFaulted?.Invoke(msg?.CommandAndKey, fault, $"{origin} ({path}#{lineNumber})"); + + [Conditional("VERBOSE")] + internal void OnInfoMessage(string message) => InfoMessage?.Invoke(message); + + [Conditional("VERBOSE")] + internal void OnClosing(bool complete) => Closing?.Invoke(complete); + + [Conditional("VERBOSE")] + internal void OnConnecting(EndPoint endpoint, ConnectionType connectionType) => Connecting?.Invoke(endpoint, connectionType); + + [Conditional("VERBOSE")] + internal void OnResurrecting(EndPoint endpoint, ConnectionType connectionType) => Resurrecting.Invoke(endpoint, connectionType); + + [Conditional("VERBOSE")] + internal void OnPreTransactionExec(Message message) => PreTransactionExec?.Invoke(message.CommandAndKey); + + [Conditional("VERBOSE")] + internal void OnTransactionLog(string message) => TransactionLog?.Invoke(message); + } +} diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.cs b/src/StackExchange.Redis/ConnectionMultiplexer.cs index 0cc31e76b..6e42daaed 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.cs @@ -1504,33 +1504,6 @@ public IServer GetServer(EndPoint endpoint, object asyncState = null) return new RedisServer(this, server, asyncState); } - [Conditional("VERBOSE")] - internal void Trace(string message, [CallerMemberName] string category = null) - { - OnTrace(message, category); - } - - [Conditional("VERBOSE")] - internal void Trace(bool condition, string message, [CallerMemberName] string category = null) - { - if (condition) OnTrace(message, category); - } - - partial void OnTrace(string message, string category); - static partial void OnTraceWithoutContext(string message, string category); - - [Conditional("VERBOSE")] - internal static void TraceWithoutContext(string message, [CallerMemberName] string category = null) - { - OnTraceWithoutContext(message, category); - } - - [Conditional("VERBOSE")] - internal static void TraceWithoutContext(bool condition, string message, [CallerMemberName] string category = null) - { - if (condition) OnTraceWithoutContext(message, category); - } - /// /// The number of operations that have been performed on all connections /// @@ -1773,7 +1746,7 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, LogP { var server = servers[i]; var task = available[i]; - var bs = server.GetBridgeStatus(RedisCommand.PING); + var bs = server.GetBridgeStatus(ConnectionType.Interactive); log?.WriteLine($" Server[{i}] ({Format.ToString(server)}) Status: {task.Status} (inst: {bs.MessagesSinceLastHeartbeat}, qs: {bs.Connection.MessagesSentAwaitingResponse}, in: {bs.Connection.BytesAvailableOnSocket}, qu: {bs.MessagesSinceLastHeartbeat}, aw: {bs.IsWriterActive}, in-pipe: {bs.Connection.BytesInReadPipe}, out-pipe: {bs.Connection.BytesInWritePipe}, bw: {bs.BacklogStatus}, rs: {bs.Connection.ReadStatus}. ws: {bs.Connection.WriteStatus})"); } @@ -2174,16 +2147,14 @@ internal void UpdateClusterRange(ClusterConfiguration configuration) private IDisposable pulse; - internal ServerEndPoint SelectServer(Message message) - { - if (message == null) return null; - return ServerSelectionStrategy.Select(message); - } + internal ServerEndPoint SelectServer(Message message) => + message == null ? null : ServerSelectionStrategy.Select(message); - internal ServerEndPoint SelectServer(RedisCommand command, CommandFlags flags, in RedisKey key) - { - return ServerSelectionStrategy.Select(command, key, flags); - } + internal ServerEndPoint SelectServer(RedisCommand command, CommandFlags flags, in RedisKey key) => + ServerSelectionStrategy.Select(command, key, flags); + + internal ServerEndPoint SelectServer(RedisCommand command, CommandFlags flags, in RedisChannel channel) => + ServerSelectionStrategy.Select(command, channel, flags); private bool PrepareToPushMessageToBridge(Message message, ResultProcessor processor, IResultBox resultBox, ref ServerEndPoint server) { diff --git a/src/StackExchange.Redis/Enums/CommandFlags.cs b/src/StackExchange.Redis/Enums/CommandFlags.cs index f0a670d76..c1efc65c1 100644 --- a/src/StackExchange.Redis/Enums/CommandFlags.cs +++ b/src/StackExchange.Redis/Enums/CommandFlags.cs @@ -82,5 +82,7 @@ public enum CommandFlags NoScriptCache = 512, // 1024: used for timed-out; never user-specified, so not visible on the public API + + // 2048: Use subscription connection type; never user-specified, so not visible on the public API } } diff --git a/src/StackExchange.Redis/ExceptionFactory.cs b/src/StackExchange.Redis/ExceptionFactory.cs index 4cc274d24..fe7aabc3c 100644 --- a/src/StackExchange.Redis/ExceptionFactory.cs +++ b/src/StackExchange.Redis/ExceptionFactory.cs @@ -312,7 +312,7 @@ ServerEndPoint server // Add server data, if we have it if (server != null && message != null) { - var bs = server.GetBridgeStatus(message.Command); + var bs = server.GetBridgeStatus(message.IsForSubscriptionBridge ? ConnectionType.Subscription: ConnectionType.Interactive); switch (bs.Connection.ReadStatus) { @@ -338,7 +338,7 @@ ServerEndPoint server if (multiplexer.StormLogThreshold >= 0 && bs.Connection.MessagesSentAwaitingResponse >= multiplexer.StormLogThreshold && Interlocked.CompareExchange(ref multiplexer.haveStormLog, 1, 0) == 0) { - var log = server.GetStormLog(message.Command); + var log = server.GetStormLog(message); if (string.IsNullOrWhiteSpace(log)) Interlocked.Exchange(ref multiplexer.haveStormLog, 0); else Interlocked.Exchange(ref multiplexer.stormLogSnapshot, log); } diff --git a/src/StackExchange.Redis/Interfaces/ISubscriber.cs b/src/StackExchange.Redis/Interfaces/ISubscriber.cs index b479a8d8d..11e985a0b 100644 --- a/src/StackExchange.Redis/Interfaces/ISubscriber.cs +++ b/src/StackExchange.Redis/Interfaces/ISubscriber.cs @@ -100,8 +100,8 @@ public interface ISubscriber : IRedis EndPoint SubscribedEndpoint(RedisChannel channel); /// - /// Unsubscribe from a specified message channel; note; if no handler is specified, the subscription is cancelled regardless - /// of the subscribers; if a handler is specified, the subscription is only cancelled if this handler is the + /// Unsubscribe from a specified message channel; note; if no handler is specified, the subscription is canceled regardless + /// of the subscribers; if a handler is specified, the subscription is only canceled if this handler is the /// last handler remaining against the channel /// /// The channel that was subscribed to. @@ -128,8 +128,8 @@ public interface ISubscriber : IRedis Task UnsubscribeAllAsync(CommandFlags flags = CommandFlags.None); /// - /// Unsubscribe from a specified message channel; note; if no handler is specified, the subscription is cancelled regardless - /// of the subscribers; if a handler is specified, the subscription is only cancelled if this handler is the + /// Unsubscribe from a specified message channel; note; if no handler is specified, the subscription is canceled regardless + /// of the subscribers; if a handler is specified, the subscription is only canceled if this handler is the /// last handler remaining against the channel /// /// The channel that was subscribed to. diff --git a/src/StackExchange.Redis/Message.cs b/src/StackExchange.Redis/Message.cs index 86336d5c6..a7ecaf824 100644 --- a/src/StackExchange.Redis/Message.cs +++ b/src/StackExchange.Redis/Message.cs @@ -74,7 +74,8 @@ internal void SetBacklogState(int position, PhysicalConnection physical) private const CommandFlags AskingFlag = (CommandFlags)32, ScriptUnavailableFlag = (CommandFlags)256, - NeedsAsyncTimeoutCheckFlag = (CommandFlags)1024; + NeedsAsyncTimeoutCheckFlag = (CommandFlags)1024, + DemandSubscriptionConnection = (CommandFlags)2048; private const CommandFlags MaskMasterServerPreference = CommandFlags.DemandMaster | CommandFlags.DemandReplica @@ -670,6 +671,15 @@ internal void SetWriteTime() private int _writeTickCount; public int GetWriteTime() => Volatile.Read(ref _writeTickCount); + /// + /// Gets if this command should be sent over the subscription bridge. + /// + internal bool IsForSubscriptionBridge => (Flags & DemandSubscriptionConnection) != 0; + /// + /// Sends this command to the subscription connection rather than the interactive. + /// + internal void SetForSubscriptionBridge() => Flags |= DemandSubscriptionConnection; + private void SetNeedsTimeoutCheck() => Flags |= NeedsAsyncTimeoutCheckFlag; internal bool HasAsyncTimedOut(int now, int timeoutMilliseconds, out int millisecondsTaken) { diff --git a/src/StackExchange.Redis/PhysicalBridge.cs b/src/StackExchange.Redis/PhysicalBridge.cs index 0e25bd189..38abeb672 100644 --- a/src/StackExchange.Redis/PhysicalBridge.cs +++ b/src/StackExchange.Redis/PhysicalBridge.cs @@ -364,6 +364,7 @@ internal void KeepAlive() if (commandMap.IsAvailable(RedisCommand.PING) && features.PingOnSubscriber) { msg = Message.Create(-1, CommandFlags.FireAndForget, RedisCommand.PING); + msg.SetForSubscriptionBridge(); msg.SetSource(ResultProcessor.Tracer, null); } else if (commandMap.IsAvailable(RedisCommand.UNSUBSCRIBE)) diff --git a/src/StackExchange.Redis/RedisBatch.cs b/src/StackExchange.Redis/RedisBatch.cs index 6f4d70700..7abe234c5 100644 --- a/src/StackExchange.Redis/RedisBatch.cs +++ b/src/StackExchange.Redis/RedisBatch.cs @@ -30,7 +30,7 @@ public void Execute() FailNoServer(snapshot); throw ExceptionFactory.NoConnectionAvailable(multiplexer, message, server); } - var bridge = server.GetBridge(message.Command); + var bridge = server.GetBridge(message); if (bridge == null) { FailNoServer(snapshot); diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 22b9664b7..2d6788d25 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -1,8 +1,6 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.Net; -using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using Pipelines.Sockets.Unofficial; @@ -13,34 +11,11 @@ public partial class ConnectionMultiplexer { private readonly Dictionary subscriptions = new Dictionary(); - internal static void CompleteAsWorker(ICompletable completable) + internal int GetSubscriptionsCount() { - if (completable != null) ThreadPool.QueueUserWorkItem(s_CompleteAsWorker, completable); - } - - private static readonly WaitCallback s_CompleteAsWorker = s => ((ICompletable)s).TryComplete(true); - - internal static bool TryCompleteHandler(EventHandler handler, object sender, T args, bool isAsync) where T : EventArgs, ICompletable - { - if (handler == null) return true; - if (isAsync) - { - if (handler.IsSingle()) - { - try { handler(sender, args); } catch { } - } - else - { - foreach (EventHandler sub in handler.AsEnumerable()) - { - try { sub(sender, args); } catch { } - } - } - return true; - } - else + lock (subscriptions) { - return false; + return subscriptions.Count; } } @@ -106,7 +81,7 @@ internal void OnMessage(in RedisChannel subscription, in RedisChannel channel, i } } if (queues != null) ChannelMessageQueue.WriteAll(ref queues, channel, payload); - if (completable != null && !completable.TryComplete(false)) ConnectionMultiplexer.CompleteAsWorker(completable); + if (completable != null && !completable.TryComplete(false)) CompleteAsWorker(completable); } internal Task RemoveAllSubscriptions(CommandFlags flags, object asyncState) @@ -169,7 +144,7 @@ internal void ResendSubscriptions(ServerEndPoint server) var server = GetSubscribedServer(channel); if (server != null) return server.IsConnected; - server = SelectServer(RedisCommand.SUBSCRIBE, CommandFlags.DemandMaster, default(RedisKey)); + server = SelectServer(RedisCommand.SUBSCRIBE, CommandFlags.DemandMaster, channel); return server?.IsConnected == true; } @@ -221,7 +196,7 @@ public bool Remove(Action handler, ChannelMessageQueue [System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "RCS1210:Return completed task instead of returning null.", Justification = "Intentional for efficient success check")] public Task SubscribeToServer(ConnectionMultiplexer multiplexer, in RedisChannel channel, CommandFlags flags, object asyncState, bool internalCall) { - var selected = multiplexer.SelectServer(RedisCommand.SUBSCRIBE, flags, default(RedisKey)); + var selected = multiplexer.SelectServer(RedisCommand.SUBSCRIBE, flags, channel); var bridge = selected?.GetBridge(ConnectionType.Subscription, true); if (bridge == null) return null; @@ -348,51 +323,6 @@ internal void GetSubscriberCounts(out int handlers, out int queues) } } } - - internal string GetConnectionName(EndPoint endPoint, ConnectionType connectionType) - => GetServerEndPoint(endPoint)?.GetBridge(connectionType, false)?.PhysicalName; - - internal event Action MessageFaulted; - internal event Action Closing; - internal event Action PreTransactionExec, TransactionLog, InfoMessage; - internal event Action Connecting; - internal event Action Resurrecting; - - [Conditional("VERBOSE")] - internal void OnMessageFaulted(Message msg, Exception fault, [CallerMemberName] string origin = default, [CallerFilePath] string path = default, [CallerLineNumber] int lineNumber = default) - { - MessageFaulted?.Invoke(msg?.CommandAndKey, fault, $"{origin} ({path}#{lineNumber})"); - } - [Conditional("VERBOSE")] - internal void OnInfoMessage(string message) - { - InfoMessage?.Invoke(message); - } - [Conditional("VERBOSE")] - internal void OnClosing(bool complete) - { - Closing?.Invoke(complete); - } - [Conditional("VERBOSE")] - internal void OnConnecting(EndPoint endpoint, ConnectionType connectionType) - { - Connecting?.Invoke(endpoint, connectionType); - } - [Conditional("VERBOSE")] - internal void OnResurrecting(EndPoint endpoint, ConnectionType connectionType) - { - Resurrecting.Invoke(endpoint, connectionType); - } - [Conditional("VERBOSE")] - internal void OnPreTransactionExec(Message message) - { - PreTransactionExec?.Invoke(message.CommandAndKey); - } - [Conditional("VERBOSE")] - internal void OnTransactionLog(string message) - { - TransactionLog?.Invoke(message); - } } internal sealed class RedisSubscriber : RedisBase, ISubscriber diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs index f41e360a3..ab2889e65 100755 --- a/src/StackExchange.Redis/ServerEndPoint.cs +++ b/src/StackExchange.Redis/ServerEndPoint.cs @@ -74,6 +74,8 @@ public ServerEndPoint(ConnectionMultiplexer multiplexer, EndPoint endpoint) public bool IsConnected => interactive?.IsConnected == true; + public bool IsSubscriberConnected => subscription?.IsConnected == true; + public bool IsConnecting => interactive?.IsConnecting == true; private readonly List> _pendingConnectionMonitors = new List>(); @@ -92,7 +94,7 @@ async Task IfConnectedAsync(LogProxy log, bool sendTracerIfConnected, bo } if (sendTracerIfConnected) { - await SendTracer(log).ForAwait(); + await SendTracerAsync(log).ForAwait(); } log?.WriteLine($"{Format.ToString(this)}: OnConnectedAsync already connected end"); return "Already connected"; @@ -209,6 +211,28 @@ public PhysicalBridge GetBridge(ConnectionType type, bool create = true, LogProx }; } + public PhysicalBridge GetBridge(Message message, bool create = true) + { + if (isDisposed) return null; + + // Subscription commands go to a specific bridge - so we need to set that up. + // There are other commands we need to send to the right connection (e.g. subscriber PING with an explicit SetForSubscriptionBridge call), + // but these always go subscriber. + switch (message.Command) + { + case RedisCommand.SUBSCRIBE: + case RedisCommand.UNSUBSCRIBE: + case RedisCommand.PSUBSCRIBE: + case RedisCommand.PUNSUBSCRIBE: + message.SetForSubscriptionBridge(); + break; + } + + return message.IsForSubscriptionBridge + ? subscription ?? (create ? subscription = CreateBridge(ConnectionType.Subscription, null) : null) + : interactive ?? (create ? interactive = CreateBridge(ConnectionType.Interactive, null) : null); + } + public PhysicalBridge GetBridge(RedisCommand command, bool create = true) { if (isDisposed) return null; @@ -281,9 +305,9 @@ public void SetUnselectable(UnselectableFlags flags) public override string ToString() => Format.ToString(EndPoint); [Obsolete("prefer async")] - public WriteResult TryWriteSync(Message message) => GetBridge(message.Command)?.TryWriteSync(message, isReplica) ?? WriteResult.NoConnectionAvailable; + public WriteResult TryWriteSync(Message message) => GetBridge(message)?.TryWriteSync(message, isReplica) ?? WriteResult.NoConnectionAvailable; - public ValueTask TryWriteAsync(Message message) => GetBridge(message.Command)?.TryWriteAsync(message, isReplica) ?? new ValueTask(WriteResult.NoConnectionAvailable); + public ValueTask TryWriteAsync(Message message) => GetBridge(message)?.TryWriteAsync(message, isReplica) ?? new ValueTask(WriteResult.NoConnectionAvailable); internal void Activate(ConnectionType type, LogProxy log) { @@ -445,11 +469,11 @@ internal ServerCounters GetCounters() return counters; } - internal BridgeStatus GetBridgeStatus(RedisCommand command) + internal BridgeStatus GetBridgeStatus(ConnectionType connectionType) { try { - return GetBridge(command, false)?.GetStatus() ?? BridgeStatus.Zero; + return GetBridge(connectionType, false)?.GetStatus() ?? BridgeStatus.Zero; } catch (Exception ex) { // only needs to be best efforts @@ -484,9 +508,9 @@ internal byte[] GetScriptHash(string script, RedisCommand command) return found; } - internal string GetStormLog(RedisCommand command) + internal string GetStormLog(Message message) { - var bridge = GetBridge(command); + var bridge = GetBridge(message); return bridge?.GetStormLog(); } @@ -686,7 +710,7 @@ internal void OnHeartbeat() } } - internal Task WriteDirectAsync(Message message, ResultProcessor processor, object asyncState = null, PhysicalBridge bridge = null) + internal Task WriteDirectAsync(Message message, ResultProcessor processor, PhysicalBridge bridge = null) { static async Task Awaited(ServerEndPoint @this, Message message, ValueTask write, TaskCompletionSource tcs) { @@ -699,9 +723,9 @@ static async Task Awaited(ServerEndPoint @this, Message message, ValueTask.Create(out var tcs, asyncState); + var source = TaskResultBox.Create(out var tcs, null); message.SetSource(processor, source); - if (bridge == null) bridge = GetBridge(message.Command); + if (bridge == null) bridge = GetBridge(message); WriteResult result; if (bridge == null) @@ -733,7 +757,7 @@ internal void WriteDirectFireAndForgetSync(Message message, ResultProcessor SendTracer(LogProxy log = null) + internal Task SendTracerAsync(LogProxy log = null) { var msg = GetTracerMessage(false); msg = LoggingMessage.Create(log, msg); @@ -783,6 +807,9 @@ internal string Summary() return sb.ToString(); } + /// + /// Write the message directly to the pipe or fail...will not queue. + /// internal ValueTask WriteDirectOrQueueFireAndForgetAsync(PhysicalConnection connection, Message message, ResultProcessor processor) { static async ValueTask Awaited(ValueTask l_result) => await l_result.ForAwait(); @@ -794,7 +821,7 @@ internal ValueTask WriteDirectOrQueueFireAndForgetAsync(PhysicalConnection co if (connection == null) { Multiplexer.Trace($"{Format.ToString(this)}: Enqueue (async): " + message); - result = GetBridge(message.Command).TryWriteAsync(message, isReplica); + result = GetBridge(message).TryWriteAsync(message, isReplica); } else { @@ -886,7 +913,7 @@ private async Task HandshakeAsync(PhysicalConnection connection, LogProxy log) log?.WriteLine($"{Format.ToString(this)}: Sending critical tracer (handshake): {tracer.CommandAndKey}"); await WriteDirectOrQueueFireAndForgetAsync(connection, tracer, ResultProcessor.EstablishConnection).ForAwait(); - // note: this **must** be the last thing on the subscription handshake, because after this + // Note: this **must** be the last thing on the subscription handshake, because after this // we will be in subscriber mode: regular commands cannot be sent if (connType == ConnectionType.Subscription) { @@ -894,6 +921,7 @@ private async Task HandshakeAsync(PhysicalConnection connection, LogProxy log) if (configChannel != null) { msg = Message.Create(-1, CommandFlags.FireAndForget, RedisCommand.SUBSCRIBE, (RedisChannel)configChannel); + // Note: this is NOT internal, we want it to queue in a backlog for sending when ready if necessary await WriteDirectOrQueueFireAndForgetAsync(connection, msg, ResultProcessor.TrackSubscriptions).ForAwait(); } } diff --git a/src/StackExchange.Redis/ServerSelectionStrategy.cs b/src/StackExchange.Redis/ServerSelectionStrategy.cs index ac9e664ca..cb4bb954c 100644 --- a/src/StackExchange.Redis/ServerSelectionStrategy.cs +++ b/src/StackExchange.Redis/ServerSelectionStrategy.cs @@ -58,19 +58,26 @@ public ServerSelectionStrategy(ConnectionMultiplexer multiplexer) internal static int TotalSlots => RedisClusterSlotCount; /// - /// Computes the hash-slot that would be used by the given key + /// Computes the hash-slot that would be used by the given key. /// /// The to determine a slot ID for. public int HashSlot(in RedisKey key) - => ServerType == ServerType.Standalone ? NoSlot : GetClusterSlot(key); + => ServerType == ServerType.Standalone || key.IsNull ? NoSlot : GetClusterSlot((byte[])key); - private static unsafe int GetClusterSlot(in RedisKey key) + /// + /// Computes the hash-slot that would be used by the given channel. + /// + /// The to determine a slot ID for. + public int HashSlot(in RedisChannel channel) + => ServerType == ServerType.Standalone || channel.IsNull ? NoSlot : GetClusterSlot((byte[])channel); + + /// + /// HASH_SLOT = CRC16(key) mod 16384 + /// + private static unsafe int GetClusterSlot(byte[] blob) { - //HASH_SLOT = CRC16(key) mod 16384 - if (key.IsNull) return NoSlot; unchecked { - var blob = (byte[])key; fixed (byte* ptr = blob) { fixed (ushort* crc16tab = s_crc16tab) @@ -116,6 +123,12 @@ public ServerEndPoint Select(RedisCommand command, in RedisKey key, CommandFlags return Select(slot, command, flags); } + public ServerEndPoint Select(RedisCommand command, in RedisChannel channel, CommandFlags flags) + { + int slot = ServerType == ServerType.Cluster ? HashSlot(channel) : NoSlot; + return Select(slot, command, flags); + } + public bool TryResend(int hashSlot, Message message, EndPoint endpoint, bool isMoved) { try diff --git a/tests/StackExchange.Redis.Tests/AsyncTests.cs b/tests/StackExchange.Redis.Tests/AsyncTests.cs index 5ee26f815..4dd36670b 100644 --- a/tests/StackExchange.Redis.Tests/AsyncTests.cs +++ b/tests/StackExchange.Redis.Tests/AsyncTests.cs @@ -19,7 +19,7 @@ public void AsyncTasksReportFailureIfServerUnavailable() { SetExpectedAmbientFailureCount(-1); // this will get messy - using (var conn = Create(allowAdmin: true)) + using (var conn = Create(allowAdmin: true, shared: false)) { var server = conn.GetServer(TestConfig.Current.MasterServer, TestConfig.Current.MasterPort); diff --git a/tests/StackExchange.Redis.Tests/Config.cs b/tests/StackExchange.Redis.Tests/Config.cs index a2d0d7034..2a5cf6625 100644 --- a/tests/StackExchange.Redis.Tests/Config.cs +++ b/tests/StackExchange.Redis.Tests/Config.cs @@ -14,7 +14,7 @@ namespace StackExchange.Redis.Tests { public class Config : TestBase { - public Version DefaultVersion = new (2, 8, 0); + public Version DefaultVersion = new (3, 0, 0); public Version DefaultAzureVersion = new (4, 0, 0); public Config(ITestOutputHelper output) : base(output) { } diff --git a/tests/StackExchange.Redis.Tests/ConnectFailTimeout.cs b/tests/StackExchange.Redis.Tests/ConnectFailTimeout.cs index c52082d12..73af84fa4 100644 --- a/tests/StackExchange.Redis.Tests/ConnectFailTimeout.cs +++ b/tests/StackExchange.Redis.Tests/ConnectFailTimeout.cs @@ -13,7 +13,7 @@ public ConnectFailTimeout(ITestOutputHelper output) : base (output) { } public async Task NoticesConnectFail() { SetExpectedAmbientFailureCount(-1); - using (var conn = Create(allowAdmin: true)) + using (var conn = Create(allowAdmin: true, shared: false)) { var server = conn.GetServer(conn.GetEndPoints()[0]); diff --git a/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs b/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs index 9d9c88f8c..fb0b84d21 100644 --- a/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs +++ b/tests/StackExchange.Redis.Tests/ConnectingFailDetection.cs @@ -105,17 +105,17 @@ public async Task Issue922_ReconnectRaised() int failCount = 0, restoreCount = 0; - using (var muxer = ConnectionMultiplexer.Connect(config, log: Writer)) + using (var muxer = ConnectionMultiplexer.Connect(config)) { muxer.ConnectionFailed += (s, e) => { Interlocked.Increment(ref failCount); - Log($"Connection Failed ({e.ConnectionType},{e.FailureType}): {e.Exception}"); + Log($"Connection Failed ({e.ConnectionType}, {e.FailureType}): {e.Exception}"); }; muxer.ConnectionRestored += (s, e) => { Interlocked.Increment(ref restoreCount); - Log($"Connection Restored ({e.ConnectionType},{e.FailureType}): {e.Exception}"); + Log($"Connection Restored ({e.ConnectionType}, {e.FailureType})"); }; muxer.GetDatabase(); diff --git a/tests/StackExchange.Redis.Tests/ConnectionShutdown.cs b/tests/StackExchange.Redis.Tests/ConnectionShutdown.cs index a4e720772..d75054ca4 100644 --- a/tests/StackExchange.Redis.Tests/ConnectionShutdown.cs +++ b/tests/StackExchange.Redis.Tests/ConnectionShutdown.cs @@ -14,7 +14,7 @@ public ConnectionShutdown(ITestOutputHelper output) : base(output) { } [Fact(Skip = "Unfriendly")] public async Task ShutdownRaisesConnectionFailedAndRestore() { - using (var conn = Create(allowAdmin: true)) + using (var conn = Create(allowAdmin: true, shared: false)) { int failed = 0, restored = 0; Stopwatch watch = Stopwatch.StartNew(); diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 0e4131913..646041055 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -1,11 +1,13 @@ using System; using System.Collections.Generic; using System.Diagnostics; +using System.Linq; using System.Text; using System.Threading; using System.Threading.Channels; using System.Threading.Tasks; using StackExchange.Redis.Maintenance; +using StackExchange.Redis.Profiling; using Xunit; using Xunit.Abstractions; // ReSharper disable AccessToModifiedClosure @@ -55,7 +57,7 @@ await UntilCondition(TimeSpan.FromSeconds(10), [InlineData("Foo:", true, "f")] public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string breaker) { - using (var muxer = Create(channelPrefix: channelPrefix)) + using (var muxer = Create(channelPrefix: channelPrefix, log: Writer)) { var pub = GetAnyMaster(muxer); var sub = muxer.GetSubscriber(); @@ -91,6 +93,7 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br await PingAsync(muxer, pub, sub, 3).ForAwait(); + await UntilCondition(TimeSpan.FromSeconds(5), () => received.Count == 1); lock (received) { Assert.Single(received); @@ -124,7 +127,7 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br [Fact] public async Task TestBasicPubSubFireAndForget() { - using (var muxer = Create()) + using (var muxer = Create(log: Writer)) { var pub = GetAnyMaster(muxer); var sub = muxer.GetSubscriber(); @@ -155,6 +158,7 @@ public async Task TestBasicPubSubFireAndForget() var count = sub.Publish(key, "def", CommandFlags.FireAndForget); await PingAsync(muxer, pub, sub).ForAwait(); + await UntilCondition(TimeSpan.FromSeconds(5), () => received.Count == 1); lock (received) { Assert.Single(received); @@ -182,9 +186,7 @@ private static async Task PingAsync(IConnectionMultiplexer muxer, IServer pub, I // way to prove that is to use TPL objects var t1 = sub.PingAsync(); var t2 = pub.PingAsync(); - await Task.Delay(100).ForAwait(); // especially useful when testing any-order mode - - if (!Task.WaitAll(new[] { t1, t2 }, muxer.TimeoutMilliseconds * 2)) throw new TimeoutException(); + await Task.WhenAll(t1, t2).ForAwait(); } } @@ -220,6 +222,7 @@ public async Task TestPatternPubSub() var count = sub.Publish("abc", "def"); await PingAsync(muxer, pub, sub).ForAwait(); + await UntilCondition(TimeSpan.FromSeconds(5), () => received.Count == 1); lock (received) { Assert.Single(received); @@ -348,7 +351,7 @@ await sub.SubscribeAsync(channel, (_, val) => [Fact] public async Task PubSubGetAllCorrectOrder() { - using (var muxer = Create(configuration: TestConfig.Current.RemoteServerAndPort, syncTimeout: 20000)) + using (var muxer = Create(configuration: TestConfig.Current.RemoteServerAndPort, syncTimeout: 20000, log: Writer)) { var sub = muxer.GetSubscriber(); RedisChannel channel = Me(); @@ -520,6 +523,9 @@ public async Task PubSubGetAllCorrectOrder_OnMessage_Async() }); await sub.PingAsync().ForAwait(); + // Give a delay between subscriptions and when we try to publish to be safe + await Task.Delay(1000).ForAwait(); + lock (syncLock) { for (int i = 0; i < count; i++) @@ -743,8 +749,10 @@ public async Task AzureRedisEventsAutomaticSubscribe() [Fact] public async Task SubscriptionsSurviveConnectionFailureAsync() { - using (var muxer = Create(allowAdmin: true, shared: false)) + var session = new ProfilingSession(); + using (var muxer = Create(allowAdmin: true, shared: false, syncTimeout: 1000) as ConnectionMultiplexer) { + muxer.RegisterProfiler(() => session); RedisChannel channel = Me(); var sub = muxer.GetSubscriber(); int counter = 0; @@ -752,23 +760,89 @@ await sub.SubscribeAsync(channel, delegate { Interlocked.Increment(ref counter); }).ConfigureAwait(false); + + var profile1 = session.FinishProfiling(); + foreach (var command in profile1) + { + Log($"{command.EndPoint}: {command}"); + } + // We shouldn't see the initial connection here + Assert.Equal(0, profile1.Count(p => p.Command == nameof(RedisCommand.SUBSCRIBE))); + + Assert.Equal(1, muxer.GetSubscriptionsCount()); + await Task.Delay(200).ConfigureAwait(false); + await sub.PublishAsync(channel, "abc").ConfigureAwait(false); sub.Ping(); await Task.Delay(200).ConfigureAwait(false); - Assert.Equal(1, Thread.VolatileRead(ref counter)); + + var counter1 = Thread.VolatileRead(ref counter); + Log($"Expecting 1 messsage, got {counter1}"); + Assert.Equal(1, counter1); + var server = GetServer(muxer); - Assert.Equal(1, server.GetCounters().Subscription.SocketCount); + var socketCount = server.GetCounters().Subscription.SocketCount; + Log($"Expecting 1 socket, got {socketCount}"); + Assert.Equal(1, socketCount); + + // We might fail both connections or just the primary in the time period + SetExpectedAmbientFailureCount(-1); + // Make sure we fail all the way + muxer.AllowConnect = false; + Log("Failing connection"); + // Fail all connections server.SimulateConnectionFailure(SimulatedFailureType.All); - SetExpectedAmbientFailureCount(2); - await Task.Delay(200).ConfigureAwait(false); - sub.Ping(); - Assert.Equal(2, server.GetCounters().Subscription.SocketCount); - await sub.PublishAsync(channel, "abc").ConfigureAwait(false); - await Task.Delay(200).ConfigureAwait(false); + // Trigger failure + Assert.Throws(() => sub.Ping()); + Assert.False(sub.IsConnected(channel)); + + // Now reconnect... + muxer.AllowConnect = true; + Log("Waiting on reconnect"); + // Wait until we're reconnected + await UntilCondition(TimeSpan.FromSeconds(10), () => sub.IsConnected(channel)); + Log("Reconnected"); + // Ensure we're reconnected + Assert.True(sub.IsConnected(channel)); + + // And time to resubscribe... + await Task.Delay(1000).ConfigureAwait(false); + + // Ensure we've sent the subscribe command after reconnecting + var profile2 = session.FinishProfiling(); + foreach (var command in profile2) + { + Log($"{command.EndPoint}: {command}"); + } + //Assert.Equal(1, profile2.Count(p => p.Command == nameof(RedisCommand.SUBSCRIBE))); + + Log($"Issuing ping after reconnected"); sub.Ping(); - Assert.Equal(2, Thread.VolatileRead(ref counter)); + Assert.Equal(1, muxer.GetSubscriptionsCount()); + + Log("Publishing"); + var published = await sub.PublishAsync(channel, "abc").ConfigureAwait(false); + + Log($"Published to {published} subscriber(s)."); + Assert.Equal(1, published); + + // Give it a few seconds to get our messages + Log("Waiting for 2 messages"); + await UntilCondition(TimeSpan.FromSeconds(5), () => Thread.VolatileRead(ref counter) == 2); + + var counter2 = Thread.VolatileRead(ref counter); + Log($"Expecting 2 messsages, got {counter2}"); + Assert.Equal(2, counter2); + + // Log all commands at the end + Log("All commands since connecting:"); + var profile3 = session.FinishProfiling(); + foreach (var command in profile3) + { + Log($"{command.EndPoint}: {command}"); + } } } } diff --git a/tests/StackExchange.Redis.Tests/TestBase.cs b/tests/StackExchange.Redis.Tests/TestBase.cs index a74521acb..0cca4e5b7 100644 --- a/tests/StackExchange.Redis.Tests/TestBase.cs +++ b/tests/StackExchange.Redis.Tests/TestBase.cs @@ -128,6 +128,7 @@ protected void OnConnectionFailed(object sender, ConnectionFailedEventArgs e) { privateExceptions.Add($"{Time()}: Connection failed ({e.FailureType}): {EndPointCollection.ToString(e.EndPoint)}/{e.ConnectionType}: {e.Exception}"); } + Log($"Connection Failed ({e.ConnectionType},{e.FailureType}): {e.Exception}"); } protected void OnInternalError(object sender, InternalErrorEventArgs e) @@ -285,6 +286,10 @@ internal virtual IInternalConnectionMultiplexer Create( caller); muxer.InternalError += OnInternalError; muxer.ConnectionFailed += OnConnectionFailed; + muxer.ConnectionRestored += (s, e) => + { + Log($"Connection Restored ({e.ConnectionType},{e.FailureType}): {e.Exception}"); + }; return muxer; } From 6ecde2a5ed3f696ae0eb390b52342683980aa157 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Thu, 20 Jan 2022 11:15:14 -0500 Subject: [PATCH 16/56] Include PING routing --- src/StackExchange.Redis/RedisSubscriber.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 2d6788d25..9b2b9c18d 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -372,16 +372,20 @@ private Message CreatePingMessage(CommandFlags flags, out ServerEndPoint server) catch { } } + Message msg; if (usePing) { - return ResultProcessor.TimingProcessor.CreateMessage(-1, flags, RedisCommand.PING); + msg = ResultProcessor.TimingProcessor.CreateMessage(-1, flags, RedisCommand.PING); } else { // can't use regular PING, but we can unsubscribe from something random that we weren't even subscribed to... RedisValue channel = multiplexer.UniqueId; - return ResultProcessor.TimingProcessor.CreateMessage(-1, flags, RedisCommand.UNSUBSCRIBE, channel); + msg = ResultProcessor.TimingProcessor.CreateMessage(-1, flags, RedisCommand.UNSUBSCRIBE, channel); } + // Ensure the ping is sent over the intended subscriver connection, which wouldn't happen in GetBridge() by default with PING; + msg.SetForSubscriptionBridge(); + return msg; } public long Publish(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None) From f91e4c5e8c66850edf5c9555700d1abb80b8852a Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Thu, 20 Jan 2022 11:18:10 -0500 Subject: [PATCH 17/56] Revert testing change --- tests/StackExchange.Redis.Tests/PubSub.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 646041055..2ca92a430 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -523,9 +523,6 @@ public async Task PubSubGetAllCorrectOrder_OnMessage_Async() }); await sub.PingAsync().ForAwait(); - // Give a delay between subscriptions and when we try to publish to be safe - await Task.Delay(1000).ForAwait(); - lock (syncLock) { for (int i = 0; i < count; i++) From daa1b9c4ca375b56e4957e89b658ab7be50206b9 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Thu, 20 Jan 2022 11:54:41 -0500 Subject: [PATCH 18/56] Revert that bandaid test --- tests/StackExchange.Redis.Tests/PubSub.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 646041055..2ca92a430 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -523,9 +523,6 @@ public async Task PubSubGetAllCorrectOrder_OnMessage_Async() }); await sub.PingAsync().ForAwait(); - // Give a delay between subscriptions and when we try to publish to be safe - await Task.Delay(1000).ForAwait(); - lock (syncLock) { for (int i = 0; i < count; i++) From 70e1735ade62e112a3af4976d71236a3fde07135 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Thu, 20 Jan 2022 12:10:38 -0500 Subject: [PATCH 19/56] Nope. --- tests/StackExchange.Redis.Tests/PubSub.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 646041055..2ca92a430 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -523,9 +523,6 @@ public async Task PubSubGetAllCorrectOrder_OnMessage_Async() }); await sub.PingAsync().ForAwait(); - // Give a delay between subscriptions and when we try to publish to be safe - await Task.Delay(1000).ForAwait(); - lock (syncLock) { for (int i = 0; i < count; i++) From a814231c133a3fae5999388c3bd37e5df610a3b7 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Thu, 20 Jan 2022 13:47:50 -0500 Subject: [PATCH 20/56] Bits --- src/StackExchange.Redis/RedisSubscriber.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 6e36c5a92..0de5ccc5f 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -27,7 +27,7 @@ internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out internal async Task AddSubscriptionAsync(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags, object asyncState) { - if (handler != null | queue != null) + if (handler != null || queue != null) { if (!subscriptions.TryGetValue(channel, out Subscription sub)) { @@ -207,9 +207,6 @@ public async Task SubscribeToServerAsync(ConnectionMultiplexer multiplexer { var command = channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE; var selected = multiplexer.SelectServer(command, flags, channel); - // TODO: look at this case - var bridge = selected?.GetBridge(ConnectionType.Subscription, true); - if (bridge == null) return false; // Do we have a server already? And is it connected? Then bail out. if (CurrentServer?.IsSubscriberConnected == true) From 1d4b4ad63f80305ca8e9cebc560420d88907fe20 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Fri, 21 Jan 2022 17:20:20 -0500 Subject: [PATCH 21/56] Sync work stop commit (moving to laptop!) --- src/StackExchange.Redis/RedisSubscriber.cs | 76 +++++++++++++++++----- 1 file changed, 61 insertions(+), 15 deletions(-) diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 0de5ccc5f..36000e9ed 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -25,6 +25,24 @@ internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out return false; } + internal bool AddSubscription(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) + { + if (handler != null || queue != null) + { + if (!subscriptions.TryGetValue(channel, out Subscription sub)) + { + sub = new Subscription(flags); + subscriptions.TryAdd(channel, sub); + if (!sub.SubscribeToServer(this, channel, flags, false)) + { + return false; + } + } + sub.Add(handler, queue); + } + return true; + } + internal async Task AddSubscriptionAsync(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags, object asyncState) { if (handler != null || queue != null) @@ -146,12 +164,7 @@ internal sealed class Subscription public Subscription(CommandFlags flags) => Flags = flags; - private Message GetMessage( - RedisChannel channel, - RedisCommand command, - object asyncState, - bool internalCall, - out TaskCompletionSource taskSource) + private Message GetMessage(RedisChannel channel, RedisCommand command, bool internalCall) { var msg = Message.Create(-1, Flags, command, channel); msg.SetForSubscriptionBridge(); @@ -159,9 +172,6 @@ private Message GetMessage( { msg.SetInternalCall(); } - - var source = TaskResultBox.Create(out taskSource, asyncState); - msg.SetSource(ResultProcessor.TrackSubscriptions, source); return msg; } @@ -203,10 +213,34 @@ public bool Remove(Action handler, ChannelMessageQueue return _handlers == null & _queues == null; } + public bool SubscribeToServer(ConnectionMultiplexer multiplexer, RedisChannel channel, CommandFlags flags, bool internalCall) + { + ServerEndPoint selected = null; + // Do we have a server already? And is it connected? Then bail out. + if (CurrentServer?.IsSubscriberConnected == true) + { + return false; + } + // Otherwise try and subscribe on the server side + try + { + var command = channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE; + selected = multiplexer.SelectServer(command, flags, channel); + + var message = GetMessage(channel, command, internalCall); + return multiplexer.ExecuteSyncImpl(message, ResultProcessor.TrackSubscriptions, selected); + } + catch + { + // If there was an exception, clear the owner + Interlocked.CompareExchange(ref CurrentServer, null, selected); + throw; + } + } + public async Task SubscribeToServerAsync(ConnectionMultiplexer multiplexer, RedisChannel channel, CommandFlags flags, object asyncState, bool internalCall) { - var command = channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE; - var selected = multiplexer.SelectServer(command, flags, channel); + ServerEndPoint selected = null; // Do we have a server already? And is it connected? Then bail out. if (CurrentServer?.IsSubscriberConnected == true) @@ -216,7 +250,13 @@ public async Task SubscribeToServerAsync(ConnectionMultiplexer multiplexer // Otherwise try and subscribe on the server side try { - var message = GetMessage(channel, command, asyncState, internalCall, out var taskSource); + var command = channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE; + selected = multiplexer.SelectServer(command, flags, channel); + + var source = TaskResultBox.Create(out var taskSource, asyncState); + var message = GetMessage(channel, command, internalCall); + message.SetSource(ResultProcessor.TrackSubscriptions, source); + // TODO: Could move this entirely into a processor, e.g. the CurrentServer removal we need below var success = await multiplexer.ExecuteAsyncImpl(message, ResultProcessor.TrackSubscriptions, asyncState, selected); if (!success) @@ -239,7 +279,10 @@ public async Task UnsubscribeFromServerAsync(RedisChannel channel, object var oldOwner = Interlocked.Exchange(ref CurrentServer, null); if (oldOwner != null) { - var message = GetMessage(channel, command, asyncState, internalCall, out var taskSource); + var source = TaskResultBox.Create(out var taskSource, asyncState); + var message = GetMessage(channel, command, internalCall); + message.SetSource(ResultProcessor.TrackSubscriptions, source); + var success = await oldOwner.Multiplexer.ExecuteAsyncImpl(message, ResultProcessor.TrackSubscriptions, asyncState, oldOwner); if (!success) { @@ -387,8 +430,11 @@ void ISubscriber.Subscribe(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) { - var task = SubscribeAsync(channel, handler, queue, flags); - if ((flags & CommandFlags.FireAndForget) == 0) Wait(task); + if (channel.IsNullOrEmpty) + { + throw new ArgumentNullException(nameof(channel)); + } + multiplexer.AddSubscription(channel, handler, queue, flags); } public ChannelMessageQueue Subscribe(RedisChannel channel, CommandFlags flags = CommandFlags.None) From 5de45a26019911766c4e3e3282237e0ed42a667b Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 13:48:07 -0500 Subject: [PATCH 22/56] Tests: profiler logging made easier --- .../Profiling/ProfiledCommand.cs | 23 +++++++++---------- .../Helpers/TextWriterOutputHelper.cs | 14 +++++++++++ tests/StackExchange.Redis.Tests/TestBase.cs | 11 +++++++++ .../TestExtensions.cs | 15 ++++++++++++ 4 files changed, 51 insertions(+), 12 deletions(-) create mode 100644 tests/StackExchange.Redis.Tests/TestExtensions.cs diff --git a/src/StackExchange.Redis/Profiling/ProfiledCommand.cs b/src/StackExchange.Redis/Profiling/ProfiledCommand.cs index 5f4ff899f..4c1d366f9 100644 --- a/src/StackExchange.Redis/Profiling/ProfiledCommand.cs +++ b/src/StackExchange.Redis/Profiling/ProfiledCommand.cs @@ -55,6 +55,7 @@ private static TimeSpan GetElapsedTime(long timestampDelta) private long RequestSentTimeStamp; private long ResponseReceivedTimeStamp; private long CompletedTimeStamp; + private ConnectionType? ConnectionType; private readonly ProfilingSession PushToWhenFinished; @@ -86,7 +87,11 @@ public void SetMessage(Message msg) MessageCreatedTimeStamp = msg.CreatedTimestamp; } - public void SetEnqueued() => SetTimestamp(ref EnqueuedTimeStamp); + public void SetEnqueued(ConnectionType? connType) + { + SetTimestamp(ref EnqueuedTimeStamp); + ConnectionType = connType; + } public void SetRequestSent() => SetTimestamp(ref RequestSentTimeStamp); @@ -117,16 +122,10 @@ public void SetCompleted() } public override string ToString() => -$@"EndPoint = {EndPoint} -Db = {Db} -Command = {Command} -CommandCreated = {CommandCreated:u} -CreationToEnqueued = {CreationToEnqueued} -EnqueuedToSending = {EnqueuedToSending} -SentToResponse = {SentToResponse} -ResponseToCompletion = {ResponseToCompletion} -ElapsedTime = {ElapsedTime} -Flags = {Flags} -RetransmissionOf = ({RetransmissionOf?.ToString() ?? "nothing"})"; +$@"{Command} (DB: {Db}, Flags: {Flags}) + EndPoint = {EndPoint} ({ConnectionType}) + Created = {CommandCreated:HH:mm:ss.ffff} + ElapsedTime = {ElapsedTime.TotalMilliseconds} ms (CreationToEnqueued: {CreationToEnqueued.TotalMilliseconds} ms, EnqueuedToSending: {EnqueuedToSending.TotalMilliseconds} ms, SentToResponse: {SentToResponse.TotalMilliseconds} ms, ResponseToCompletion = {ResponseToCompletion.TotalMilliseconds} ms){(RetransmissionOf != null ? @" + RetransmissionOf = " + RetransmissionOf : "")}"; } } diff --git a/tests/StackExchange.Redis.Tests/Helpers/TextWriterOutputHelper.cs b/tests/StackExchange.Redis.Tests/Helpers/TextWriterOutputHelper.cs index 7d89a187a..b0c27f6fa 100644 --- a/tests/StackExchange.Redis.Tests/Helpers/TextWriterOutputHelper.cs +++ b/tests/StackExchange.Redis.Tests/Helpers/TextWriterOutputHelper.cs @@ -20,6 +20,20 @@ public TextWriterOutputHelper(ITestOutputHelper outputHelper, bool echoToConsole public void EchoTo(StringBuilder sb) => Echo = sb; + public void WriteLineNoTime(string value) + { + try + { + base.WriteLine(value); + } + catch (Exception ex) + { + Console.Write("Attempted to write: "); + Console.WriteLine(value); + Console.WriteLine(ex); + } + } + public override void WriteLine(string value) { try diff --git a/tests/StackExchange.Redis.Tests/TestBase.cs b/tests/StackExchange.Redis.Tests/TestBase.cs index 0cca4e5b7..dd04fa9da 100644 --- a/tests/StackExchange.Redis.Tests/TestBase.cs +++ b/tests/StackExchange.Redis.Tests/TestBase.cs @@ -8,6 +8,7 @@ using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; +using StackExchange.Redis.Profiling; using StackExchange.Redis.Tests.Helpers; using Xunit; using Xunit.Abstractions; @@ -76,6 +77,16 @@ protected void Log(string message, params object[] args) } } + protected ProfiledCommandEnumerable Log(ProfilingSession session) + { + var profile = session.FinishProfiling(); + foreach (var command in profile) + { + Writer.WriteLineNoTime(command.ToString()); + } + return profile; + } + protected void CollectGarbage() { GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced); diff --git a/tests/StackExchange.Redis.Tests/TestExtensions.cs b/tests/StackExchange.Redis.Tests/TestExtensions.cs new file mode 100644 index 000000000..b4c9707fd --- /dev/null +++ b/tests/StackExchange.Redis.Tests/TestExtensions.cs @@ -0,0 +1,15 @@ +using System; +using StackExchange.Redis.Profiling; + +namespace StackExchange.Redis.Tests +{ + public static class TestExtensions + { + public static ProfilingSession AddProfiler(this IConnectionMultiplexer mutex) + { + var session = new ProfilingSession(); + mutex.RegisterProfiler(() => session); + return session; + } + } +} From 64565dd13a882be445ae39a85be6b5b30c5695cd Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 14:12:43 -0500 Subject: [PATCH 23/56] Tests: use profiling and add more logging --- tests/StackExchange.Redis.Tests/PubSub.cs | 49 ++++++++++------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 2ca92a430..0ba6a928c 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -1,13 +1,11 @@ using System; using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using System.Text; using System.Threading; using System.Threading.Channels; using System.Threading.Tasks; using StackExchange.Redis.Maintenance; -using StackExchange.Redis.Profiling; using Xunit; using Xunit.Abstractions; // ReSharper disable AccessToModifiedClosure @@ -127,13 +125,14 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br [Fact] public async Task TestBasicPubSubFireAndForget() { - using (var muxer = Create(log: Writer)) + using (var muxer = Create(log: Writer, shared: false)) { + var profiler = muxer.AddProfiler(); var pub = GetAnyMaster(muxer); var sub = muxer.GetSubscriber(); RedisChannel key = Me() + Guid.NewGuid(); - HashSet received = new HashSet(); + HashSet received = new(); int secondHandler = 0; await PingAsync(muxer, pub, sub).ForAwait(); sub.Subscribe(key, (channel, payload) => @@ -148,6 +147,7 @@ public async Task TestBasicPubSubFireAndForget() }, CommandFlags.FireAndForget); sub.Subscribe(key, (_, __) => Interlocked.Increment(ref secondHandler), CommandFlags.FireAndForget); + Log(profiler); lock (received) { @@ -159,6 +159,8 @@ public async Task TestBasicPubSubFireAndForget() await PingAsync(muxer, pub, sub).ForAwait(); await UntilCondition(TimeSpan.FromSeconds(5), () => received.Count == 1); + Log(profiler); + lock (received) { Assert.Single(received); @@ -169,7 +171,7 @@ public async Task TestBasicPubSubFireAndForget() count = sub.Publish(key, "ghi", CommandFlags.FireAndForget); await PingAsync(muxer, pub, sub).ForAwait(); - + Log(profiler); lock (received) { Assert.Single(received); @@ -178,27 +180,30 @@ public async Task TestBasicPubSubFireAndForget() } } - private static async Task PingAsync(IConnectionMultiplexer muxer, IServer pub, ISubscriber sub, int times = 1) + private async Task PingAsync(IConnectionMultiplexer muxer, IServer pub, ISubscriber sub, int times = 1) { while (times-- > 0) { // both use async because we want to drain the completion managers, and the only // way to prove that is to use TPL objects - var t1 = sub.PingAsync(); - var t2 = pub.PingAsync(); - await Task.WhenAll(t1, t2).ForAwait(); + var subTask = sub.PingAsync(); + var pubTask = pub.PingAsync(); + await Task.WhenAll(subTask, pubTask).ForAwait(); + + Log($"Sub PING time: {subTask.Result.TotalMilliseconds} ms"); + Log($"Pub PING time: {pubTask.Result.TotalMilliseconds} ms"); } } [Fact] public async Task TestPatternPubSub() { - using (var muxer = Create()) + using (var muxer = Create(shared: false)) { var pub = GetAnyMaster(muxer); var sub = muxer.GetSubscriber(); - HashSet received = new HashSet(); + HashSet received = new(); int secondHandler = 0; sub.Subscribe("a*c", (channel, payload) => { @@ -238,7 +243,6 @@ public async Task TestPatternPubSub() { Assert.Single(received); } - Assert.Equal(0, count); } } @@ -746,10 +750,9 @@ public async Task AzureRedisEventsAutomaticSubscribe() [Fact] public async Task SubscriptionsSurviveConnectionFailureAsync() { - var session = new ProfilingSession(); using (var muxer = Create(allowAdmin: true, shared: false, syncTimeout: 1000) as ConnectionMultiplexer) { - muxer.RegisterProfiler(() => session); + var profiler = muxer.AddProfiler(); RedisChannel channel = Me(); var sub = muxer.GetSubscriber(); int counter = 0; @@ -758,11 +761,7 @@ await sub.SubscribeAsync(channel, delegate Interlocked.Increment(ref counter); }).ConfigureAwait(false); - var profile1 = session.FinishProfiling(); - foreach (var command in profile1) - { - Log($"{command.EndPoint}: {command}"); - } + var profile1 = Log(profiler); // We shouldn't see the initial connection here Assert.Equal(0, profile1.Count(p => p.Command == nameof(RedisCommand.SUBSCRIBE))); @@ -775,7 +774,7 @@ await sub.SubscribeAsync(channel, delegate await Task.Delay(200).ConfigureAwait(false); var counter1 = Thread.VolatileRead(ref counter); - Log($"Expecting 1 messsage, got {counter1}"); + Log($"Expecting 1 message, got {counter1}"); Assert.Equal(1, counter1); var server = GetServer(muxer); @@ -808,11 +807,7 @@ await sub.SubscribeAsync(channel, delegate await Task.Delay(1000).ConfigureAwait(false); // Ensure we've sent the subscribe command after reconnecting - var profile2 = session.FinishProfiling(); - foreach (var command in profile2) - { - Log($"{command.EndPoint}: {command}"); - } + var profile2 = Log(profiler); //Assert.Equal(1, profile2.Count(p => p.Command == nameof(RedisCommand.SUBSCRIBE))); Log($"Issuing ping after reconnected"); @@ -830,12 +825,12 @@ await sub.SubscribeAsync(channel, delegate await UntilCondition(TimeSpan.FromSeconds(5), () => Thread.VolatileRead(ref counter) == 2); var counter2 = Thread.VolatileRead(ref counter); - Log($"Expecting 2 messsages, got {counter2}"); + Log($"Expecting 2 messages, got {counter2}"); Assert.Equal(2, counter2); // Log all commands at the end Log("All commands since connecting:"); - var profile3 = session.FinishProfiling(); + var profile3 = profiler.FinishProfiling(); foreach (var command in profile3) { Log($"{command.EndPoint}: {command}"); From 00f851c300f1a7c3d02e28c67688015c15356940 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 14:13:51 -0500 Subject: [PATCH 24/56] Pub/Sub: Register immediately, but complete async This moves the registration bits into the result processor so it happens at the right time and we can properly FireAndForget with things still behaving. --- src/StackExchange.Redis/Message.cs | 4 +- src/StackExchange.Redis/RedisSubscriber.cs | 440 +++++++++--------- src/StackExchange.Redis/ResultProcessor.cs | 13 +- src/StackExchange.Redis/ServerEndPoint.cs | 2 +- .../Issues/Issue1101.cs | 2 +- 5 files changed, 225 insertions(+), 236 deletions(-) diff --git a/src/StackExchange.Redis/Message.cs b/src/StackExchange.Redis/Message.cs index a23c5cf20..708e4600f 100644 --- a/src/StackExchange.Redis/Message.cs +++ b/src/StackExchange.Redis/Message.cs @@ -589,7 +589,7 @@ internal bool TrySetResult(T value) internal void SetEnqueued(PhysicalConnection connection) { SetWriteTime(); - performance?.SetEnqueued(); + performance?.SetEnqueued(connection?.BridgeCouldBeNull?.ConnectionType); _enqueuedTo = connection; if (connection == null) { @@ -735,6 +735,8 @@ protected CommandChannelBase(int db, CommandFlags flags, RedisCommand command, i } public override string CommandAndKey => Command + " " + Channel; + + public override int GetHashSlot(ServerSelectionStrategy serverSelectionStrategy) => serverSelectionStrategy.HashSlot(Channel); } internal abstract class CommandKeyBase : Message diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 36000e9ed..5b75ca9b1 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -4,61 +4,50 @@ using System.Threading; using System.Threading.Tasks; using Pipelines.Sockets.Unofficial; +using static StackExchange.Redis.ConnectionMultiplexer; namespace StackExchange.Redis { public partial class ConnectionMultiplexer { - private readonly SemaphoreSlim subscriptionsAddLock = new SemaphoreSlim(1, 1); + private RedisSubscriber _defaultSubscriber; + private RedisSubscriber DefaultSubscriber => _defaultSubscriber ??= new RedisSubscriber(this, null); + private readonly ConcurrentDictionary subscriptions = new(); + internal ConcurrentDictionary GetSubscriptions() => subscriptions; internal int GetSubscriptionsCount() => subscriptions.Count; - internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out int queues) + internal Subscription GetOrAddSubscription(in RedisChannel channel, CommandFlags flags) { - if (subscriptions.TryGetValue(channel, out var sub)) + lock (subscriptions) { - sub.GetSubscriberCounts(out handlers, out queues); - return true; - } - handlers = queues = 0; - return false; - } - - internal bool AddSubscription(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) - { - if (handler != null || queue != null) - { - if (!subscriptions.TryGetValue(channel, out Subscription sub)) + if (!subscriptions.TryGetValue(channel, out var sub)) { sub = new Subscription(flags); subscriptions.TryAdd(channel, sub); - if (!sub.SubscribeToServer(this, channel, flags, false)) - { - return false; - } } - sub.Add(handler, queue); + return sub; + } + } + internal bool TryGetSubscription(in RedisChannel channel, out Subscription sub) => subscriptions.TryGetValue(channel, out sub); + internal bool TryRemoveSubscription(in RedisChannel channel, out Subscription sub) + { + lock (subscriptions) + { + return subscriptions.TryRemove(channel, out sub); } - return true; } - internal async Task AddSubscriptionAsync(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags, object asyncState) + internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out int queues) { - if (handler != null || queue != null) + if (subscriptions.TryGetValue(channel, out var sub)) { - if (!subscriptions.TryGetValue(channel, out Subscription sub)) - { - sub = new Subscription(flags); - subscriptions.TryAdd(channel, sub); - if (!(await sub.SubscribeToServerAsync(this, channel, flags, asyncState, false))) - { - return false; - } - } - sub.Add(handler, queue); + sub.GetSubscriberCounts(out handlers, out queues); + return true; } - return true; + handlers = queues = 0; + return false; } internal ServerEndPoint GetSubscribedServer(in RedisChannel channel) @@ -88,66 +77,20 @@ internal void OnMessage(in RedisChannel subscription, in RedisChannel channel, i } } - internal Task RemoveAllSubscriptionsAsync(CommandFlags flags, object asyncState) + internal void EnsureSubscriptions(CommandFlags flags = CommandFlags.None) { - Task last = null; foreach (var pair in subscriptions) { - if (subscriptions.TryRemove(pair.Key, out var sub)) - { - pair.Value.MarkCompleted(); - last = pair.Value.UnsubscribeFromServerAsync(pair.Key, asyncState, false); - } - } - return last ?? CompletedTask.Default(asyncState); - } - - internal Task RemoveSubscriptionAsync(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags, object asyncState) - { - Task task = null; - if (subscriptions.TryGetValue(channel, out Subscription sub)) - { - bool removeChannel; - if (handler == null & queue == null) // blanket wipe - { - sub.MarkCompleted(); - removeChannel = true; - } - else - { - removeChannel = sub.Remove(handler, queue); - } - // If it was the last handler or a blanket wipe, remove it. - if (removeChannel) - { - subscriptions.TryRemove(channel, out _); - task = sub.UnsubscribeFromServerAsync(channel, asyncState, false); - } + DefaultSubscriber.EnsureSubscribedToServer(pair.Value, pair.Key, flags, true); } - return task ?? CompletedTask.Default(asyncState); } - internal void ResendSubscriptions(ServerEndPoint server) - { - if (server == null) return; - foreach (var pair in subscriptions) - { - pair.Value.Resubscribe(pair.Key, server); - } - } - - internal bool SubscriberConnected(in RedisChannel channel = default(RedisChannel)) - { - var server = GetSubscribedServer(channel) ?? SelectServer(RedisCommand.SUBSCRIBE, CommandFlags.DemandMaster, channel); - return server?.IsConnected == true && server.IsSubscriberConnected; - } - - internal async Task EnsureSubscriptionsAsync() + internal async Task EnsureSubscriptionsAsync(CommandFlags flags = CommandFlags.None) { long count = 0; foreach (var pair in subscriptions) { - if (await pair.Value.EnsureSubscribedAsync(this, pair.Key)) + if (await DefaultSubscriber.EnsureSubscribedToServerAsync(pair.Value, pair.Key, flags, true)) { count++; } @@ -155,18 +98,43 @@ internal async Task EnsureSubscriptionsAsync() return count; } + internal enum SubscriptionAction + { + Subscribe, + Unsubscribe + } + internal sealed class Subscription { private Action _handlers; private ChannelMessageQueue _queues; private ServerEndPoint CurrentServer; public CommandFlags Flags { get; } + public ResultProcessor.TrackSubscriptionsProcessor Processor { get; } - public Subscription(CommandFlags flags) => Flags = flags; + internal bool IsConnected => CurrentServer?.IsSubscriberConnected == true; + + public Subscription(CommandFlags flags) + { + Flags = flags; + Processor = new ResultProcessor.TrackSubscriptionsProcessor(this); + } - private Message GetMessage(RedisChannel channel, RedisCommand command, bool internalCall) + internal Message GetMessage(RedisChannel channel, SubscriptionAction action, CommandFlags flags, bool internalCall) { - var msg = Message.Create(-1, Flags, command, channel); + var isPattern = channel.IsPatternBased; + var command = action switch + { + SubscriptionAction.Subscribe when isPattern => RedisCommand.PSUBSCRIBE, + SubscriptionAction.Unsubscribe when isPattern => RedisCommand.PUNSUBSCRIBE, + + SubscriptionAction.Subscribe when !isPattern => RedisCommand.SUBSCRIBE, + SubscriptionAction.Unsubscribe when !isPattern => RedisCommand.UNSUBSCRIBE, + _ => throw new ArgumentOutOfRangeException("This would be an impressive boolean feat"), + }; + + // TODO: Consider flags here - we need to pass Fire and Forget, but don't want to intermingle Primary/Replica + var msg = Message.Create(-1, Flags | flags, command, channel); msg.SetForSubscriptionBridge(); if (internalCall) { @@ -175,6 +143,8 @@ private Message GetMessage(RedisChannel channel, RedisCommand command, bool inte return msg; } + internal void SetServer(ServerEndPoint server) => CurrentServer = server; + public void Add(Action handler, ChannelMessageQueue queue) { if (handler != null) @@ -213,118 +183,8 @@ public bool Remove(Action handler, ChannelMessageQueue return _handlers == null & _queues == null; } - public bool SubscribeToServer(ConnectionMultiplexer multiplexer, RedisChannel channel, CommandFlags flags, bool internalCall) - { - ServerEndPoint selected = null; - // Do we have a server already? And is it connected? Then bail out. - if (CurrentServer?.IsSubscriberConnected == true) - { - return false; - } - // Otherwise try and subscribe on the server side - try - { - var command = channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE; - selected = multiplexer.SelectServer(command, flags, channel); - - var message = GetMessage(channel, command, internalCall); - return multiplexer.ExecuteSyncImpl(message, ResultProcessor.TrackSubscriptions, selected); - } - catch - { - // If there was an exception, clear the owner - Interlocked.CompareExchange(ref CurrentServer, null, selected); - throw; - } - } - - public async Task SubscribeToServerAsync(ConnectionMultiplexer multiplexer, RedisChannel channel, CommandFlags flags, object asyncState, bool internalCall) - { - ServerEndPoint selected = null; - - // Do we have a server already? And is it connected? Then bail out. - if (CurrentServer?.IsSubscriberConnected == true) - { - return false; - } - // Otherwise try and subscribe on the server side - try - { - var command = channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE; - selected = multiplexer.SelectServer(command, flags, channel); - - var source = TaskResultBox.Create(out var taskSource, asyncState); - var message = GetMessage(channel, command, internalCall); - message.SetSource(ResultProcessor.TrackSubscriptions, source); - - // TODO: Could move this entirely into a processor, e.g. the CurrentServer removal we need below - var success = await multiplexer.ExecuteAsyncImpl(message, ResultProcessor.TrackSubscriptions, asyncState, selected); - if (!success) - { - taskSource.SetResult(false); - } - return await taskSource.Task; - } - catch - { - // If there was an exception, clear the owner - Interlocked.CompareExchange(ref CurrentServer, null, selected); - throw; - } - } - - public async Task UnsubscribeFromServerAsync(RedisChannel channel, object asyncState, bool internalCall) - { - var command = channel.IsPatternBased ? RedisCommand.PUNSUBSCRIBE : RedisCommand.UNSUBSCRIBE; - var oldOwner = Interlocked.Exchange(ref CurrentServer, null); - if (oldOwner != null) - { - var source = TaskResultBox.Create(out var taskSource, asyncState); - var message = GetMessage(channel, command, internalCall); - message.SetSource(ResultProcessor.TrackSubscriptions, source); - - var success = await oldOwner.Multiplexer.ExecuteAsyncImpl(message, ResultProcessor.TrackSubscriptions, asyncState, oldOwner); - if (!success) - { - taskSource.SetCanceled(); - } - return await taskSource.Task; - } - return false; - } - internal ServerEndPoint GetCurrentServer() => Volatile.Read(ref CurrentServer); - internal void Resubscribe(in RedisChannel channel, ServerEndPoint server) - { - // Only re-subscribe to the original server - if (server != null && GetCurrentServer() == server) - { - var cmd = channel.IsPatternBased ? RedisCommand.PSUBSCRIBE : RedisCommand.SUBSCRIBE; - var msg = Message.Create(-1, CommandFlags.FireAndForget, cmd, channel); - msg.SetInternalCall(); - server.Multiplexer.ExecuteSyncImpl(msg, ResultProcessor.TrackSubscriptions, server); - } - } - - internal async ValueTask EnsureSubscribedAsync(ConnectionMultiplexer multiplexer, RedisChannel channel) - { - bool changed = false; - var oldOwner = Volatile.Read(ref CurrentServer); - // If the old server is bad, unsubscribe - if (oldOwner != null && !oldOwner.IsSelectable(RedisCommand.PSUBSCRIBE)) - { - changed = await UnsubscribeFromServerAsync(channel, null, true); - oldOwner = null; - } - // If we didn't have an owner or just cleared one, subscribe - if (oldOwner == null) - { - changed = await SubscribeToServerAsync(multiplexer, channel, CommandFlags.FireAndForget, null, true); - } - return changed; - } - internal void GetSubscriberCounts(out int handlers, out int queues) { queues = ChannelMessageQueue.Count(ref _queues); @@ -366,7 +226,11 @@ public Task IdentifyEndpointAsync(RedisChannel channel, CommandFlags f return ExecuteAsync(msg, ResultProcessor.ConnectionIdentity); } - public bool IsConnected(RedisChannel channel = default(RedisChannel)) => multiplexer.SubscriberConnected(channel); + public bool IsConnected(RedisChannel channel = default(RedisChannel)) + { + var server = multiplexer.GetSubscribedServer(channel) ?? multiplexer.SelectServer(RedisCommand.SUBSCRIBE, CommandFlags.DemandMaster, channel); + return server?.IsConnected == true && server.IsSubscriberConnected; + } public override TimeSpan Ping(CommandFlags flags = CommandFlags.None) { @@ -405,22 +269,24 @@ private Message CreatePingMessage(CommandFlags flags) return msg; } - public long Publish(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None) + private void ThrowIfNull(in RedisChannel channel) { if (channel.IsNullOrEmpty) { throw new ArgumentNullException(nameof(channel)); } + } + + public long Publish(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None) + { + ThrowIfNull(channel); var msg = Message.Create(-1, flags, RedisCommand.PUBLISH, channel, message); return ExecuteSync(msg, ResultProcessor.Int64); } public Task PublishAsync(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None) { - if (channel.IsNullOrEmpty) - { - throw new ArgumentNullException(nameof(channel)); - } + ThrowIfNull(channel); var msg = Message.Create(-1, flags, RedisCommand.PUBLISH, channel, message); return ExecuteAsync(msg, ResultProcessor.Int64); } @@ -428,15 +294,6 @@ public Task PublishAsync(RedisChannel channel, RedisValue message, Command void ISubscriber.Subscribe(RedisChannel channel, Action handler, CommandFlags flags) => Subscribe(channel, handler, null, flags); - public void Subscribe(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) - { - if (channel.IsNullOrEmpty) - { - throw new ArgumentNullException(nameof(channel)); - } - multiplexer.AddSubscription(channel, handler, queue, flags); - } - public ChannelMessageQueue Subscribe(RedisChannel channel, CommandFlags flags = CommandFlags.None) { var queue = new ChannelMessageQueue(channel, this); @@ -444,20 +301,37 @@ public ChannelMessageQueue Subscribe(RedisChannel channel, CommandFlags flags = return queue; } - Task ISubscriber.SubscribeAsync(RedisChannel channel, Action handler, CommandFlags flags) - => SubscribeAsync(channel, handler, null, flags); + public bool Subscribe(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) + { + ThrowIfNull(channel); + if (handler == null && queue == null) { return true; } - public Task SubscribeAsync(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) + var sub = multiplexer.GetOrAddSubscription(channel, flags); + sub.Add(handler, queue); + return EnsureSubscribedToServer(sub, channel, flags, false); + } + + internal bool EnsureSubscribedToServer(Subscription sub, RedisChannel channel, CommandFlags flags, bool internalCall) { - if (channel.IsNullOrEmpty) + if (sub.IsConnected) { return true; } + + // TODO: Cleanup old hangers here? + + try { - throw new ArgumentNullException(nameof(channel)); + var message = sub.GetMessage(channel, SubscriptionAction.Subscribe, flags, internalCall); + var selected = multiplexer.SelectServer(message); + return multiplexer.ExecuteSyncImpl(message, sub.Processor, selected); + } + catch + { + sub.SetServer(null); // If there was an exception, clear the owner + throw; } - return multiplexer.AddSubscriptionAsync(channel, handler, queue, flags, asyncState); } - internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out int queues) - => multiplexer.GetSubscriberCounts(channel, out handlers, out queues); + Task ISubscriber.SubscribeAsync(RedisChannel channel, Action handler, CommandFlags flags) + => SubscribeAsync(channel, handler, null, flags); public async Task SubscribeAsync(RedisChannel channel, CommandFlags flags = CommandFlags.None) { @@ -466,35 +340,137 @@ public async Task SubscribeAsync(RedisChannel channel, Comm return queue; } + public Task SubscribeAsync(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) + { + ThrowIfNull(channel); + if (handler == null && queue == null) { return CompletedTask.Default(null); } + + var sub = multiplexer.GetOrAddSubscription(channel, flags); + sub.Add(handler, queue); + return EnsureSubscribedToServerAsync(sub, channel, flags, false); + } + + public async Task EnsureSubscribedToServerAsync(Subscription sub, RedisChannel channel, CommandFlags flags, bool internalCall) + { + if (sub.IsConnected) { return false; } + + // TODO: Cleanup old hangers here? + + try + { + var message = sub.GetMessage(channel, SubscriptionAction.Subscribe, flags, internalCall); + var selected = multiplexer.SelectServer(message); + return await ExecuteAsync(message, sub.Processor, selected); + } + catch + { + // If there was an exception, clear the owner + sub.SetServer(null); + throw; + } + } + public EndPoint SubscribedEndpoint(RedisChannel channel) => multiplexer.GetSubscribedServer(channel)?.EndPoint; void ISubscriber.Unsubscribe(RedisChannel channel, Action handler, CommandFlags flags) => Unsubscribe(channel, handler, null, flags); - public void Unsubscribe(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) + public bool Unsubscribe(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) { - var task = UnsubscribeAsync(channel, handler, queue, flags); - if ((flags & CommandFlags.FireAndForget) == 0) Wait(task); + ThrowIfNull(channel); + return UnregisterSubscription(channel, handler, queue, out var sub) + ? UnsubscribeFromServer(sub, channel, flags, false) + : true; } - public void UnsubscribeAll(CommandFlags flags = CommandFlags.None) + private bool UnsubscribeFromServer(Subscription sub, RedisChannel channel, CommandFlags flags, bool internalCall) { - var task = UnsubscribeAllAsync(flags); - if ((flags & CommandFlags.FireAndForget) == 0) Wait(task); + if (sub.GetCurrentServer() is ServerEndPoint oldOwner) + { + var message = sub.GetMessage(channel, SubscriptionAction.Unsubscribe, flags, internalCall); + return multiplexer.ExecuteSyncImpl(message, sub.Processor, oldOwner); + } + return false; } - public Task UnsubscribeAllAsync(CommandFlags flags = CommandFlags.None) => multiplexer.RemoveAllSubscriptionsAsync(flags, asyncState); - Task ISubscriber.UnsubscribeAsync(RedisChannel channel, Action handler, CommandFlags flags) => UnsubscribeAsync(channel, handler, null, flags); - public Task UnsubscribeAsync(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) + public Task UnsubscribeAsync(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) { - if (channel.IsNullOrEmpty) + ThrowIfNull(channel); + return UnregisterSubscription(channel, handler, queue, out var sub) + ? UnsubscribeFromServerAsync(sub, channel, flags, asyncState, false) + : CompletedTask.Default(asyncState); + } + + private Task UnsubscribeFromServerAsync(Subscription sub, RedisChannel channel, CommandFlags flags, object asyncState, bool internalCall) + { + if (sub.GetCurrentServer() is ServerEndPoint oldOwner) { - throw new ArgumentNullException(nameof(channel)); + var message = sub.GetMessage(channel, SubscriptionAction.Unsubscribe, flags, internalCall); + return multiplexer.ExecuteAsyncImpl(message, sub.Processor, asyncState, oldOwner); + } + return CompletedTask.FromResult(true, asyncState); + } + + /// + /// Unregisters a handler or queue and returns if we should remove it from the server. + /// + /// True if we should remove the subscription from the server, false otherwise. + private bool UnregisterSubscription(in RedisChannel channel, Action handler, ChannelMessageQueue queue, out Subscription sub) + { + ThrowIfNull(channel); + if (multiplexer.TryGetSubscription(channel, out sub)) + { + bool shouldRemoveSubscriptionFromServer = false; + if (handler == null & queue == null) // blanket wipe + { + sub.MarkCompleted(); + shouldRemoveSubscriptionFromServer = true; + } + else + { + shouldRemoveSubscriptionFromServer = sub.Remove(handler, queue); + } + // If it was the last handler or a blanket wipe, remove it. + if (shouldRemoveSubscriptionFromServer) + { + multiplexer.TryRemoveSubscription(channel, out _); + return true; + } + } + return false; + } + + public void UnsubscribeAll(CommandFlags flags = CommandFlags.None) + { + // TODO: Unsubscribe multi key command to reduce round trips + var subs = multiplexer.GetSubscriptions(); + foreach (var pair in subs) + { + if (subs.TryRemove(pair.Key, out var sub)) + { + sub.MarkCompleted(); + UnsubscribeFromServer(sub, pair.Key, flags, false); + } + } + } + + public Task UnsubscribeAllAsync(CommandFlags flags = CommandFlags.None) + { + // TODO: Unsubscribe multi key command to reduce round trips + Task last = null; + var subs = multiplexer.GetSubscriptions(); + foreach (var pair in subs) + { + if (subs.TryRemove(pair.Key, out var sub)) + { + sub.MarkCompleted(); + last = UnsubscribeFromServerAsync(sub, pair.Key, flags, asyncState, false); + } } - return multiplexer.RemoveSubscriptionAsync(channel, handler, queue, flags, asyncState); + return last ?? CompletedTask.Default(asyncState); } } } diff --git a/src/StackExchange.Redis/ResultProcessor.cs b/src/StackExchange.Redis/ResultProcessor.cs index 0fc523316..ea56a02a4 100644 --- a/src/StackExchange.Redis/ResultProcessor.cs +++ b/src/StackExchange.Redis/ResultProcessor.cs @@ -18,7 +18,7 @@ public static readonly ResultProcessor DemandPONG = new ExpectBasicStringProcessor(CommonReplies.PONG), DemandZeroOrOne = new DemandZeroOrOneProcessor(), AutoConfigure = new AutoConfigureProcessor(), - TrackSubscriptions = new TrackSubscriptionsProcessor(), + TrackSubscriptions = new TrackSubscriptionsProcessor(null), Tracer = new TracerProcessor(false), EstablishConnection = new TracerProcessor(true), BackgroundSaveStarted = new ExpectBasicStringProcessor(CommonReplies.backgroundSavingStarted_trimmed, startsWith: true); @@ -392,6 +392,9 @@ protected override void WriteImpl(PhysicalConnection physical) public sealed class TrackSubscriptionsProcessor : ResultProcessor { + private ConnectionMultiplexer.Subscription Subscription { get; } + public TrackSubscriptionsProcessor(ConnectionMultiplexer.Subscription sub) => Subscription = sub; + protected override bool SetResultCore(PhysicalConnection connection, Message message, in RawResult result) { if (result.Type == ResultType.MultiBulk) @@ -400,6 +403,14 @@ protected override bool SetResultCore(PhysicalConnection connection, Message mes if (items.Length >= 3 && items[2].TryGetInt64(out long count)) { connection.SubscriptionCount = count; + SetResult(message, true); + + var newServer = message.Command switch + { + RedisCommand.SUBSCRIBE or RedisCommand.PSUBSCRIBE => connection.BridgeCouldBeNull?.ServerEndPoint, + _ => null + }; + Subscription?.SetServer(newServer); return true; } } diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs index ab2889e65..d19f2d4eb 100755 --- a/src/StackExchange.Redis/ServerEndPoint.cs +++ b/src/StackExchange.Redis/ServerEndPoint.cs @@ -615,7 +615,7 @@ internal void OnFullyEstablished(PhysicalConnection connection, string source) { if (bridge == subscription) { - Multiplexer.ResendSubscriptions(this); + Multiplexer.EnsureSubscriptions(); } else if (bridge == interactive) { diff --git a/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs b/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs index aebf76648..5812dfccc 100644 --- a/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs +++ b/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs @@ -15,7 +15,7 @@ public Issue1101(ITestOutputHelper output) : base(output) { } private static void AssertCounts(ISubscriber pubsub, in RedisChannel channel, bool has, int handlers, int queues) { - var aHas = ((RedisSubscriber)pubsub).GetSubscriberCounts(channel, out var ah, out var aq); + var aHas = (pubsub.Multiplexer as ConnectionMultiplexer).GetSubscriberCounts(channel, out var ah, out var aq); Assert.Equal(has, aHas); Assert.Equal(handlers, ah); Assert.Equal(queues, aq); From 029c2a35591f1ce13f3673246530b09de8807343 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 14:19:52 -0500 Subject: [PATCH 25/56] Pre-clear rather than await --- src/StackExchange.Redis/RedisSubscriber.cs | 35 +++++++--------------- tests/StackExchange.Redis.Tests/PubSub.cs | 2 -- 2 files changed, 10 insertions(+), 27 deletions(-) diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 5b75ca9b1..6885b28f9 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -317,17 +317,10 @@ internal bool EnsureSubscribedToServer(Subscription sub, RedisChannel channel, C // TODO: Cleanup old hangers here? - try - { - var message = sub.GetMessage(channel, SubscriptionAction.Subscribe, flags, internalCall); - var selected = multiplexer.SelectServer(message); - return multiplexer.ExecuteSyncImpl(message, sub.Processor, selected); - } - catch - { - sub.SetServer(null); // If there was an exception, clear the owner - throw; - } + sub.SetServer(null); // we're not appropriately connected, so blank it out for eligible reconnection + var message = sub.GetMessage(channel, SubscriptionAction.Subscribe, flags, internalCall); + var selected = multiplexer.SelectServer(message); + return multiplexer.ExecuteSyncImpl(message, sub.Processor, selected); } Task ISubscriber.SubscribeAsync(RedisChannel channel, Action handler, CommandFlags flags) @@ -350,24 +343,16 @@ public Task SubscribeAsync(RedisChannel channel, Action EnsureSubscribedToServerAsync(Subscription sub, RedisChannel channel, CommandFlags flags, bool internalCall) + public Task EnsureSubscribedToServerAsync(Subscription sub, RedisChannel channel, CommandFlags flags, bool internalCall) { - if (sub.IsConnected) { return false; } + if (sub.IsConnected) { return CompletedTask.Default(null); } // TODO: Cleanup old hangers here? - try - { - var message = sub.GetMessage(channel, SubscriptionAction.Subscribe, flags, internalCall); - var selected = multiplexer.SelectServer(message); - return await ExecuteAsync(message, sub.Processor, selected); - } - catch - { - // If there was an exception, clear the owner - sub.SetServer(null); - throw; - } + sub.SetServer(null); // we're not appropriately connected, so blank it out for eligible reconnection + var message = sub.GetMessage(channel, SubscriptionAction.Subscribe, flags, internalCall); + var selected = multiplexer.SelectServer(message); + return ExecuteAsync(message, sub.Processor, selected); } public EndPoint SubscribedEndpoint(RedisChannel channel) => multiplexer.GetSubscribedServer(channel)?.EndPoint; diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 0ba6a928c..10d4d8760 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -762,8 +762,6 @@ await sub.SubscribeAsync(channel, delegate }).ConfigureAwait(false); var profile1 = Log(profiler); - // We shouldn't see the initial connection here - Assert.Equal(0, profile1.Count(p => p.Command == nameof(RedisCommand.SUBSCRIBE))); Assert.Equal(1, muxer.GetSubscriptionsCount()); From 341f53299b88e6b9e43bf28e67ab27890eb25687 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 15:04:07 -0500 Subject: [PATCH 26/56] Fix more things --- src/StackExchange.Redis/ResultProcessor.cs | 1 + src/StackExchange.Redis/ServerEndPoint.cs | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/StackExchange.Redis/ResultProcessor.cs b/src/StackExchange.Redis/ResultProcessor.cs index ea56a02a4..79fb5a7a6 100644 --- a/src/StackExchange.Redis/ResultProcessor.cs +++ b/src/StackExchange.Redis/ResultProcessor.cs @@ -414,6 +414,7 @@ protected override bool SetResultCore(PhysicalConnection connection, Message mes return true; } } + SetResult(message, false); return false; } } diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs index d19f2d4eb..902368800 100755 --- a/src/StackExchange.Redis/ServerEndPoint.cs +++ b/src/StackExchange.Redis/ServerEndPoint.cs @@ -615,7 +615,10 @@ internal void OnFullyEstablished(PhysicalConnection connection, string source) { if (bridge == subscription) { - Multiplexer.EnsureSubscriptions(); + // Note: this MUST be fire and forget, because we might be in the middle of a Sync processing + // TracerProcessor which is executing this line inside a SetResultCore(). + // Since we're issuing commands inside a SetResult path in a message, we'd create a deadlock by waiting. + Multiplexer.EnsureSubscriptions(CommandFlags.FireAndForget); } else if (bridge == interactive) { From 5dbf57558dc7a0779132f42337a3fec4912e0314 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 15:09:43 -0500 Subject: [PATCH 27/56] Add logging to TestPublishWithSubscribers --- tests/StackExchange.Redis.Tests/PubSub.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 10d4d8760..e2ea9b157 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -568,8 +568,8 @@ await Assert.ThrowsAsync(async delegate public async Task TestPublishWithSubscribers() { var channel = Me(); - using (var muxerA = Create(shared: false)) - using (var muxerB = Create(shared: false)) + using (var muxerA = Create(shared: false, log: Writer)) + using (var muxerB = Create(shared: false, log: Writer)) using (var conn = Create()) { var listenA = muxerA.GetSubscriber(); From 51c927b87ce93a1118e81f0cd91adac20eb902e7 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 19:06:54 -0500 Subject: [PATCH 28/56] Light up more pubsub tests --- tests/StackExchange.Redis.Tests/PubSub.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index e2ea9b157..0f7ff0b78 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -256,7 +256,7 @@ public void TestPublishWithNoSubscribers() } } - [FactLongRunning] + [Fact] public void TestMassivePublishWithWithoutFlush_Local() { using (var muxer = Create()) @@ -304,7 +304,7 @@ private void TestMassivePublish(ISubscriber conn, string channel, string caption Assert.True(withFAF.ElapsedMilliseconds < withAsync.ElapsedMilliseconds + 3000, caption); } - [FactLongRunning] + [Fact] public async Task PubSubGetAllAnyOrder() { using (var muxer = Create(syncTimeout: 20000)) From 20d2d281c493ffc5f3824570fc269081f5979f48 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 19:46:27 -0500 Subject: [PATCH 29/56] Several subtle but big changes 1. Wait for *both* interactive and subscription connections to complete when connecting 2. Instantly mark subscriptions as having no server on every disconnect 3. Adds some more test/diagnostic logging for the tests around this --- src/StackExchange.Redis/RedisSubscriber.cs | 21 ++++++++++++++++--- src/StackExchange.Redis/ServerEndPoint.cs | 7 ++++++- .../ServerSelectionStrategy.cs | 2 +- tests/StackExchange.Redis.Tests/PubSub.cs | 17 ++++++++++----- 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 6885b28f9..152a0a492 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -85,6 +85,14 @@ internal void EnsureSubscriptions(CommandFlags flags = CommandFlags.None) } } + internal void UpdateSubscriptions() + { + foreach (var pair in subscriptions) + { + pair.Value.UpdateServer(); + } + } + internal async Task EnsureSubscriptionsAsync(CommandFlags flags = CommandFlags.None) { long count = 0; @@ -182,9 +190,6 @@ public bool Remove(Action handler, ChannelMessageQueue } return _handlers == null & _queues == null; } - - internal ServerEndPoint GetCurrentServer() => Volatile.Read(ref CurrentServer); - internal void GetSubscriberCounts(out int handlers, out int queues) { queues = ChannelMessageQueue.Count(ref _queues); @@ -203,6 +208,16 @@ internal void GetSubscriberCounts(out int handlers, out int queues) foreach (var sub in tmp.AsEnumerable()) { handlers++; } } } + + internal ServerEndPoint GetCurrentServer() => Volatile.Read(ref CurrentServer); + + internal void UpdateServer() + { + if (!IsConnected) + { + SetServer(null); + } + } } } diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs index 902368800..18f68f36b 100755 --- a/src/StackExchange.Redis/ServerEndPoint.cs +++ b/src/StackExchange.Redis/ServerEndPoint.cs @@ -575,6 +575,10 @@ internal void OnDisconnected(PhysicalBridge bridge) { CompletePendingConnectionMonitors("Disconnected"); } + else if (bridge == subscription) + { + Multiplexer.UpdateSubscriptions(); + } } internal Task OnEstablishingAsync(PhysicalConnection connection, LogProxy log) @@ -620,8 +624,9 @@ internal void OnFullyEstablished(PhysicalConnection connection, string source) // Since we're issuing commands inside a SetResult path in a message, we'd create a deadlock by waiting. Multiplexer.EnsureSubscriptions(CommandFlags.FireAndForget); } - else if (bridge == interactive) + if (IsConnected && IsSubscriberConnected) { + // Only connect on the second leg - we can accomplish this by checking both CompletePendingConnectionMonitors(source); } diff --git a/src/StackExchange.Redis/ServerSelectionStrategy.cs b/src/StackExchange.Redis/ServerSelectionStrategy.cs index cb4bb954c..7578936f2 100644 --- a/src/StackExchange.Redis/ServerSelectionStrategy.cs +++ b/src/StackExchange.Redis/ServerSelectionStrategy.cs @@ -288,7 +288,7 @@ private ServerEndPoint[] MapForMutation() private ServerEndPoint Select(int slot, RedisCommand command, CommandFlags flags) { - flags = Message.GetMasterReplicaFlags(flags); // only intersted in master/replica preferences + flags = Message.GetMasterReplicaFlags(flags); // only interested in master/replica preferences ServerEndPoint[] arr; if (slot == NoSlot || (arr = map) == null) return Any(command, flags); diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 0f7ff0b78..817c130a1 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -801,16 +801,23 @@ await sub.SubscribeAsync(channel, delegate // Ensure we're reconnected Assert.True(sub.IsConnected(channel)); - // And time to resubscribe... - await Task.Delay(1000).ConfigureAwait(false); - // Ensure we've sent the subscribe command after reconnecting var profile2 = Log(profiler); //Assert.Equal(1, profile2.Count(p => p.Command == nameof(RedisCommand.SUBSCRIBE))); - Log($"Issuing ping after reconnected"); + Log("Issuing ping after reconnected"); sub.Ping(); - Assert.Equal(1, muxer.GetSubscriptionsCount()); + + var muxerSubCount = muxer.GetSubscriptionsCount(); + Log($"Muxer thinks we have {muxerSubCount} subscriber(s)."); + Assert.Equal(1, muxerSubCount); + + var muxerSubs = muxer.GetSubscriptions(); + foreach (var pair in muxerSubs) + { + var muxerSub = pair.Value; + Log($" Muxer Sub: {pair.Key}: (EndPoint: {muxerSub.GetCurrentServer()}, Connected: {muxerSub.IsConnected})"); + } Log("Publishing"); var published = await sub.PublishAsync(channel, "abc").ConfigureAwait(false); From 814b4016e453e329574b243a3ed01c0690d1993d Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 20:06:32 -0500 Subject: [PATCH 30/56] More PubSub logging Give the sub *receipt* just a moment to execute under load as well - we can be properly subscribe with the handler not instantly executing on the receiving side. --- tests/StackExchange.Redis.Tests/PubSub.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 817c130a1..d6a436fef 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -60,7 +60,7 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br var pub = GetAnyMaster(muxer); var sub = muxer.GetSubscriber(); await PingAsync(muxer, pub, sub).ForAwait(); - HashSet received = new HashSet(); + HashSet received = new(); int secondHandler = 0; string subChannel = (wildCard ? "a*c" : "abc") + breaker; string pubChannel = "abc" + breaker; @@ -106,7 +106,12 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br { Assert.Single(received); } - Assert.Equal(2, Thread.VolatileRead(ref secondHandler)); + + await UntilCondition(TimeSpan.FromSeconds(2), () => Thread.VolatileRead(ref secondHandler) == 2); + + var secondHandlerCount = Thread.VolatileRead(ref secondHandler); + Log("Expecting 2 from second handler, got: " + secondHandlerCount); + Assert.Equal(2, secondHandlerCount); Assert.Equal(1, count); // unsubscribe from second; should see nothing this time @@ -117,7 +122,9 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br { Assert.Single(received); } - Assert.Equal(2, Thread.VolatileRead(ref secondHandler)); + secondHandlerCount = Thread.VolatileRead(ref secondHandler); + Log("Expecting 2 from second handler, got: " + secondHandlerCount); + Assert.Equal(2, secondHandlerCount); Assert.Equal(0, count); } } From 667aa34c5321074ea40968736dc01c1d95755aeb Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 20:17:44 -0500 Subject: [PATCH 31/56] TestPatternPubSub: give reception a moment Again, async handler may take a second to get there under load. --- tests/StackExchange.Redis.Tests/PubSub.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index d6a436fef..1831ec2e6 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -239,6 +239,9 @@ public async Task TestPatternPubSub() { Assert.Single(received); } + + // Give reception a bit, the handler could be delayed under load + await UntilCondition(TimeSpan.FromSeconds(2), () => Thread.VolatileRead(ref secondHandler) == 1); Assert.Equal(1, Thread.VolatileRead(ref secondHandler)); sub.Unsubscribe("a*c"); From cf96bbaa2c7d5e6a58fca304cb5fafc6c6bcb3eb Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 20:40:50 -0500 Subject: [PATCH 32/56] Moar! --- tests/StackExchange.Redis.Tests/PubSub.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 1831ec2e6..157585c09 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -96,6 +96,8 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br { Assert.Single(received); } + // Give handler firing a moment + await UntilCondition(TimeSpan.FromSeconds(2), () => Thread.VolatileRead(ref secondHandler) == 1); Assert.Equal(1, Thread.VolatileRead(ref secondHandler)); // unsubscribe from first; should still see second From a6be64a5714447cbe3c71e61e87be0d7ae99aba5 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 20:51:40 -0500 Subject: [PATCH 33/56] Add logging to Issue1101 pipe --- tests/StackExchange.Redis.Tests/Issues/Issue1101.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs b/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs index 5812dfccc..bb471b744 100644 --- a/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs +++ b/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs @@ -23,7 +23,7 @@ private static void AssertCounts(ISubscriber pubsub, in RedisChannel channel, [Fact] public async Task ExecuteWithUnsubscribeViaChannel() { - using (var muxer = Create()) + using (var muxer = Create(log: Writer)) { RedisChannel name = Me(); var pubsub = muxer.GetSubscriber(); @@ -89,7 +89,7 @@ public async Task ExecuteWithUnsubscribeViaChannel() [Fact] public async Task ExecuteWithUnsubscribeViaSubscriber() { - using (var muxer = Create()) + using (var muxer = Create(log: Writer)) { RedisChannel name = Me(); var pubsub = muxer.GetSubscriber(); @@ -141,7 +141,7 @@ public async Task ExecuteWithUnsubscribeViaSubscriber() [Fact] public async Task ExecuteWithUnsubscribeViaClearAll() { - using (var muxer = Create()) + using (var muxer = Create(log: Writer)) { RedisChannel name = Me(); var pubsub = muxer.GetSubscriber(); From 31b38dcd8cb0efbb05f182597958ac2a51302f7a Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 21:03:41 -0500 Subject: [PATCH 34/56] PubSubGetAllAnyOrder: don't share conn --- tests/StackExchange.Redis.Tests/PubSub.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 157585c09..43e0fcecb 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -319,7 +319,7 @@ private void TestMassivePublish(ISubscriber conn, string channel, string caption [Fact] public async Task PubSubGetAllAnyOrder() { - using (var muxer = Create(syncTimeout: 20000)) + using (var muxer = Create(syncTimeout: 20000, shared: false)) { var sub = muxer.GetSubscriber(); RedisChannel channel = Me(); From 79e50901ebe0eef4019063deba0c662e480659b7 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 21:05:54 -0500 Subject: [PATCH 35/56] TextWriterOutputHelper: fix end-of-tests race case This is purely handler logging after we care - abort out. --- .../Helpers/TextWriterOutputHelper.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/Helpers/TextWriterOutputHelper.cs b/tests/StackExchange.Redis.Tests/Helpers/TextWriterOutputHelper.cs index b0c27f6fa..bdcb7b55f 100644 --- a/tests/StackExchange.Redis.Tests/Helpers/TextWriterOutputHelper.cs +++ b/tests/StackExchange.Redis.Tests/Helpers/TextWriterOutputHelper.cs @@ -82,7 +82,14 @@ protected override void Dispose(bool disposing) private void FlushBuffer() { var text = Buffer.ToString(); - Output.WriteLine(text); + try + { + Output.WriteLine(text); + } + catch (InvalidOperationException) + { + // Thrown when writing from a handler after a test has ended - just bail in this case + } Echo?.AppendLine(text); if (ToConsole) { From 8ea3b6d1256bb2ebc322aafb7add5b149e4ce1c6 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 21:47:44 -0500 Subject: [PATCH 36/56] Bump Docker images to 6.2.6 GitHub is skipping > 5.x, lights up all tests properly. --- tests/RedisConfigs/Dockerfile | 2 +- tests/RedisConfigs/docker-compose.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/RedisConfigs/Dockerfile b/tests/RedisConfigs/Dockerfile index 969497b7d..047da2975 100644 --- a/tests/RedisConfigs/Dockerfile +++ b/tests/RedisConfigs/Dockerfile @@ -1,4 +1,4 @@ -FROM redis:5 +FROM redis:6.2.6 COPY Basic /data/Basic/ COPY Failover /data/Failover/ diff --git a/tests/RedisConfigs/docker-compose.yml b/tests/RedisConfigs/docker-compose.yml index 6475e6664..cb3dd099c 100644 --- a/tests/RedisConfigs/docker-compose.yml +++ b/tests/RedisConfigs/docker-compose.yml @@ -1,4 +1,4 @@ -version: '2.5' +version: '2.6' services: redis: From d9caedbd91a37fb807cb61b16382dfd904e8cea0 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 22:12:40 -0500 Subject: [PATCH 37/56] Nick, you idiot. --- tests/StackExchange.Redis.Tests/Locking.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/Locking.cs b/tests/StackExchange.Redis.Tests/Locking.cs index 9053d2956..69a86ac4d 100644 --- a/tests/StackExchange.Redis.Tests/Locking.cs +++ b/tests/StackExchange.Redis.Tests/Locking.cs @@ -99,7 +99,7 @@ private void TestLockOpCountByVersion(IConnectionMultiplexer conn, int expectedO Assert.Equal(!existFirst, taken); Assert.Equal(expectedVal, valAfter); - Assert.True(expectedOps >= countAfter - countBefore, $"{expectedOps} >= ({countAfter} - {countBefore})"); + Assert.True(expectedOps <= countAfter - countBefore, $"{expectedOps} >= ({countAfter} - {countBefore})"); // note we get a ping from GetCounters } From 3fa77a4736018ae56c22f356f30954d762cdfe9c Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 22:32:37 -0500 Subject: [PATCH 38/56] Fix log message too --- tests/StackExchange.Redis.Tests/Locking.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/Locking.cs b/tests/StackExchange.Redis.Tests/Locking.cs index 69a86ac4d..760b1b65f 100644 --- a/tests/StackExchange.Redis.Tests/Locking.cs +++ b/tests/StackExchange.Redis.Tests/Locking.cs @@ -99,8 +99,8 @@ private void TestLockOpCountByVersion(IConnectionMultiplexer conn, int expectedO Assert.Equal(!existFirst, taken); Assert.Equal(expectedVal, valAfter); - Assert.True(expectedOps <= countAfter - countBefore, $"{expectedOps} >= ({countAfter} - {countBefore})"); // note we get a ping from GetCounters + Assert.True(countAfter - countBefore >= expectedOps, $"({countAfter} - {countBefore}) >= {expectedOps}"); } private IConnectionMultiplexer Create(TestMode mode) => mode switch From 04233fe0f82a5ba0018833dcd87a60607c3fef5b Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 22:34:36 -0500 Subject: [PATCH 39/56] Sentinel: Fire and Forget on startup This lets us queue the subscription up on the subscriptions collection before it can succeed, but we won't eat the failure in connect from a race either. --- src/StackExchange.Redis/ConnectionMultiplexer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.cs b/src/StackExchange.Redis/ConnectionMultiplexer.cs index 2254cf50a..2e3fe9541 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.cs @@ -2321,7 +2321,7 @@ internal void InitializeSentinel(LogProxy logProxy) } } } - }); + }, CommandFlags.FireAndForget); } // If we lose connection to a sentinel server, From ff5401253665276e0fbee8e4aae721f02d6c2175 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sat, 22 Jan 2022 22:39:57 -0500 Subject: [PATCH 40/56] ExplicitPublishMode: remove delay --- tests/StackExchange.Redis.Tests/PubSub.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 43e0fcecb..b3d644ded 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -29,7 +29,6 @@ public async Task ExplicitPublishMode() pub.Subscribe(new RedisChannel("ab*d", RedisChannel.PatternMode.Auto), (x, y) => Interlocked.Increment(ref c)); pub.Subscribe("abc*", (x, y) => Interlocked.Increment(ref d)); - await Task.Delay(1000).ForAwait(); pub.Publish("abcd", "efg"); await UntilCondition(TimeSpan.FromSeconds(10), () => Thread.VolatileRead(ref b) == 1 From 84439d4f6120ef7ce1d64a447dcf124288ea0a2b Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sun, 23 Jan 2022 12:55:34 -0500 Subject: [PATCH 41/56] Cleanup and comments! --- src/StackExchange.Redis/RedisSubscriber.cs | 116 +++++++++++++++------ src/StackExchange.Redis/ResultProcessor.cs | 2 +- 2 files changed, 85 insertions(+), 33 deletions(-) diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 152a0a492..ac55030d2 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Concurrent; +using System.Diagnostics.CodeAnalysis; using System.Net; using System.Threading; using System.Threading.Tasks; @@ -39,6 +40,10 @@ internal bool TryRemoveSubscription(in RedisChannel channel, out Subscription su } } + /// + /// Gets the subscriber counts for a channel. + /// + /// True if there's a subscription registered at all. internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out int queues) { if (subscriptions.TryGetValue(channel, out var sub)) @@ -50,6 +55,13 @@ internal bool GetSubscriberCounts(in RedisChannel channel, out int handlers, out return false; } + /// + /// Gets which server, if any, there's a registered subscription to for this channel. + /// + /// + /// This may be null if there is a subscription, but we don't have a connected server at the moment. + /// This behavior is fine but IsConnected checks, but is a subtle difference in . + /// internal ServerEndPoint GetSubscribedServer(in RedisChannel channel) { if (!channel.IsNullOrEmpty && subscriptions.TryGetValue(channel, out Subscription sub)) @@ -59,6 +71,9 @@ internal ServerEndPoint GetSubscribedServer(in RedisChannel channel) return null; } + /// + /// Handler that executes whenever a message comes in, this doles out messages to any registered handlers. + /// internal void OnMessage(in RedisChannel subscription, in RedisChannel channel, in RedisValue payload) { ICompletable completable = null; @@ -77,24 +92,37 @@ internal void OnMessage(in RedisChannel subscription, in RedisChannel channel, i } } - internal void EnsureSubscriptions(CommandFlags flags = CommandFlags.None) + /// + /// Updates all subscriptions re-evaluating their state. + /// This clears the current server if it's not connected, prepping them to reconnect. + /// + internal void UpdateSubscriptions() { foreach (var pair in subscriptions) { - DefaultSubscriber.EnsureSubscribedToServer(pair.Value, pair.Key, flags, true); + pair.Value.UpdateServer(); } } - internal void UpdateSubscriptions() + /// + /// Ensures all subscriptions are connected to a server, if possible. + /// + internal void EnsureSubscriptions(CommandFlags flags = CommandFlags.None) { + // TODO: Subscribe with variadic commands to reduce round trips foreach (var pair in subscriptions) { - pair.Value.UpdateServer(); + DefaultSubscriber.EnsureSubscribedToServer(pair.Value, pair.Key, flags, true); } } + /// + /// Ensures all subscriptions are connected to a server, if possible. + /// internal async Task EnsureSubscriptionsAsync(CommandFlags flags = CommandFlags.None) { + // TODO: Evaluate performance here, this isn't good for a large number of subscriptions. + // It's probable we want to fire and forget `n` here, recording how many are going to try to reconnect? long count = 0; foreach (var pair in subscriptions) { @@ -112,6 +140,11 @@ internal enum SubscriptionAction Unsubscribe } + /// + /// This is the record of a single subscription to a redis server. + /// It's the singular channel (which may or may not be a pattern), to one or more handlers. + /// We subscriber to a redis server once (for all messages) and execute 1-many handlers when a message arrives. + /// internal sealed class Subscription { private Action _handlers; @@ -120,6 +153,10 @@ internal sealed class Subscription public CommandFlags Flags { get; } public ResultProcessor.TrackSubscriptionsProcessor Processor { get; } + /// + /// Whether the we have is connected. + /// Since we clear on a disconnect, this should stay correct. + /// internal bool IsConnected => CurrentServer?.IsSubscriberConnected == true; public Subscription(CommandFlags flags) @@ -128,6 +165,9 @@ public Subscription(CommandFlags flags) Processor = new ResultProcessor.TrackSubscriptionsProcessor(this); } + /// + /// Gets the configured (P)SUBSCRIBE or (P)UNSUBSCRIBE for an action. + /// internal Message GetMessage(RedisChannel channel, SubscriptionAction action, CommandFlags flags, bool internalCall) { var isPattern = channel.IsPatternBased; @@ -151,8 +191,6 @@ internal Message GetMessage(RedisChannel channel, SubscriptionAction action, Com return msg; } - internal void SetServer(ServerEndPoint server) => CurrentServer = server; - public void Add(Action handler, ChannelMessageQueue queue) { if (handler != null) @@ -165,6 +203,19 @@ public void Add(Action handler, ChannelMessageQueue qu } } + public bool Remove(Action handler, ChannelMessageQueue queue) + { + if (handler != null) + { + _handlers -= handler; + } + if (queue != null) + { + ChannelMessageQueue.Remove(ref _queues, queue); + } + return _handlers == null & _queues == null; + } + public ICompletable ForInvoke(in RedisChannel channel, in RedisValue message, out ChannelMessageQueue queues) { var handlers = _handlers; @@ -178,18 +229,6 @@ internal void MarkCompleted() ChannelMessageQueue.MarkAllCompleted(ref _queues); } - public bool Remove(Action handler, ChannelMessageQueue queue) - { - if (handler != null) - { - _handlers -= handler; - } - if (queue != null) - { - ChannelMessageQueue.Remove(ref _queues, queue); - } - return _handlers == null & _queues == null; - } internal void GetSubscriberCounts(out int handlers, out int queues) { queues = ChannelMessageQueue.Count(ref _queues); @@ -210,17 +249,28 @@ internal void GetSubscriberCounts(out int handlers, out int queues) } internal ServerEndPoint GetCurrentServer() => Volatile.Read(ref CurrentServer); + internal void SetCurrentServer(ServerEndPoint server) => CurrentServer = server; + /// + /// Evaluates state and if we're not currently connected, clears the server reference. + /// internal void UpdateServer() { if (!IsConnected) { - SetServer(null); + CurrentServer = null; } } } } + /// + /// A wrapper for subscription actions. + /// + /// + /// By having most functionality here and state on , we can + /// use the baseline execution methods to take the normal message paths. + /// internal sealed class RedisSubscriber : RedisBase, ISubscriber { internal RedisSubscriber(ConnectionMultiplexer multiplexer, object asyncState) : base(multiplexer, asyncState) @@ -241,6 +291,10 @@ public Task IdentifyEndpointAsync(RedisChannel channel, CommandFlags f return ExecuteAsync(msg, ResultProcessor.ConnectionIdentity); } + /// + /// This is *could* we be connected, as in "what's the theoretical endpoint for this channel?", + /// rather than if we're actually connected and actually listening on that channel. + /// public bool IsConnected(RedisChannel channel = default(RedisChannel)) { var server = multiplexer.GetSubscribedServer(channel) ?? multiplexer.SelectServer(RedisCommand.SUBSCRIBE, CommandFlags.DemandMaster, channel); @@ -332,7 +386,7 @@ internal bool EnsureSubscribedToServer(Subscription sub, RedisChannel channel, C // TODO: Cleanup old hangers here? - sub.SetServer(null); // we're not appropriately connected, so blank it out for eligible reconnection + sub.SetCurrentServer(null); // we're not appropriately connected, so blank it out for eligible reconnection var message = sub.GetMessage(channel, SubscriptionAction.Subscribe, flags, internalCall); var selected = multiplexer.SelectServer(message); return multiplexer.ExecuteSyncImpl(message, sub.Processor, selected); @@ -364,7 +418,7 @@ public Task EnsureSubscribedToServerAsync(Subscription sub, RedisChannel c // TODO: Cleanup old hangers here? - sub.SetServer(null); // we're not appropriately connected, so blank it out for eligible reconnection + sub.SetCurrentServer(null); // we're not appropriately connected, so blank it out for eligible reconnection var message = sub.GetMessage(channel, SubscriptionAction.Subscribe, flags, internalCall); var selected = multiplexer.SelectServer(message); return ExecuteAsync(message, sub.Processor, selected); @@ -375,6 +429,7 @@ public Task EnsureSubscribedToServerAsync(Subscription sub, RedisChannel c void ISubscriber.Unsubscribe(RedisChannel channel, Action handler, CommandFlags flags) => Unsubscribe(channel, handler, null, flags); + [SuppressMessage("Style", "IDE0075:Simplify conditional expression", Justification = "The suggestion sucks.")] public bool Unsubscribe(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) { ThrowIfNull(channel); @@ -423,19 +478,16 @@ private bool UnregisterSubscription(in RedisChannel channel, Action connection.BridgeCouldBeNull?.ServerEndPoint, _ => null }; - Subscription?.SetServer(newServer); + Subscription?.SetCurrentServer(newServer); return true; } } From 60d9b8002764dd22ed15b0ff3d9f87782a13f56d Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sun, 23 Jan 2022 13:08:15 -0500 Subject: [PATCH 42/56] More comments! --- src/StackExchange.Redis/RedisSubscriber.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index ac55030d2..28ff75c98 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -433,6 +433,7 @@ void ISubscriber.Unsubscribe(RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) { ThrowIfNull(channel); + // Unregister the subscription handler/queue, and if that returns true (last handler removed), also disconnect from the server return UnregisterSubscription(channel, handler, queue, out var sub) ? UnsubscribeFromServer(sub, channel, flags, false) : true; @@ -454,6 +455,7 @@ Task ISubscriber.UnsubscribeAsync(RedisChannel channel, Action UnsubscribeAsync(in RedisChannel channel, Action handler, ChannelMessageQueue queue, CommandFlags flags) { ThrowIfNull(channel); + // Unregister the subscription handler/queue, and if that returns true (last handler removed), also disconnect from the server return UnregisterSubscription(channel, handler, queue, out var sub) ? UnsubscribeFromServerAsync(sub, channel, flags, asyncState, false) : CompletedTask.Default(asyncState); From c6fb5698116ccd77d719bd999336a927f8cc246c Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sun, 23 Jan 2022 13:34:46 -0500 Subject: [PATCH 43/56] Annnnnd the other Sentinel one --- src/StackExchange.Redis/ConnectionMultiplexer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.cs b/src/StackExchange.Redis/ConnectionMultiplexer.cs index 2e3fe9541..d6f89c35a 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.cs @@ -2340,7 +2340,7 @@ internal void InitializeSentinel(LogProxy logProxy) { string[] messageParts = ((string)message).Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries); UpdateSentinelAddressList(messageParts[0]); - }); + }, CommandFlags.FireAndForget); } } From c0206fac530f395059d9c6f0907b8ad7f09b8332 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Sun, 23 Jan 2022 23:17:10 -0500 Subject: [PATCH 44/56] Start PubSubMultiserver --- .../SimulatedFailureType.cs | 5 ++ tests/StackExchange.Redis.Tests/Cluster.cs | 10 +-- .../Helpers/TestConfig.cs | 2 + .../PubSubMultiserver.cs | 78 +++++++++++++++++++ 4 files changed, 86 insertions(+), 9 deletions(-) create mode 100644 tests/StackExchange.Redis.Tests/PubSubMultiserver.cs diff --git a/src/StackExchange.Redis/SimulatedFailureType.cs b/src/StackExchange.Redis/SimulatedFailureType.cs index 0084746a7..80fca095c 100644 --- a/src/StackExchange.Redis/SimulatedFailureType.cs +++ b/src/StackExchange.Redis/SimulatedFailureType.cs @@ -10,8 +10,13 @@ internal enum SimulatedFailureType InteractiveOutbound = 1 << 1, SubscriptionInbound = 1 << 2, SubscriptionOutbound = 1 << 3, + AllInbound = InteractiveInbound | SubscriptionInbound, AllOutbound = InteractiveOutbound | SubscriptionOutbound, + + AllInteractive = InteractiveInbound | InteractiveOutbound, + AllSubscription = SubscriptionInbound | SubscriptionOutbound, + All = AllInbound | AllOutbound, } } diff --git a/tests/StackExchange.Redis.Tests/Cluster.cs b/tests/StackExchange.Redis.Tests/Cluster.cs index a7af400da..6e6e6e7c3 100644 --- a/tests/StackExchange.Redis.Tests/Cluster.cs +++ b/tests/StackExchange.Redis.Tests/Cluster.cs @@ -3,7 +3,6 @@ using System.IO; using System.Linq; using System.Net; -using System.Text; using System.Threading; using System.Threading.Tasks; using StackExchange.Redis.Profiling; @@ -15,14 +14,7 @@ namespace StackExchange.Redis.Tests public class Cluster : TestBase { public Cluster(ITestOutputHelper output) : base (output) { } - - protected override string GetConfiguration() - { - var server = TestConfig.Current.ClusterServer; - return string.Join(",", - Enumerable.Range(TestConfig.Current.ClusterStartPort, TestConfig.Current.ClusterServerCount).Select(port => server + ":" + port) - ) + ",connectTimeout=10000"; - } + protected override string GetConfiguration() => TestConfig.Current.ClusterServersAndPorts + ",connectTimeout=10000"; [Fact] public void ExportConfiguration() diff --git a/tests/StackExchange.Redis.Tests/Helpers/TestConfig.cs b/tests/StackExchange.Redis.Tests/Helpers/TestConfig.cs index 07cbc6e58..a11afa18c 100644 --- a/tests/StackExchange.Redis.Tests/Helpers/TestConfig.cs +++ b/tests/StackExchange.Redis.Tests/Helpers/TestConfig.cs @@ -2,6 +2,7 @@ using System; using Newtonsoft.Json; using System.Threading; +using System.Linq; namespace StackExchange.Redis.Tests { @@ -87,6 +88,7 @@ public class Config public string ClusterServer { get; set; } = "127.0.0.1"; public int ClusterStartPort { get; set; } = 7000; public int ClusterServerCount { get; set; } = 6; + public string ClusterServersAndPorts => string.Join(",", Enumerable.Range(ClusterStartPort, ClusterServerCount).Select(port => ClusterServer + ":" + port)); public string SslServer { get; set; } public int SslPort { get; set; } diff --git a/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs b/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs new file mode 100644 index 000000000..2b1a5304c --- /dev/null +++ b/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs @@ -0,0 +1,78 @@ +using System; +using System.Threading.Tasks; +using Xunit; +using Xunit.Abstractions; + +namespace StackExchange.Redis.Tests +{ + [Collection(SharedConnectionFixture.Key)] + public class PubSubMultiserver : TestBase + { + public PubSubMultiserver(ITestOutputHelper output, SharedConnectionFixture fixture) : base(output, fixture) { } + protected override string GetConfiguration() => TestConfig.Current.ClusterServersAndPorts + ",connectTimeout=10000"; + + [Fact] + public void ChannelSharding() + { + using var muxer = Create(channelPrefix: Me()) as ConnectionMultiplexer; + + var defaultSlot = muxer.ServerSelectionStrategy.HashSlot(default(RedisChannel)); + var slot1 = muxer.ServerSelectionStrategy.HashSlot((RedisChannel)"hey"); + var slot2 = muxer.ServerSelectionStrategy.HashSlot((RedisChannel)"hey2"); + + Assert.NotEqual(defaultSlot, slot1); + Assert.NotEqual(ServerSelectionStrategy.NoSlot, slot1); + Assert.NotEqual(slot1, slot2); + } + + [Fact] + public async Task SubscriptionNodeReconnecting() + { + Log("Connecting..."); + using var muxer = Create(allowAdmin: true) as ConnectionMultiplexer; + var sub = muxer.GetSubscriber(); + var channel = (RedisChannel)Me(); + + Log("Subscribing..."); + await sub.SubscribeAsync(channel, (channel, val) => Log("Message: " + val)); + + Assert.True(sub.IsConnected(channel)); + + var endpoint = sub.SubscribedEndpoint(channel); + var subscribedServer = muxer.GetServer(endpoint); + var subscribedServerEndpoint = muxer.GetServerEndPoint(endpoint); + + Assert.True(subscribedServer.IsConnected, "subscribedServer.IsConnected"); + Assert.True(subscribedServerEndpoint.IsConnected, "subscribedServerEndpoint.IsConnected"); + Assert.True(subscribedServerEndpoint.IsSubscriberConnected, "subscribedServerEndpoint.IsSubscriberConnected"); + + Assert.True(muxer.TryGetSubscription(channel, out var subscription)); + var initialServer = subscription.GetCurrentServer(); + Assert.NotNull(initialServer); + Assert.True(initialServer.IsConnected); + Log($"Connected to: " + initialServer); + + muxer.AllowConnect = false; + subscribedServerEndpoint.SimulateConnectionFailure(SimulatedFailureType.AllSubscription); + + Assert.True(subscribedServerEndpoint.IsConnected, "subscribedServerEndpoint.IsConnected"); + Assert.False(subscribedServerEndpoint.IsSubscriberConnected, "subscribedServerEndpoint.IsSubscriberConnected"); + + await UntilCondition(TimeSpan.FromSeconds(5), () => subscription.IsConnected); + Assert.True(subscription.IsConnected); + + var newServer = subscription.GetCurrentServer(); + Assert.NotNull(newServer); + Assert.NotEqual(newServer, initialServer); + Log($"Now connected to: " + initialServer); + } + + // 04:14:23.7955: Connection failed(InternalFailure): 127.0.0.1:7002/Subscription: StackExchange.Redis.RedisConnectionException: InternalFailure on 127.0.0.1:7002/Subscription, Initializing/NotStarted, last: SUBSCRIBE, origin: ConnectedAsync, outstanding: 0, last-read: 0s ago, last-write: 0s ago, keep-alive: 60s, state: Connecting, mgr: 9 of 10 available, last-heartbeat: never, last-mbeat: 0s ago, global: 23s ago, v: 2.5.49.64454 ---> StackExchange.Redis.RedisConnectionException: debugging + //at StackExchange.Redis.PhysicalConnection.OnDebugAbort() in C:\git\StackExchange\StackExchange.Redis\src\StackExchange.Redis\PhysicalConnection.cs:line 1560 + // at StackExchange.Redis.PhysicalConnection.d__104.MoveNext() in C:\git\StackExchange\StackExchange.Redis\src\StackExchange.Redis\PhysicalConnection.cs:line 1389 + // --- End of inner exception stack trace --- + + // TODO: Primary/Replica failover + // TODO: Subscribe failover, but with CommandFlags + } +} From b484e5f094a2a70239181221adb3ae92955507c1 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 24 Jan 2022 08:34:13 -0500 Subject: [PATCH 45/56] Tests, yay! --- tests/StackExchange.Redis.Tests/PubSub.cs | 2 +- .../PubSubMultiserver.cs | 36 ++++++++++++++----- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index b3d644ded..f84efe8e9 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -54,7 +54,7 @@ await UntilCondition(TimeSpan.FromSeconds(10), [InlineData("Foo:", true, "f")] public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string breaker) { - using (var muxer = Create(channelPrefix: channelPrefix, log: Writer)) + using (var muxer = Create(channelPrefix: channelPrefix, shared: false, log: Writer)) { var pub = GetAnyMaster(muxer); var sub = muxer.GetSubscriber(); diff --git a/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs b/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs index 2b1a5304c..85ce6b2a4 100644 --- a/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs +++ b/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs @@ -1,4 +1,5 @@ using System; +using System.Threading; using System.Threading.Tasks; using Xunit; using Xunit.Abstractions; @@ -33,11 +34,24 @@ public async Task SubscriptionNodeReconnecting() var sub = muxer.GetSubscriber(); var channel = (RedisChannel)Me(); + var count = 0; Log("Subscribing..."); - await sub.SubscribeAsync(channel, (channel, val) => Log("Message: " + val)); - + await sub.SubscribeAsync(channel, (channel, val) => + { + Interlocked.Increment(ref count); + Log("Message: " + val); + }); Assert.True(sub.IsConnected(channel)); + Log("Publishing (1)..."); + Assert.Equal(0, count); + var publishedTo = await sub.PublishAsync(channel, "message1"); + // Client -> Redis -> Client -> handler takes just a moment + await UntilCondition(TimeSpan.FromSeconds(2), () => Volatile.Read(ref count) == 1); + Assert.Equal(1, count); + Log($" Published (1) to {publishedTo} subscriber(s)."); + Assert.Equal(1, publishedTo); + var endpoint = sub.SubscribedEndpoint(channel); var subscribedServer = muxer.GetServer(endpoint); var subscribedServerEndpoint = muxer.GetServerEndPoint(endpoint); @@ -64,14 +78,20 @@ public async Task SubscriptionNodeReconnecting() var newServer = subscription.GetCurrentServer(); Assert.NotNull(newServer); Assert.NotEqual(newServer, initialServer); - Log($"Now connected to: " + initialServer); + Log($"Now connected to: " + newServer); + + count = 0; + Log("Publishing (2)..."); + Assert.Equal(0, count); + publishedTo = await sub.PublishAsync(channel, "message2"); + // Client -> Redis -> Client -> handler takes just a moment + await UntilCondition(TimeSpan.FromSeconds(2), () => Volatile.Read(ref count) == 1); + Assert.Equal(1, count); + Log($" Published (2) to {publishedTo} subscriber(s)."); + + ClearAmbientFailures(); } - // 04:14:23.7955: Connection failed(InternalFailure): 127.0.0.1:7002/Subscription: StackExchange.Redis.RedisConnectionException: InternalFailure on 127.0.0.1:7002/Subscription, Initializing/NotStarted, last: SUBSCRIBE, origin: ConnectedAsync, outstanding: 0, last-read: 0s ago, last-write: 0s ago, keep-alive: 60s, state: Connecting, mgr: 9 of 10 available, last-heartbeat: never, last-mbeat: 0s ago, global: 23s ago, v: 2.5.49.64454 ---> StackExchange.Redis.RedisConnectionException: debugging - //at StackExchange.Redis.PhysicalConnection.OnDebugAbort() in C:\git\StackExchange\StackExchange.Redis\src\StackExchange.Redis\PhysicalConnection.cs:line 1560 - // at StackExchange.Redis.PhysicalConnection.d__104.MoveNext() in C:\git\StackExchange\StackExchange.Redis\src\StackExchange.Redis\PhysicalConnection.cs:line 1389 - // --- End of inner exception stack trace --- - // TODO: Primary/Replica failover // TODO: Subscribe failover, but with CommandFlags } From 837a654e0e2565edc91040882789998d9130c50f Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 24 Jan 2022 09:39:20 -0500 Subject: [PATCH 46/56] Assert up front --- tests/StackExchange.Redis.Tests/PubSub.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index f84efe8e9..46ce97a2a 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -325,6 +325,7 @@ public async Task PubSubGetAllAnyOrder() const int count = 1000; var syncLock = new object(); + Assert.True(sub.IsConnected()); var data = new HashSet(); await sub.SubscribeAsync(channel, (_, val) => { From b0001ab83e0e84574157aaac3b67045317b78ecd Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 24 Jan 2022 09:51:41 -0500 Subject: [PATCH 47/56] PubSub tests: log everything Hunting this immediate-subscribe-no-connection issue. --- tests/StackExchange.Redis.Tests/PubSub.cs | 45 ++++++++++++----------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 46ce97a2a..9c94213fc 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -20,7 +20,7 @@ public PubSub(ITestOutputHelper output, SharedConnectionFixture fixture) : base( [Fact] public async Task ExplicitPublishMode() { - using (var mx = Create(channelPrefix: "foo:")) + using (var mx = Create(channelPrefix: "foo:", log: Writer)) { var pub = mx.GetSubscriber(); int a = 0, b = 0, c = 0, d = 0; @@ -58,7 +58,7 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br { var pub = GetAnyMaster(muxer); var sub = muxer.GetSubscriber(); - await PingAsync(muxer, pub, sub).ForAwait(); + await PingAsync(pub, sub).ForAwait(); HashSet received = new(); int secondHandler = 0; string subChannel = (wildCard ? "a*c" : "abc") + breaker; @@ -88,7 +88,7 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br Assert.Equal(0, Thread.VolatileRead(ref secondHandler)); var count = sub.Publish(pubChannel, "def"); - await PingAsync(muxer, pub, sub, 3).ForAwait(); + await PingAsync(pub, sub, 3).ForAwait(); await UntilCondition(TimeSpan.FromSeconds(5), () => received.Count == 1); lock (received) @@ -102,7 +102,7 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br // unsubscribe from first; should still see second sub.Unsubscribe(subChannel, handler1); count = sub.Publish(pubChannel, "ghi"); - await PingAsync(muxer, pub, sub).ForAwait(); + await PingAsync(pub, sub).ForAwait(); lock (received) { Assert.Single(received); @@ -118,7 +118,7 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br // unsubscribe from second; should see nothing this time sub.Unsubscribe(subChannel, handler2); count = sub.Publish(pubChannel, "ghi"); - await PingAsync(muxer, pub, sub).ForAwait(); + await PingAsync(pub, sub).ForAwait(); lock (received) { Assert.Single(received); @@ -133,7 +133,7 @@ public async Task TestBasicPubSub(string channelPrefix, bool wildCard, string br [Fact] public async Task TestBasicPubSubFireAndForget() { - using (var muxer = Create(log: Writer, shared: false)) + using (var muxer = Create(shared: false, log: Writer)) { var profiler = muxer.AddProfiler(); var pub = GetAnyMaster(muxer); @@ -142,7 +142,7 @@ public async Task TestBasicPubSubFireAndForget() RedisChannel key = Me() + Guid.NewGuid(); HashSet received = new(); int secondHandler = 0; - await PingAsync(muxer, pub, sub).ForAwait(); + await PingAsync(pub, sub).ForAwait(); sub.Subscribe(key, (channel, payload) => { lock (received) @@ -162,9 +162,9 @@ public async Task TestBasicPubSubFireAndForget() Assert.Empty(received); } Assert.Equal(0, Thread.VolatileRead(ref secondHandler)); - await PingAsync(muxer, pub, sub).ForAwait(); + await PingAsync(pub, sub).ForAwait(); var count = sub.Publish(key, "def", CommandFlags.FireAndForget); - await PingAsync(muxer, pub, sub).ForAwait(); + await PingAsync(pub, sub).ForAwait(); await UntilCondition(TimeSpan.FromSeconds(5), () => received.Count == 1); Log(profiler); @@ -178,7 +178,7 @@ public async Task TestBasicPubSubFireAndForget() sub.Unsubscribe(key); count = sub.Publish(key, "ghi", CommandFlags.FireAndForget); - await PingAsync(muxer, pub, sub).ForAwait(); + await PingAsync(pub, sub).ForAwait(); Log(profiler); lock (received) { @@ -188,7 +188,7 @@ public async Task TestBasicPubSubFireAndForget() } } - private async Task PingAsync(IConnectionMultiplexer muxer, IServer pub, ISubscriber sub, int times = 1) + private async Task PingAsync(IServer pub, ISubscriber sub, int times = 1) { while (times-- > 0) { @@ -206,7 +206,7 @@ private async Task PingAsync(IConnectionMultiplexer muxer, IServer pub, ISubscri [Fact] public async Task TestPatternPubSub() { - using (var muxer = Create(shared: false)) + using (var muxer = Create(shared: false, log: Writer)) { var pub = GetAnyMaster(muxer); var sub = muxer.GetSubscriber(); @@ -231,9 +231,9 @@ public async Task TestPatternPubSub() } Assert.Equal(0, Thread.VolatileRead(ref secondHandler)); - await PingAsync(muxer, pub, sub).ForAwait(); + await PingAsync(pub, sub).ForAwait(); var count = sub.Publish("abc", "def"); - await PingAsync(muxer, pub, sub).ForAwait(); + await PingAsync(pub, sub).ForAwait(); await UntilCondition(TimeSpan.FromSeconds(5), () => received.Count == 1); lock (received) @@ -248,7 +248,7 @@ public async Task TestPatternPubSub() sub.Unsubscribe("a*c"); count = sub.Publish("abc", "ghi"); - await PingAsync(muxer, pub, sub).ForAwait(); + await PingAsync(pub, sub).ForAwait(); lock (received) { @@ -318,7 +318,7 @@ private void TestMassivePublish(ISubscriber conn, string channel, string caption [Fact] public async Task PubSubGetAllAnyOrder() { - using (var muxer = Create(syncTimeout: 20000, shared: false)) + using (var muxer = Create(syncTimeout: 20000, shared: false, log: Writer)) { var sub = muxer.GetSubscriber(); RedisChannel channel = Me(); @@ -440,7 +440,7 @@ await Assert.ThrowsAsync(async delegate [Fact] public async Task PubSubGetAllCorrectOrder_OnMessage_Sync() { - using (var muxer = Create(configuration: TestConfig.Current.RemoteServerAndPort, syncTimeout: 20000)) + using (var muxer = Create(configuration: TestConfig.Current.RemoteServerAndPort, syncTimeout: 20000, log: Writer)) { var sub = muxer.GetSubscriber(); RedisChannel channel = Me(); @@ -509,7 +509,7 @@ await Assert.ThrowsAsync(async delegate [Fact] public async Task PubSubGetAllCorrectOrder_OnMessage_Async() { - using (var muxer = Create(configuration: TestConfig.Current.RemoteServerAndPort, syncTimeout: 20000)) + using (var muxer = Create(configuration: TestConfig.Current.RemoteServerAndPort, syncTimeout: 20000, log: Writer)) { var sub = muxer.GetSubscriber(); RedisChannel channel = Me(); @@ -604,8 +604,8 @@ public async Task TestPublishWithSubscribers() public async Task TestMultipleSubscribersGetMessage() { var channel = Me(); - using (var muxerA = Create(shared: false)) - using (var muxerB = Create(shared: false)) + using (var muxerA = Create(shared: false, log: Writer)) + using (var muxerB = Create(shared: false, log: Writer)) using (var conn = Create()) { var listenA = muxerA.GetSubscriber(); @@ -635,7 +635,7 @@ public async Task TestMultipleSubscribersGetMessage() public async Task Issue38() { // https://code.google.com/p/booksleeve/issues/detail?id=38 - using (var pub = Create()) + using (var pub = Create(log: Writer)) { var sub = pub.GetSubscriber(); int count = 0; @@ -762,12 +762,13 @@ public async Task AzureRedisEventsAutomaticSubscribe() [Fact] public async Task SubscriptionsSurviveConnectionFailureAsync() { - using (var muxer = Create(allowAdmin: true, shared: false, syncTimeout: 1000) as ConnectionMultiplexer) + using (var muxer = Create(allowAdmin: true, shared: false, log: Writer, syncTimeout: 1000) as ConnectionMultiplexer) { var profiler = muxer.AddProfiler(); RedisChannel channel = Me(); var sub = muxer.GetSubscriber(); int counter = 0; + Assert.True(sub.IsConnected()); await sub.SubscribeAsync(channel, delegate { Interlocked.Increment(ref counter); From 1bc971dd6659ac22d7f906f48ab7e9842568e1fe Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 24 Jan 2022 19:37:21 -0500 Subject: [PATCH 48/56] Primary/Replica tests --- .../PubSubMultiserver.cs | 94 ++++++++++++++++++- 1 file changed, 91 insertions(+), 3 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs b/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs index 85ce6b2a4..a0a405fac 100644 --- a/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs +++ b/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs @@ -27,7 +27,7 @@ public void ChannelSharding() } [Fact] - public async Task SubscriptionNodeReconnecting() + public async Task ClusterNodeSubscriptionFailover() { Log("Connecting..."); using var muxer = Create(allowAdmin: true) as ConnectionMultiplexer; @@ -92,7 +92,95 @@ await sub.SubscribeAsync(channel, (channel, val) => ClearAmbientFailures(); } - // TODO: Primary/Replica failover - // TODO: Subscribe failover, but with CommandFlags + [Theory] + [InlineData(CommandFlags.PreferMaster, true)] + [InlineData(CommandFlags.PreferReplica, true)] + [InlineData(CommandFlags.DemandMaster, false)] + [InlineData(CommandFlags.DemandReplica, false)] + public async Task PrimaryReplicaSubscriptionFailover(CommandFlags flags, bool expectSuccess) + { + var config = TestConfig.Current.MasterServerAndPort + "," + TestConfig.Current.ReplicaServerAndPort; + Log("Connecting..."); + using var muxer = Create(configuration: config, allowAdmin: true) as ConnectionMultiplexer; + var sub = muxer.GetSubscriber(); + var channel = (RedisChannel)Me(); + + var count = 0; + Log("Subscribing..."); + await sub.SubscribeAsync(channel, (channel, val) => + { + Interlocked.Increment(ref count); + Log("Message: " + val); + }, flags); + Assert.True(sub.IsConnected(channel)); + + Log("Publishing (1)..."); + Assert.Equal(0, count); + var publishedTo = await sub.PublishAsync(channel, "message1"); + // Client -> Redis -> Client -> handler takes just a moment + await UntilCondition(TimeSpan.FromSeconds(2), () => Volatile.Read(ref count) == 1); + Assert.Equal(1, count); + Log($" Published (1) to {publishedTo} subscriber(s)."); + + var endpoint = sub.SubscribedEndpoint(channel); + var subscribedServer = muxer.GetServer(endpoint); + var subscribedServerEndpoint = muxer.GetServerEndPoint(endpoint); + + Assert.True(subscribedServer.IsConnected, "subscribedServer.IsConnected"); + Assert.True(subscribedServerEndpoint.IsConnected, "subscribedServerEndpoint.IsConnected"); + Assert.True(subscribedServerEndpoint.IsSubscriberConnected, "subscribedServerEndpoint.IsSubscriberConnected"); + + Assert.True(muxer.TryGetSubscription(channel, out var subscription)); + var initialServer = subscription.GetCurrentServer(); + Assert.NotNull(initialServer); + Assert.True(initialServer.IsConnected); + Log($"Connected to: " + initialServer); + + muxer.AllowConnect = false; + subscribedServerEndpoint.SimulateConnectionFailure(SimulatedFailureType.AllSubscription); + + Assert.True(subscribedServerEndpoint.IsConnected, "subscribedServerEndpoint.IsConnected"); + Assert.False(subscribedServerEndpoint.IsSubscriberConnected, "subscribedServerEndpoint.IsSubscriberConnected"); + + if (expectSuccess) + { + await UntilCondition(TimeSpan.FromSeconds(5), () => subscription.IsConnected); + Assert.True(subscription.IsConnected); + + var newServer = subscription.GetCurrentServer(); + Assert.NotNull(newServer); + Assert.NotEqual(newServer, initialServer); + Log($"Now connected to: " + newServer); + } + else + { + // This subscription shouldn't be able to reconnect by flags (demanding an unavailable server) + await UntilCondition(TimeSpan.FromSeconds(2), () => subscription.IsConnected); + Assert.False(subscription.IsConnected); + Log("Unable to reconnect (as expected)"); + + // Allow connecting back to the original + muxer.AllowConnect = true; + await UntilCondition(TimeSpan.FromSeconds(2), () => subscription.IsConnected); + Assert.True(subscription.IsConnected); + + var newServer = subscription.GetCurrentServer(); + Assert.NotNull(newServer); + Assert.Equal(newServer, initialServer); + Log($"Now connected to: " + newServer); + } + + + count = 0; + Log("Publishing (2)..."); + Assert.Equal(0, count); + publishedTo = await sub.PublishAsync(channel, "message2"); + // Client -> Redis -> Client -> handler takes just a moment + await UntilCondition(TimeSpan.FromSeconds(2), () => Volatile.Read(ref count) == 1); + Assert.Equal(1, count); + Log($" Published (2) to {publishedTo} subscriber(s)."); + + ClearAmbientFailures(); + } } } From 9c31958b6dcca2d2a9b5a8dfca4f9495c160fede Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 24 Jan 2022 20:13:50 -0500 Subject: [PATCH 49/56] Fix PubSub tests: can't share that connection yo These will pile up handlers if we share the multiplexer, doh. --- tests/StackExchange.Redis.Tests/PubSubMultiserver.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs b/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs index a0a405fac..1a667970e 100644 --- a/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs +++ b/tests/StackExchange.Redis.Tests/PubSubMultiserver.cs @@ -101,7 +101,7 @@ public async Task PrimaryReplicaSubscriptionFailover(CommandFlags flags, bool ex { var config = TestConfig.Current.MasterServerAndPort + "," + TestConfig.Current.ReplicaServerAndPort; Log("Connecting..."); - using var muxer = Create(configuration: config, allowAdmin: true) as ConnectionMultiplexer; + using var muxer = Create(configuration: config, shared: false, allowAdmin: true) as ConnectionMultiplexer; var sub = muxer.GetSubscriber(); var channel = (RedisChannel)Me(); From eed3ba032fd7c2cd5dd28c204cfc9971c8ded6ba Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 24 Jan 2022 20:33:17 -0500 Subject: [PATCH 50/56] Remove the .NET 5.0 from Windows build too... Speeding up the Windows PR build --- .github/workflows/CI.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 32c118f8c..6d2c0e8d6 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -50,7 +50,6 @@ jobs: with: dotnet-version: | 3.1.x - 5.0.x 6.0.x - name: .NET Build run: dotnet build Build.csproj -c Release /p:CI=true From 2fe8d134c61941fa0d777057af06fd7f04ec3658 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 24 Jan 2022 21:02:19 -0500 Subject: [PATCH 51/56] Sentinel: account for multi-suite failover states We can be in a still-recovering replica state after the first test suite ran - allow it a moment to get back to good when running these. --- tests/StackExchange.Redis.Tests/Sentinel.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/Sentinel.cs b/tests/StackExchange.Redis.Tests/Sentinel.cs index 332c37877..fbe4e05e9 100644 --- a/tests/StackExchange.Redis.Tests/Sentinel.cs +++ b/tests/StackExchange.Redis.Tests/Sentinel.cs @@ -359,8 +359,11 @@ public async Task SentinelMastersAsyncTest() } [Fact] - public void SentinelReplicasTest() + public async Task SentinelReplicasTest() { + // Give previous test run a moment to reset when multi-framework failover is in play. + await UntilCondition(TimeSpan.FromSeconds(5), () => SentinelServerA.SentinelReplicas(ServiceName).Length > 0); + var replicaConfigs = SentinelServerA.SentinelReplicas(ServiceName); Assert.True(replicaConfigs.Length > 0, "Has replicaConfigs"); Assert.True(replicaConfigs[0].ToDictionary().ContainsKey("name"), "replicaConfigs contains 'name'"); @@ -378,6 +381,9 @@ public void SentinelReplicasTest() [Fact] public async Task SentinelReplicasAsyncTest() { + // Give previous test run a moment to reset when multi-framework failover is in play. + await UntilCondition(TimeSpan.FromSeconds(5), () => SentinelServerA.SentinelReplicas(ServiceName).Length > 0); + var replicaConfigs = await SentinelServerA.SentinelReplicasAsync(ServiceName).ForAwait(); Assert.True(replicaConfigs.Length > 0, "Has replicaConfigs"); Assert.True(replicaConfigs[0].ToDictionary().ContainsKey("name"), "replicaConfigs contains 'name'"); From d23423502bf59b075f99bf7ef4bd267486764126 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 24 Jan 2022 21:57:36 -0500 Subject: [PATCH 52/56] ExecuteWithUnsubscribeViaSubscriber: don't share conn --- tests/StackExchange.Redis.Tests/Issues/Issue1101.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs b/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs index bb471b744..c81a6e004 100644 --- a/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs +++ b/tests/StackExchange.Redis.Tests/Issues/Issue1101.cs @@ -89,7 +89,7 @@ public async Task ExecuteWithUnsubscribeViaChannel() [Fact] public async Task ExecuteWithUnsubscribeViaSubscriber() { - using (var muxer = Create(log: Writer)) + using (var muxer = Create(shared: false, log: Writer)) { RedisChannel name = Me(); var pubsub = muxer.GetSubscriber(); From 33f52af9a0737b6e83ed95f6d0bef50cfde14458 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 24 Jan 2022 22:17:11 -0500 Subject: [PATCH 53/56] Re-disable TestMassivePublishWithWithoutFlush_Local Server flooding, is no good. --- tests/StackExchange.Redis.Tests/PubSub.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index 9c94213fc..2c4d6ef80 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -267,7 +267,7 @@ public void TestPublishWithNoSubscribers() } } - [Fact] + [FactLongRunning] public void TestMassivePublishWithWithoutFlush_Local() { using (var muxer = Create()) From dde51a69d44e3e6ed841eb005933aa93f864281b Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Tue, 25 Jan 2022 08:03:48 -0500 Subject: [PATCH 54/56] Add initial release notes These could use some eyes but making sure we don't forget it. --- docs/ReleaseNotes.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/ReleaseNotes.md b/docs/ReleaseNotes.md index 3cd8125b3..1dadce43a 100644 --- a/docs/ReleaseNotes.md +++ b/docs/ReleaseNotes.md @@ -9,6 +9,11 @@ - Moved tiebreaker fetching in connections into the handshake phase (streamline + simplification) (#1931 via NickCraver) - Fixed potential disposed object usage around Arenas (pulling in [Piplines.Sockets.Unofficial#63](https://github.com/mgravell/Pipelines.Sockets.Unofficial/pull/63) by MarcGravell) - Adds thread pool work item stats to exception messages to help diagnose contention (#1964 via NickCraver) +- Overhauls pub/sub implementation for correctness (#1947 via NickCraver) + - Fixes a race in subscribing right after connected + - Fixes a race in subscribing immediately before a publish + - Fixes subscription routing on clusters (spreading instead of choosing 1 node) + - More correctly reconnects subscriptions on connection failures, including to other endpoints ## 2.2.88 From 0465ee1813e40ef84582f04e3e38be820a2be0bb Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Thu, 27 Jan 2022 21:11:53 -0500 Subject: [PATCH 55/56] Update publish docs This is something that tripped me and at least 1 user up - let's save some pain. --- src/StackExchange.Redis/Interfaces/IDatabase.cs | 5 ++++- src/StackExchange.Redis/Interfaces/IDatabaseAsync.cs | 5 ++++- src/StackExchange.Redis/Interfaces/ISubscriber.cs | 10 ++++++++-- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/StackExchange.Redis/Interfaces/IDatabase.cs b/src/StackExchange.Redis/Interfaces/IDatabase.cs index 6a8e8a46e..3a9cc3e4e 100644 --- a/src/StackExchange.Redis/Interfaces/IDatabase.cs +++ b/src/StackExchange.Redis/Interfaces/IDatabase.cs @@ -851,7 +851,10 @@ public interface IDatabase : IRedis, IDatabaseAsync /// The channel to publish to. /// The message to send. /// The flags to use for this operation. - /// The number of clients that received the message. + /// + /// The number of clients that received the message *on the destination server*, + /// note that this doesn't mean much in a cluster as clients can get the message through other nodes. + /// /// https://redis.io/commands/publish long Publish(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None); diff --git a/src/StackExchange.Redis/Interfaces/IDatabaseAsync.cs b/src/StackExchange.Redis/Interfaces/IDatabaseAsync.cs index 63f5ad644..9626945a0 100644 --- a/src/StackExchange.Redis/Interfaces/IDatabaseAsync.cs +++ b/src/StackExchange.Redis/Interfaces/IDatabaseAsync.cs @@ -828,7 +828,10 @@ public interface IDatabaseAsync : IRedisAsync /// The channel to publish to. /// The message to send. /// The flags to use for this operation. - /// The number of clients that received the message. + /// + /// The number of clients that received the message *on the destination server*, + /// note that this doesn't mean much in a cluster as clients can get the message through other nodes. + /// /// https://redis.io/commands/publish Task PublishAsync(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None); diff --git a/src/StackExchange.Redis/Interfaces/ISubscriber.cs b/src/StackExchange.Redis/Interfaces/ISubscriber.cs index 11e985a0b..f26ea7a11 100644 --- a/src/StackExchange.Redis/Interfaces/ISubscriber.cs +++ b/src/StackExchange.Redis/Interfaces/ISubscriber.cs @@ -38,7 +38,10 @@ public interface ISubscriber : IRedis /// The channel to publish to. /// The message to publish. /// The command flags to use. - /// the number of clients that received the message. + /// + /// The number of clients that received the message *on the destination server*, + /// note that this doesn't mean much in a cluster as clients can get the message through other nodes. + /// /// https://redis.io/commands/publish long Publish(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None); @@ -48,7 +51,10 @@ public interface ISubscriber : IRedis /// The channel to publish to. /// The message to publish. /// The command flags to use. - /// the number of clients that received the message. + /// + /// The number of clients that received the message *on the destination server*, + /// note that this doesn't mean much in a cluster as clients can get the message through other nodes. + /// /// https://redis.io/commands/publish Task PublishAsync(RedisChannel channel, RedisValue message, CommandFlags flags = CommandFlags.None); From 169a173e9371c265b27f50ac9a8979cb4ea8f80e Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 31 Jan 2022 08:19:04 -0500 Subject: [PATCH 56/56] ReconfigureAsyns: FireAndForget for subscription re-establish This is effectively the behavior we had before (but we are ensured connected now). We just want the count here and to start them, so let's do that also pipelining the `n` commands as we did before instead of awaiting each. We also don't need the `Task` overhead. This makes moving to a variadic command set for this less urgent. --- .../ConnectionMultiplexer.cs | 5 +++-- src/StackExchange.Redis/RedisSubscriber.cs | 19 ++++--------------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.cs b/src/StackExchange.Redis/ConnectionMultiplexer.cs index 0463ab62d..13eebbee3 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.cs @@ -1898,14 +1898,15 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, LogP } if (!first) { - long subscriptionChanges = await EnsureSubscriptionsAsync(); + // Calling the sync path here because it's all fire and forget + long subscriptionChanges = EnsureSubscriptions(CommandFlags.FireAndForget); if (subscriptionChanges == 0) { log?.WriteLine("No subscription changes necessary"); } else { - log?.WriteLine($"Subscriptions reconfigured: {subscriptionChanges}"); + log?.WriteLine($"Subscriptions attempting reconnect: {subscriptionChanges}"); } } if (showStats) diff --git a/src/StackExchange.Redis/RedisSubscriber.cs b/src/StackExchange.Redis/RedisSubscriber.cs index 28ff75c98..5ca22dfb5 100644 --- a/src/StackExchange.Redis/RedisSubscriber.cs +++ b/src/StackExchange.Redis/RedisSubscriber.cs @@ -107,28 +107,17 @@ internal void UpdateSubscriptions() /// /// Ensures all subscriptions are connected to a server, if possible. /// - internal void EnsureSubscriptions(CommandFlags flags = CommandFlags.None) + /// The count of subscriptions attempting to reconnect (same as the count currently not connected). + internal long EnsureSubscriptions(CommandFlags flags = CommandFlags.None) { // TODO: Subscribe with variadic commands to reduce round trips - foreach (var pair in subscriptions) - { - DefaultSubscriber.EnsureSubscribedToServer(pair.Value, pair.Key, flags, true); - } - } - - /// - /// Ensures all subscriptions are connected to a server, if possible. - /// - internal async Task EnsureSubscriptionsAsync(CommandFlags flags = CommandFlags.None) - { - // TODO: Evaluate performance here, this isn't good for a large number of subscriptions. - // It's probable we want to fire and forget `n` here, recording how many are going to try to reconnect? long count = 0; foreach (var pair in subscriptions) { - if (await DefaultSubscriber.EnsureSubscribedToServerAsync(pair.Value, pair.Key, flags, true)) + if (!pair.Value.IsConnected) { count++; + DefaultSubscriber.EnsureSubscribedToServer(pair.Value, pair.Key, flags, true); } } return count;