AMR: use RESP3 and disable the default config channel by default (#3067)

* AMR: use RESP3 and disable the default config channel by default

* release notes

* support TLS (self-signed) in the test server, use for the AMR test

* don't do the AMR post-connection callback

* fix test break in Issue883 due to lazy provider lookup

* improved release notes

* verify config override works as expected

* spin up pub/sub on demand (or if pub/sub is active)

* build warning cleanup

* logging

* breakfix

* format

* AMR test for RESP2

Author: mgravell

Directory.Build.props

@@ -43,7 +43,7 @@
     <PackageReference Include="StyleCop.Analyzers" PrivateAssets="All" />
   </ItemGroup>
 
-  <ItemGroup Condition="'$(MSBuildProjectName)' != 'Build' and '$(MSBuildProjectName)' != 'StackExchange.Redis.Build'">
+  <ItemGroup Condition="'$(MSBuildProjectName)' != 'Build' and '$(MSBuildProjectName)' != 'StackExchange.Redis.Build' and '$(MSBuildProjectName)' != 'docker' and '$(MSBuildProjectName)' != 'docs' and '$(MSBuildProjectName)' != '.github'">
     <!-- for everything except the build-tools project itself and the Build.csproj wrapper: use the build project as an analyzer -->
     <ProjectReference Include="..\..\eng\StackExchange.Redis.Build\StackExchange.Redis.Build.csproj"
                       OutputItemType="Analyzer" ReferenceOutputAssembly="false" />

docs/ReleaseNotes.md

@@ -8,17 +8,17 @@ Current package versions:
 
 ## Unreleased
 
-- (none)
+- Prefer RESP3 and avoid opening a separate subscription connection for Azure Managed Redis endpoints ([#3067 by @mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/3067))
 
 ## 2.12.27
 
+- Recognize Azure Managed Redis (AMR) resources in new Azure clouds ([#3068 by @philon-msft](https://github.com/StackExchange/StackExchange.Redis/pull/3068))
 - Remove `[Experimental]` 8.8 `GCRA` feature ([#3074 by @mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/3074))
 - Detect server-mode correctly on Valkey 8+ instances ([#3050 by @wipiano](https://github.com/StackExchange/StackExchange.Redis/pull/3050))
 - Add Redis 8.8 stream negative acknowledgements (`XNACK`) ([#3058 by @mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/3058))
 - Add experimental `Aggregate.Count` support for sorted-set combination operations against Redis 8.8 ([#3059 by @mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/3059))
-- Support sub-key (hash field) notifications ([#3062 by @mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/3062))
+- Support Redis 8.8 sub-key (hash field) notifications ([#3062 by @mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/3062))
 - Add `ValueCondition` overloads for `SortedSetIncrement`/`SortedSetIncrementAsync`, supporting `ZADD INCR` with existence conditions ([#3071 by @mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/3071))
-- Recognize Azure Managed Redis (AMR) resources in new Azure clouds ([#3068 by @philon-msft](https://github.com/StackExchange/StackExchange.Redis/pull/3068))
 
 ## 2.12.14
 

docs/Resp3.md

@@ -2,35 +2,80 @@
 
 RESP2 and RESP3 are evolutions of the Redis protocol, with RESP3 existing from Redis server version 6 onwards (v7.2+ for Redis Enterprise). The main differences are:
 
-1. RESP3 can carry out-of-band / "push" messages on a single connection, where-as RESP2 requires a separate connection for these messages
-2. RESP3 can (when appropriate) convey additional semantic meaning about returned payloads inside the same result structure
-3. Some commands (see [this topic](https://github.com/redis/redis-doc/issues/2511)) return different result structures in RESP3 mode; for example a flat interleaved array might become a jagged array
+1. RESP3 can carry out-of-band / "push" messages on a single connection, where-as RESP2 requires a separate connection for out-of-band (pub/sub) messages
+    - this single connection can be of huge benefit in high-usage servers, as it halves the number of connections required
+2. RESP3 supports *additional* out-of-band messages that cannot be expressed in RESP2, which allows advanced features such as "smart client handoffs" (a family of
+   server maintenance notifications)
+    - these features (not yet implemented in SE.Redis) allow for greater stability in complex deployments
+3. RESP3 can (when appropriate) convey additional semantic meaning about returned payloads inside the same result structure
+    - this is *mostly* relevant to client libraries that do not explicitly interpret the results before exposing to the user, so this does not directly impact SE.Redis itself,
+      but it is relevant to consumers of SE.Redis that use Lua scripts or ad-hoc commands
 
-For most people, #1 is the main reason to consider RESP3, as in high-usage servers - this can halve the number of connections required.
-This is particularly useful in hosted environments where the number of inbound connections to the server is capped as part of a service plan.
-Alternatively, where users are currently choosing to disable the out-of-band connection to achieve this, they may now be able to re-enable this
-(for example, to receive server maintenance notifications) *without* incurring any additional connection overhead.
+For many users, using RESP3 is a "no-brainer" - it offers significant benefits with no real downsides. However, there are some important things to be aware of, and some
+migration work that may be required. In particular, some commands *return different result structures* in RESP3 mode; for example a jagged (nested) array might become a "map"
+(essentially an interleaved flat array). SE.Redis has been updated to handle these cases transparently, but if you are using `Execute[Async]` or `ScriptEvaluate[Async]` (or if
+you are using an additional library that issues ad-hoc commands or scripts on your behalf) you may need to update your processing code to compensate for this. This is discussed more below.
 
-Because of the significance of #3 (and to avoid breaking your code), the library does not currently default to RESP3 mode. This must be enabled explicitly
-via `ConfigurationOptions.Protocol` or by adding `,protocol=resp3` (or `,protocol=3`) to the configuration string.
+# Enabling RESP3
 
----
+RESP2 and RESP3 are both supported options (if the server does not support RESP3, RESP2 will always be used). To make full use of the benefits of RESP3,
+the library is moving in the direction of *preferring* RESP3. The default behaviour is:
 
-#3 is a critical one - the library *should* already handle all documented commands that have revised results in RESP3, but if you're using
-`Execute[Async]` to issue ad-hoc commands, you may need to update your processing code to compensate for this, ideally using detection to handle
-*either* format so that the same code works in both REP2 and RESP3. Since the impacted commands are handled internally by the library, in reality
-this should not usually present a difficulty.
+| Library version         | Endpoint                                                        | Default protocol
+|-------------------------|-----------------------------------------------------------------|-
+| < 2.13               | (any)                                                           | RESP2
+| >= 2.13 and < 3.0 | (non-AMR)                                                       | RESP2
+| >= 2.13 and < 3.0 | [AMR](https://azure.microsoft.com/products/managed-redis) | RESP3
+| &gt; 3.0<sup>†</sup>    | (any)                                                           | RESP3
 
-The minor (#2) and major (#3) differences to results are only visible to your code when using:
+<sup>†</sup> = planned
+
+You can override this behaviour by setting the `protocol` option in the connection string, or by setting the `ConfigurationOptions.Protocol` property:
+
+```csharp
+var options = ConfigurationOptions.Parse("someserver");
+options.Protocol = RedisProtocol.Resp3; // or .Resp2
+var muxer = await ConnectionMultiplexer.ConnectAsync(options);
+```
+
+or
+
+```csharp
+var options = ConfigurationOptions.Parse("someserver,protocol=resp3"); // or =resp2
+var muxer = await ConnectionMultiplexer.ConnectAsync(options);
+```
+
+You can use this configuration to *explicitly enable* RESP3 on earlier library versions, or to *explicitly disable* RESP3 on later versions, if you encounter issues.
+
+# Handling RESP3
+
+For most users, *no additional work will be required*, or the additional work may be limited to updating libraries; for example, For example, [NRedisStack](https://www.nuget.org/packages/NRedisStack/)
+now fully supports RESP3 for the commands it exposes (search, json, time-series, etc).
+
+Scenarios impacted by RESP3 include:
 
 - Lua scripts invoked via the `ScriptEvaluate[Async](...)` or related APIs, that either:
   - Uses the `redis.setresp(3)` API and returns a value from `redis.[p]call(...)`
   - Returns a value that satisfies the [LUA to RESP3 type conversion rules](https://redis.io/docs/manual/programmability/lua-api/#lua-to-resp3-type-conversion)
-- Ad-hoc commands (in particular: *modules*) that are invoked via the `Execute[Async](string command, ...)` API
+- Ad-hoc commands that are invoked via the `Execute[Async](string command, ...)` API
+
+This delta is *especially* pronounced for some of the "modules" in Redis, even those that now ship by default in OSS Redis, including:
+- "search" (`FT.SEARCH`, `FT.AGGREGATE`, etc.)
+- "time-series" (`TS.RANGE`, etc.)
+- "json" (`JSON.NUMINCRBY`, etc.)
+
+Note that NRedisStack wraps most of these common modules, and has been updated to understand RESP3; if you are using these modules via NRedisStack, you should update to the latest version; if
+you are using these modules via ad-hoc commands, you may need to update your processing code to compensate for this, or consider using NRedisStack instead, which will handle the RESP3 conversion for you.
 
-...both which return `RedisResult`. **If you are not using these APIs, you should not need to do anything additional.**
+This leaves a small category of users who are currently using the `RedisResult` type directly (via `Execute[Async](...)` or `ScriptEvaluate[Async](...)`).
 
-Historically, you could use the `RedisResult.Type` property to query the type of data returned (integer, string, etc). In particular:
+## Impact on RedisResult
+
+Firstly, note that it is possible that the *structure* of the data changes between RESP2 and RESP3; for example, a jagged array might become a map, or a single string value might become an array. You will
+need to identify these changes (typically via integration tests) and update your code accordingly, ideally with detection code to handle *either* structure so that the same code works in both REP2 and RESP3.
+
+This is usually combined by using the `RedisResult.Resp3Type` property to query the type of data returned (integer, string, etc). Historically, you could use the `RedisResult.Type` property to query the type of data returned (integer, string, etc).
+With RESP3, this is extended:
 
 - Two new properties are added: `RedisResult.Resp2Type` and `RedisResult.Resp3Type`
   - The `Resp3Type` property exposes the new semantic data (when using RESP3) - for example, it can indicate that a value is a double-precision number, a boolean, a map, etc (types that did not historically exist)
@@ -42,4 +87,7 @@ Possible changes required due to RESP3:
 
 1. To prevent build warnings, replace usage of `ResultType.MultiBulk` with `ResultType.Array`, and usage of `RedisResult.Type` with `RedisResult.Resp2Type`
 2. If you wish to exploit the additional semantic data when enabling RESP3, use `RedisResult.Resp3Type` where appropriate
-3. If you are enabling RESP3, you must verify whether the commands you are using can give different result shapes on RESP3 connections
+3. If you are enabling RESP3, you must verify whether the commands you are using can give different result shapes on RESP3 connections
+
+An example of the types of changes required may be seen in the [NRedisStack #471](https://github.com/redis/NRedisStack/pull/471) pull-request, which updates result processing for multiple modules
+(and changes the integration tests to run on RESP2 and RESP3 separately).

src/StackExchange.Redis/Configuration/AzureManagedRedisOptionsProvider.cs

@@ -1,7 +1,6 @@
 using System;
 using System.Net;
 using System.Threading.Tasks;
-using StackExchange.Redis.Maintenance;
 
 namespace StackExchange.Redis.Configuration
 {
@@ -56,9 +55,15 @@ private bool IsHostInDomains(string hostName, string[] domains)
 
         /// <inheritdoc/>
         public override Task AfterConnectAsync(ConnectionMultiplexer muxer, Action<string> log)
-            => AzureMaintenanceEvent.AddListenerAsync(muxer, log);
+            => Task.CompletedTask;
 
         /// <inheritdoc/>
         public override bool GetDefaultSsl(EndPointCollection endPoints) => true;
+
+        /// <inheritdoc/>
+        public override RedisProtocol? Protocol => RedisProtocol.Resp3; // prefer RESP3 on AMR
+
+        /// <inheritdoc/>
+        public override string ConfigurationChannel => ""; // disable on AMR
     }
 }

src/StackExchange.Redis/Configuration/DefaultOptionsProvider.cs

@@ -259,6 +259,11 @@ protected virtual string GetDefaultClientName() =>
         /// </summary>
         public virtual bool SetClientLibrary => true;
 
+        /// <summary>
+        /// Gets the preferred protocol to use for the connection.
+        /// </summary>
+        public virtual RedisProtocol? Protocol => null;
+
         /// <summary>
         /// Tries to get the RoleInstance Id if Microsoft.WindowsAzure.ServiceRuntime is loaded.
         /// In case of any failure, swallows the exception and returns null.

src/StackExchange.Redis/ConfigurationOptions.cs

@@ -1169,13 +1169,18 @@ private ConfigurationOptions DoParse(string configuration, bool ignoreUnknown)
         /// <summary>
         /// Specify the redis protocol type.
         /// </summary>
-        public RedisProtocol? Protocol { get; set; }
+        public RedisProtocol? Protocol
+        {
+            get => field ?? Defaults.Protocol;
+            set;
+        }
 
         internal bool TryResp3()
         {
+            var protocol = Protocol;
             // note: deliberately leaving the IsAvailable duplicated to use short-circuit
 
-            // if (Protocol is null)
+            // if (protocol is null)
             // {
             //     // if not specified, lean on the server version and whether HELLO is available
             //     return new RedisFeatures(DefaultVersion).Resp3 && CommandMap.IsAvailable(RedisCommand.HELLO);
@@ -1187,7 +1192,7 @@ internal bool TryResp3()
             // edge case in the library itself, the break is still visible to external callers via Execute[Async]; with an
             // abundance of caution, we are therefore making RESP3 explicit opt-in only for now; we may revisit this in a major
             {
-                return Protocol.GetValueOrDefault() >= RedisProtocol.Resp3 && CommandMap.IsAvailable(RedisCommand.HELLO);
+                return protocol.GetValueOrDefault() >= RedisProtocol.Resp3 && CommandMap.IsAvailable(RedisCommand.HELLO);
             }
         }
 

src/StackExchange.Redis/ConnectionMultiplexer.cs

@@ -1370,9 +1370,11 @@ internal void GetStatus(ILogger? log)
 
         private void ActivateAllServers(ILogger? log)
         {
+            // bool hasSubscriptions = GetSubscriptionsCount() != 0;
             foreach (var server in GetServerSnapshot())
             {
                 server.Activate(ConnectionType.Interactive, log);
+                // if (hasSubscriptions && server.SupportsSubscriptions && !server.KnowOrAssumeResp3())
                 if (server.SupportsSubscriptions && !server.KnowOrAssumeResp3())
                 {
                     // Intentionally not logging the sub connection

src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt

@@ -1 +1,4 @@
 #nullable enable
+override StackExchange.Redis.Configuration.AzureManagedRedisOptionsProvider.ConfigurationChannel.get -> string!
+override StackExchange.Redis.Configuration.AzureManagedRedisOptionsProvider.Protocol.get -> StackExchange.Redis.RedisProtocol?
+virtual StackExchange.Redis.Configuration.DefaultOptionsProvider.Protocol.get -> StackExchange.Redis.RedisProtocol?

src/StackExchange.Redis/ServerEndPoint.cs

@@ -107,7 +107,6 @@ public int Databases
         public bool IsConnecting => interactive?.IsConnecting == true;
         public bool IsConnected => interactive?.IsConnected == true;
         public bool IsSubscriberConnected => KnowOrAssumeResp3() ? IsConnected : subscription?.IsConnected == true;
-
         public bool KnowOrAssumeResp3()
         {
             var protocol = interactive?.Protocol;
@@ -627,7 +626,7 @@ internal bool IsSelectable(RedisCommand command, bool allowDisconnected = false)
         {
             // Until we've connected at least once, we're going to have a DidNotRespond unselectable reason present
             var bridge = unselectableReasons == 0 || (allowDisconnected && unselectableReasons == UnselectableFlags.DidNotRespond)
-                ? GetBridge(command, false)
+                ? GetBridge(command, true)
                 : null;
 
             return bridge != null && (allowDisconnected || bridge.IsConnected);

tests/StackExchange.Redis.Tests/ConfigTests.cs

@@ -166,6 +166,26 @@ public void ConfigurationOptionsDefaultForAzureManagedRedis(string hostAndPort,
         Assert.Equal(sslShouldBeEnabled, options.Ssl);
     }
 
+    [Theory]
+    // azure managed redis, no overrides
+    [InlineData("contoso.redis.azure.net:10000", RedisProtocol.Resp3, true)] // default
+    [InlineData("contoso.redis.azure.net:10000,protocol=resp2", RedisProtocol.Resp2, false)] // opt-out
+    [InlineData("contoso.redis.azure.net:10000,protocol=resp3", RedisProtocol.Resp3, true)] // opt-in
+    // azure redis cache, no overrides (we expect this to change in v3)
+    [InlineData("contoso.redis.cache.windows.net:6380", null, false)] // default
+    [InlineData("contoso.redis.cache.windows.net:6380,protocol=resp2", RedisProtocol.Resp2, false)] // opt-out
+    [InlineData("contoso.redis.cache.windows.net:6380,protocol=resp3", RedisProtocol.Resp3, true)] // opt-in
+    // arbitrary endpoint (we expect this to change in v3)
+    [InlineData("myserver:6379", null, false)] // default
+    [InlineData("myserver:6379,protocol=resp2", RedisProtocol.Resp2, false)] // opt-out
+    [InlineData("myserver:6379,protocol=resp3", RedisProtocol.Resp3, true)] // opt-in
+    public void CorrectRespProtocol(string config, RedisProtocol? expected, bool useResp3)
+    {
+        var options = ConfigurationOptions.Parse(config);
+        Assert.Equal(expected, options.Protocol);
+        Assert.Equal(useResp3, options.TryResp3());
+    }
+
     [Fact]
     public void ConfigurationOptionsForAzureWhenSpecified()
     {