Add SER308: flag the library's own Wait/WaitAll/TryWait helpers

docs/SyncOverAsync.md

@@ -1,8 +1,8 @@
 # Sync over async, and thread-pool starvation
 
-If you are here from a `SER307` warning or a support conversation: this page explains why blocking on an
-asynchronous redis call is worse than it looks, why the symptom shows up somewhere else entirely, and what to
-do about it.
+If you are here from a `SER307` or `SER308` warning, or from a support conversation: this page explains why
+blocking on an asynchronous redis call is worse than it looks, why the symptom shows up somewhere else
+entirely, and what to do about it.
 
 ## The short version
 
@@ -134,6 +134,16 @@ many connections from a small fixed set of threads rather than a pair per connec
 would make this practical at any width. There is no date on it, and nothing here depends on it; but if you
 have read the caveat above and thought "not with my shard count", the answer is "not yet" rather than "no".
 
+### The `Wait` helpers are the same thing
+
+`Wait`, `WaitAll` and `TryWait` — on `IDatabase`/`IServer`/`ISubscriber` via `IRedisAsync`, and on
+`IConnectionMultiplexer` — are the library's own blocking helpers, and everything above applies to them: the
+calling thread is held for the round-trip while the reply needs a thread of its own.
+
+They are **not worse** than `.Result` — they apply the multiplexer's configured timeout, so they fail rather
+than hanging forever — but that is a better *failure*, not an escaped problem. `SER308` flags them; await the
+task instead.
+
 ## Fire-and-forget is a special case
 
 `CommandFlags.FireAndForget` returns an already-completed task carrying the default value, so blocking on one
@@ -146,17 +156,18 @@ if you actually want a result — see `SER306`.
 The package ships a Roslyn analyzer that flags these at build time:
 
 - **`SER307`** — blocking on a redis call instead of awaiting it. This page is what it links to.
+- **`SER308`** — the same, through the library's own `Wait`/`WaitAll`/`TryWait` helpers.
 - **`SER306`** — reading a fire-and-forget result, which is always the default value. SER307 hands that case
   to this rule, since blocking on an already-completed task is not what starves anything.
 
 See [Analyzer rules](rules/) for the full set, including the transaction rules, which describe a different
 problem.
 
-### Turning SER307 off
+### Turning these off
 
-It is a **warning**, so a build with `TreatWarningsAsErrors` will fail until you act on it or turn it down.
-Nobody is going to rewrite a large codebase in an afternoon, and a rule you cannot silence is a rule people
-rip out entirely, so:
+Both are **warnings**, so a build with `TreatWarningsAsErrors` will fail until you act on them or turn them
+down. Nobody is going to rewrite a large codebase in an afternoon, and a rule you cannot silence is a rule
+people rip out entirely, so:
 
 For a single call site you have decided about — a legacy entry point, an interface you do not control:
 
@@ -167,15 +178,20 @@ For a single call site you have decided about — a legacy entry point, an inter
 For a project, while you work through it:
 
 ```xml
-<NoWarn>$(NoWarn);SER307</NoWarn>
+<NoWarn>$(NoWarn);SER307;SER308</NoWarn>
 ```
 
-Or turn it down rather than off, so it stays visible in the IDE without failing builds — in `.editorconfig`:
+Or turn them down rather than off, so they stay visible in the IDE without failing builds — in
+`.editorconfig`:
 
 ```ini
 dotnet_diagnostic.SER307.severity = suggestion   # or none, silent, warning, error
+dotnet_diagnostic.SER308.severity = suggestion
 ```
 
+Note these are IDs of our own rather than `CS0618`, and that is the point: silencing them silences *this*,
+not every deprecation you have ever taken a dependency on.
+
 Worth saying plainly: suppressing it does not make the problem go away, and if you are here because of
 timeouts then this rule is pointing at their cause. The `#pragma` form is the one to prefer where you can,
 because it records the decision at the site and keeps the rest of the codebase covered.

docs/rules/index.md

@@ -35,6 +35,7 @@ Unlike everything under [Usage](#usage), these describe code that does not do wh
 - [SER305](SER305) - **error**: waiting for a queued command before `Execute[Async]` never completes
 - [SER306](SER306) - waiting for a fire-and-forget result, which is always the default value
 - [SER307](../SyncOverAsync) - blocking on a redis call instead of awaiting it ("sync over async")
+- [SER308](../SyncOverAsync) - the same, via the library's own `Wait`/`WaitAll`/`TryWait` helpers
 
 ## Usage
 

eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md

@@ -14,3 +14,4 @@ Rule ID | Category | Severity | Notes
 SER305  | Usage    | Error    | QueuedResultAnalyzer: waiting for a command queued on a transaction or batch, before Execute[Async]() sends it, never completes
 SER306  | Usage    | Warning  | QueuedResultAnalyzer: waiting for a fire-and-forget result reads the default value rather than the server's answer
 SER307  | Usage    | Warning  | QueuedResultAnalyzer: blocking on a redis call instead of awaiting it, which ties up a thread-pool thread while the reply needs one of its own
+SER308  | Usage    | Warning  | QueuedResultAnalyzer: calling the library's own Wait/WaitAll/TryWait helpers, which block the calling thread

eng/StackExchange.Redis.Build/Diagnostics.cs

@@ -252,6 +252,45 @@ internal static class Diagnostics
         description: "Blocking on an asynchronous redis call holds a thread-pool thread while waiting for a reply whose processing also needs the thread-pool; enough of these will starve the pool, at which point replies cannot be processed at all.",
         helpLinkUri: "https://seredis.dev/SyncOverAsync");
 
+    /// <summary>
+    /// Calling the library's own blocking helpers - <c>Wait</c>, <c>WaitAll</c>, <c>TryWait</c>.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// Same problem as <see cref="BlockingOnRedisCall"/> - a thread held for the round-trip, while the reply
+    /// needs a thread of its own - but reached through the API the library itself offers for it, rather than
+    /// through <c>.Result</c>. Shipping SER307 while the library provided a blessed way to do the same thing
+    /// was only ever half a position.
+    /// </para>
+    /// <para>
+    /// This is a rule rather than <c>[Obsolete]</c> deliberately, and the reason is about how it is turned
+    /// off. <c>[Obsolete]</c> reports <c>CS0618</c>, which is shared with every obsoletion from every source,
+    /// so a consumer who wants to silence *this* has to silence *all* of them - including deprecations in
+    /// their own code and in unrelated packages. <c>ObsoleteAttribute.DiagnosticId</c> would solve that, but
+    /// it is net5+ and this library still targets netstandard2.0, so it would give a granular ID on some
+    /// target frameworks and <c>CS0618</c> on others. An ID of our own behaves the same everywhere, and sits
+    /// with the rest of the family. <c>[Experimental]</c> was considered and rejected: it is granular, but
+    /// these APIs are long-standing rather than preview, so it would be saying something untrue.
+    /// </para>
+    /// <para>
+    /// Worth revisiting rather than settled: if netstandard2.0 and net4x are ever dropped, the attribute
+    /// becomes strictly the better instrument - it is metadata, so it needs no analyzer to be loaded and
+    /// works in every tool - and this rule could retire in its favour. A hybrid was considered now and is
+    /// not worth it: the attribute would need <c>#if</c> on the interface *and* on ConnectionMultiplexer's
+    /// own members (marking only the interface does not warn through the class), and on modern targets both
+    /// mechanisms would report at the same location.
+    /// </para>
+    /// </remarks>
+    public static readonly DiagnosticDescriptor BlockingHelper = new(
+        id: "SER308",
+        title: "Blocking on a task through the library's Wait helpers",
+        messageFormat: "'{0}' blocks the calling thread until the task completes, and the reply needs a thread of its own to be processed; await the task instead",
+        category: UsageCategory,
+        defaultSeverity: DiagnosticSeverity.Warning,
+        isEnabledByDefault: true,
+        description: "The Wait/WaitAll/TryWait helpers block the calling thread while waiting for a reply whose processing also needs the thread-pool; enough of these will starve the pool.",
+        helpLinkUri: "https://seredis.dev/SyncOverAsync");
+
     /// <summary>
     /// The generated code cannot be compiled at the language version in effect, so nothing was generated.
     /// </summary>

eng/StackExchange.Redis.Build/QueuedResultAnalyzer.cs

@@ -1,4 +1,5 @@
-using System.Collections.Immutable;
+using System.Collections.Generic;
+using System.Collections.Immutable;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.Operations;
@@ -46,7 +47,8 @@ public sealed class QueuedResultAnalyzer : DiagnosticAnalyzer
         = ImmutableArray.Create(
             Diagnostics.AwaitBeforeExecute,
             Diagnostics.AwaitFireAndForgetResult,
-            Diagnostics.BlockingOnRedisCall);
+            Diagnostics.BlockingOnRedisCall,
+            Diagnostics.BlockingHelper);
 
     /// <inheritdoc/>
     public override void Initialize(AnalysisContext context)
@@ -68,6 +70,17 @@ public override void Initialize(AnalysisContext context)
 
     private static void Analyze(OperationAnalysisContext context, KnownSymbols known)
     {
+        // SER308 is a plain call rather than a wait wrapped around one, so it is answered first and on its
+        // own terms: the blocking is inside the helper, and there is nothing here to unwrap
+        if (context.Operation is IInvocationOperation helper && known.IsBlockingHelper(helper.TargetMethod))
+        {
+            context.ReportDiagnostic(Diagnostic.Create(
+                Diagnostics.BlockingHelper,
+                helper.Syntax.GetLocation(),
+                helper.TargetMethod.Name));
+            return;
+        }
+
         // what is being waited for, and how - awaiting is correct usage nearly everywhere, blocking is not
         if (Waited(context.Operation, known) is not { } waited) return;
         if (waited.Call is not { } call || !known.IsRedis(call.Instance?.Type)) return;
@@ -235,8 +248,9 @@ private static string Describe(ITypeSymbol? type, KnownSymbols known)
 
     private sealed class KnownSymbols
     {
-        private KnownSymbols(INamedTypeSymbol? redisAsync, INamedTypeSymbol? batch, INamedTypeSymbol? transactionAsync, INamedTypeSymbol? transaction, INamedTypeSymbol? commandFlags, INamedTypeSymbol? task, int? fireAndForgetValue)
+        private KnownSymbols(List<IMethodSymbol> blockingHelpers, INamedTypeSymbol? redisAsync, INamedTypeSymbol? batch, INamedTypeSymbol? transactionAsync, INamedTypeSymbol? transaction, INamedTypeSymbol? commandFlags, INamedTypeSymbol? task, int? fireAndForgetValue)
         {
+            BlockingHelpers = blockingHelpers;
             RedisAsync = redisAsync;
             Batch = batch;
             TransactionAsync = transactionAsync;
@@ -246,6 +260,15 @@ private KnownSymbols(INamedTypeSymbol? redisAsync, INamedTypeSymbol? batch, INam
             FireAndForgetValue = fireAndForgetValue;
         }
 
+        /// <summary>
+        /// <c>Wait</c>/<c>WaitAll</c>/<c>TryWait</c>, gathered from both declaring interfaces.
+        /// </summary>
+        /// <remarks>
+        /// Two unrelated interfaces declare these - IRedisAsync for database/server/subscriber calls, and
+        /// IConnectionMultiplexer for its own - so one lookup would silently cover only half the surface.
+        /// </remarks>
+        private List<IMethodSymbol> BlockingHelpers { get; }
+
         /// <summary>The root of every async redis surface: IDatabaseAsync, IServer and ISubscriber all derive from it.</summary>
         private INamedTypeSymbol? RedisAsync { get; }
 
@@ -284,7 +307,18 @@ private KnownSymbols(INamedTypeSymbol? redisAsync, INamedTypeSymbol? batch, INam
                 }
             }
 
+            var blockingHelpers = new List<IMethodSymbol>();
+            foreach (var declaring in new[] { redisAsync, compilation.GetTypeByMetadataName("StackExchange.Redis.IConnectionMultiplexer") })
+            {
+                if (declaring is null) continue;
+                foreach (var member in declaring.GetMembers())
+                {
+                    if (member is IMethodSymbol { Name: "Wait" or "WaitAll" or "TryWait" } helper) blockingHelpers.Add(helper);
+                }
+            }
+
             return new KnownSymbols(
+                blockingHelpers,
                 redisAsync,
                 batch,
                 transactionAsync,
@@ -294,6 +328,39 @@ private KnownSymbols(INamedTypeSymbol? redisAsync, INamedTypeSymbol? batch, INam
                 fireAndForget);
         }
 
+        /// <summary>
+        /// Whether this is one of the library's own blocking helpers - <c>Wait</c>, <c>WaitAll</c>,
+        /// <c>TryWait</c> - on either of the two unrelated interfaces that declare them.
+        /// </summary>
+        /// <remarks>
+        /// Matched through <c>FindImplementationForInterfaceMember</c> as well as directly, so a call on a
+        /// class rather than an interface still counts. That is not a corner case here: ConnectionMultiplexer
+        /// is what Connect returns, so <c>conn.Wait(task)</c> is the common shape and its containing type is
+        /// the class.
+        /// </remarks>
+        public bool IsBlockingHelper(IMethodSymbol method)
+        {
+            if (BlockingHelpers.Count == 0) return false;
+            if (method.Name is not ("Wait" or "WaitAll" or "TryWait")) return false; // cheap reject first
+
+            foreach (var helper in BlockingHelpers)
+            {
+                if (SymbolEqualityComparer.Default.Equals(method.OriginalDefinition, helper)) return true;
+            }
+
+            var containing = method.ContainingType;
+            if (containing is null) return false;
+            foreach (var helper in BlockingHelpers)
+            {
+                if (SymbolEqualityComparer.Default.Equals(containing.FindImplementationForInterfaceMember(helper), method.OriginalDefinition))
+                {
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
         /// <summary>Whether this receiver is any asynchronous redis surface at all.</summary>
         public bool IsRedis(ITypeSymbol? type) => Implements(type, RedisAsync);
 

src/StackExchange.Redis/Availability/MultiGroupDatabase.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Threading;
 using System.Threading.Tasks;
 using StackExchange.Redis.Interfaces;
@@ -81,8 +81,12 @@ public bool IsConnected(RedisKey key, CommandFlags flags = CommandFlags.None)
         => TryGetActiveDatabase()?.IdentifyEndpointAsync(key, flags) ?? MultiGroupMultiplexer.NoEndpoint;
 
     // the Wait family operates on caller-supplied Tasks, not server calls
+    // forwarding is not using: these decorators must implement the interface in full, and the
+    // implementation cannot be dropped while the interface declares it
+    #pragma warning disable SER308 // Blocking on a task through the library's Wait helpers
     public bool TryWait(Task task) => GetActiveDatabase().TryWait(task);
     public void Wait(Task task) => GetActiveDatabase().Wait(task);
     public T Wait<T>(Task<T> task) => GetActiveDatabase().Wait(task);
     public void WaitAll(params Task[] tasks) => GetActiveDatabase().WaitAll(tasks);
+    #pragma warning restore SER308
 }

src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
@@ -1041,11 +1041,15 @@ public event EventHandler<ServerMaintenanceEvent>? ServerMaintenanceEvent
 
             public EndPoint[] GetEndPoints(bool configuredOnly = false) => Active.GetEndPoints(configuredOnly);
 
+            // forwarding is not using: these decorators must implement the interface in full, and the
+            // implementation cannot be dropped while the interface declares it
+            #pragma warning disable SER308 // Blocking on a task through the library's Wait helpers
             public void Wait(Task task) => Active.Wait(task);
 
             public T Wait<T>(Task<T> task) => Active.Wait(task);
 
             public void WaitAll(params Task[] tasks) => Active.WaitAll(tasks);
+            #pragma warning restore SER308
 
             private EventHandler<HashSlotMovedEventArgs>? _hashSlotMoved;
 

src/StackExchange.Redis/Availability/MultiGroupSubscriber.cs

@@ -15,13 +15,17 @@ internal sealed partial class MultiGroupSubscriber(MultiGroupMultiplexer parent,
 
     public IConnectionMultiplexer Multiplexer => parent;
 
+    // forwarding is not using: these decorators must implement the interface in full, and the
+    // implementation cannot be dropped while the interface declares it
+    #pragma warning disable SER308 // Blocking on a task through the library's Wait helpers
     public bool TryWait(Task task) => GetActiveSubscriber().TryWait(task);
 
     public void Wait(Task task) => GetActiveSubscriber().Wait(task);
 
     public T Wait<T>(Task<T> task) => GetActiveSubscriber().Wait(task);
 
     public void WaitAll(params Task[] tasks) => GetActiveSubscriber().WaitAll(tasks);
+    #pragma warning restore SER308
 
     public TimeSpan Ping(CommandFlags flags = CommandFlags.None) => GetActiveSubscriber().Ping(flags);
 

src/StackExchange.Redis/Availability/RetryDatabase.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.Threading;
 using System.Threading.Tasks;
 using StackExchange.Redis.Interfaces;
@@ -135,10 +135,14 @@ private async Task ExecuteAsync<TState>(TState state, AutoDatabaseAsyncOperation
         // (nothing to post-process)
     }
 
+    // forwarding is not using: these decorators must implement the interface in full, and the
+    // implementation cannot be dropped while the interface declares it
+    #pragma warning disable SER308 // Blocking on a task through the library's Wait helpers
     void IRedisAsync.Wait(Task task) => _inner.Wait(task);
     T IRedisAsync.Wait<T>(Task<T> task) => _inner.Wait<T>(task);
     void IRedisAsync.WaitAll(Task[] tasks) => _inner.WaitAll(tasks);
     bool IRedisAsync.TryWait(Task task) => _inner.TryWait(task);
+    #pragma warning restore SER308
 
     // Methods the generator deliberately skips (see AutoDatabaseGenerator.SkipMethod): the Wait
     // family, the synchronous IsConnected probe, and the streaming IEnumerable/IAsyncEnumerable scans

src/StackExchange.Redis/Availability/RetryTransaction.cs

@@ -416,10 +416,14 @@ public void ForwardSuccess()
     // ---- hand-implemented members the generator deliberately skips ----------------------------------
     // (the Wait family, the synchronous IsConnected probe, and the streaming scans). Wait/IsConnected are
     // straight pass-throughs; scans cannot participate in a transaction.
+    // forwarding is not using: these decorators must implement the interface in full, and the
+    // implementation cannot be dropped while the interface declares it
+    #pragma warning disable SER308 // Blocking on a task through the library's Wait helpers
     void IRedisAsync.Wait(Task task) => _source.Wait(task);
     T IRedisAsync.Wait<T>(Task<T> task) => _source.Wait<T>(task);
     void IRedisAsync.WaitAll(Task[] tasks) => _source.WaitAll(tasks);
     bool IRedisAsync.TryWait(Task task) => _source.TryWait(task);
+    #pragma warning restore SER308
 
     bool IDatabaseAsync.IsConnected(RedisKey key, CommandFlags flags) => _source.IsConnected(key, flags);
 

src/StackExchange.Redis/CursorEnumerable.cs

@@ -211,7 +211,11 @@ private bool SlowNextSync()
 
             private protected TResult Wait<TResult>(Task<TResult> pending, Message message)
             {
+                // the synchronous scan surface: this *is* the blocking path, reached only from a caller who asked
+                // for the sync API, and TryWait is what applies the configured timeout to it
+                #pragma warning disable SER308 // Blocking on a task through the library's Wait helpers
                 if (!parent.redis.TryWait(pending)) ThrowTimeout(message);
+                #pragma warning restore SER308
                 return pending.Result;
             }