From cca271614392a7213050b781154c148fe2e82b60 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 16:09:06 +0100 Subject: [PATCH 1/2] Add SER308: the library's own Wait helpers block a thread too Wait, WaitAll and TryWait are what SER307 discourages everywhere else, reached through the API the library itself offers for it: the calling thread is held for the round-trip while the reply needs a thread of its own. Shipping SER307 while providing a blessed way to do the same thing was only ever half a position. An analyzer rule rather than [Obsolete], and the reason is entirely about how it is turned off. [Obsolete] reports CS0618, which every obsoletion from every source shares, so a consumer wanting to silence *this* has to silence *all* of them - including deprecations in their own code and in unrelated packages. That was the review feedback, and it is right. ObsoleteAttribute.DiagnosticId exists for exactly this and would be the natural answer, but it is net5+ and this library still targets netstandard2.0: it would give a granular ID on some target frameworks and CS0618 on others. A hybrid was considered and rejected - the attribute would need #if on the interface *and* on ConnectionMultiplexer's own members, since marking only the interface does not warn through the class, and on modern targets both mechanisms would report at the same location. [Experimental] is granular but would be saying something untrue; these APIs are long-standing, not preview. Recorded in the descriptor as worth revisiting rather than settled: if netstandard2.0 and net4x are ever dropped, the attribute becomes strictly better and this rule can retire in its favour. Two unrelated interfaces declare these - IRedisAsync for database, server and subscriber calls, and IConnectionMultiplexer for its own - so both are gathered. Matching goes through FindImplementationForInterfaceMember as well as directly, because ConnectionMultiplexer is what Connect returns, making `conn.Wait(task)` the common shape with the class as its containing type. Testing one interface would have left half the surface unguarded, which is how the first attempt at this missed the multiplexer entirely. --- docs/SyncOverAsync.md | 34 +++- docs/rules/index.md | 1 + .../AnalyzerReleases.Unshipped.md | 1 + eng/StackExchange.Redis.Build/Diagnostics.cs | 39 +++++ .../QueuedResultAnalyzer.cs | 73 +++++++- .../StackExchange.Redis.Build.Tests/SER308.cs | 156 ++++++++++++++++++ 6 files changed, 292 insertions(+), 12 deletions(-) create mode 100644 tests/StackExchange.Redis.Build.Tests/SER308.cs diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index 672687619..a1b2dd7a2 100644 --- a/docs/SyncOverAsync.md +++ b/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);SER307 +$(NoWarn);SER307;SER308 ``` -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. diff --git a/docs/rules/index.md b/docs/rules/index.md index a9ccfe750..a5f1eb48b 100644 --- a/docs/rules/index.md +++ b/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 diff --git a/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md index 6e1fd21b9..a0403f5aa 100644 --- a/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md +++ b/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 diff --git a/eng/StackExchange.Redis.Build/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs index 962499e44..8a6ebdad7 100644 --- a/eng/StackExchange.Redis.Build/Diagnostics.cs +++ b/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"); + /// + /// Calling the library's own blocking helpers - Wait, WaitAll, TryWait. + /// + /// + /// + /// Same problem as - 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 .Result. Shipping SER307 while the library provided a blessed way to do the same thing + /// was only ever half a position. + /// + /// + /// This is a rule rather than [Obsolete] deliberately, and the reason is about how it is turned + /// off. [Obsolete] reports CS0618, 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. ObsoleteAttribute.DiagnosticId 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 CS0618 on others. An ID of our own behaves the same everywhere, and sits + /// with the rest of the family. [Experimental] was considered and rejected: it is granular, but + /// these APIs are long-standing rather than preview, so it would be saying something untrue. + /// + /// + /// 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 #if 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. + /// + /// + 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"); + /// /// The generated code cannot be compiled at the language version in effect, so nothing was generated. /// diff --git a/eng/StackExchange.Redis.Build/QueuedResultAnalyzer.cs b/eng/StackExchange.Redis.Build/QueuedResultAnalyzer.cs index fff6eae99..3be7af1e8 100644 --- a/eng/StackExchange.Redis.Build/QueuedResultAnalyzer.cs +++ b/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); /// 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 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; } + /// + /// Wait/WaitAll/TryWait, gathered from both declaring interfaces. + /// + /// + /// 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. + /// + private List BlockingHelpers { get; } + /// The root of every async redis surface: IDatabaseAsync, IServer and ISubscriber all derive from it. private INamedTypeSymbol? RedisAsync { get; } @@ -284,7 +307,18 @@ private KnownSymbols(INamedTypeSymbol? redisAsync, INamedTypeSymbol? batch, INam } } + var blockingHelpers = new List(); + 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); } + /// + /// Whether this is one of the library's own blocking helpers - Wait, WaitAll, + /// TryWait - on either of the two unrelated interfaces that declare them. + /// + /// + /// Matched through FindImplementationForInterfaceMember 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 conn.Wait(task) is the common shape and its containing type is + /// the class. + /// + 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; + } + /// Whether this receiver is any asynchronous redis surface at all. public bool IsRedis(ITypeSymbol? type) => Implements(type, RedisAsync); diff --git a/tests/StackExchange.Redis.Build.Tests/SER308.cs b/tests/StackExchange.Redis.Build.Tests/SER308.cs new file mode 100644 index 000000000..5443e73ed --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER308.cs @@ -0,0 +1,156 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// The library's own blocking helpers - Wait, WaitAll, TryWait. +/// +/// +/// A rule rather than [Obsolete] so that silencing it does not mean silencing CS0618, and with +/// it every obsoletion from every source. The two declaring interfaces are unrelated - IRedisAsync and +/// IConnectionMultiplexer - so both are covered here; testing one would have left half the surface +/// unguarded, which is how the first attempt at this missed the multiplexer entirely. +/// +public class SER308 : Verifier +{ + [Fact] + public Task WaitOnDatabase_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public void M(IDatabase db, Task task) + { + {|#0:db.Wait(task)|}; + } + } + """, + Diagnostic("SER308").WithLocation(0).WithArguments("Wait")); + + [Fact] + public Task GenericWaitOnDatabase_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public void M(IDatabase db, Task task) + { + var value = {|#0:db.Wait(task)|}; + } + } + """, + Diagnostic("SER308").WithLocation(0).WithArguments("Wait")); + + [Fact] + public Task TryWait_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public void M(IDatabase db, Task task) + { + var ok = {|#0:db.TryWait(task)|}; + } + } + """, + Diagnostic("SER308").WithLocation(0).WithArguments("TryWait")); + + /// + /// IConnectionMultiplexer declares its own Wait family, unrelated to IRedisAsync's. + /// + [Fact] + public Task WaitAllOnMultiplexer_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public void M(IConnectionMultiplexer conn, Task[] tasks) + { + {|#0:conn.WaitAll(tasks)|}; + } + } + """, + Diagnostic("SER308").WithLocation(0).WithArguments("WaitAll")); + + /// + /// And on the concrete class, which is what Connect returns - so this is the common shape, and the + /// member's containing type is the class rather than the interface. + /// + [Fact] + public Task WaitOnConcreteMultiplexer_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public void M(ConnectionMultiplexer conn, Task task) + { + {|#0:conn.Wait(task)|}; + } + } + """, + Diagnostic("SER308").WithLocation(0).WithArguments("Wait")); + + [Fact] + public Task WaitOnSubscriber_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public void M(ISubscriber sub, Task task) + { + {|#0:sub.Wait(task)|}; + } + } + """, + Diagnostic("SER308").WithLocation(0).WithArguments("Wait")); + + // ---- negative cases ---- + + /// Awaiting is the answer the rule points at, and must not itself be flagged. + [Fact] + public Task Awaited_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + await db.StringGetAsync(key); + } + } + """); + + /// + /// Something else entirely called Wait is not ours; the rule matches the interface members rather + /// than the name, which is the whole reason it can be a warning rather than a guess. + /// + [Fact] + public Task UnrelatedWait_IsClean() => VerifyAsync( + """ + using System.Threading; + using System.Threading.Tasks; + class Waiter + { + public void Wait(Task task) { } + public bool TryWait(Task task) => true; + } + class C + { + public void M(Waiter waiter, Task task, ManualResetEventSlim gate) + { + waiter.Wait(task); + waiter.TryWait(task); + gate.Wait(); + task.Wait(); + } + } + """); +} From 288c56a1522428fa518ad1564be8f723004ed16d Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 16:09:06 +0100 Subject: [PATCH 2/2] Suppress SER308 where this repo blocks deliberately Two kinds, and the distinction is in the comments rather than left to the reader. Most are decorator layers - KeyPrefixed, RedisBase, the Retry* and MultiGroup* wrappers, the test fixture - which must implement the interfaces in full and simply forward; forwarding a flagged member is not using it, and the implementation cannot be dropped while the interface declares it. The exception is CursorEnumerable's synchronous scan path, which is the real thing: it is reached only from a caller who asked for the sync API, and TryWait is what applies the configured timeout to it. Test call sites are suppressed rather than rewritten to await, deliberately: the Wait helpers apply the multiplexer's timeout, which a bare await does not, so rewriting would quietly change what those tests exercise. --- .../Availability/MultiGroupDatabase.cs | 6 +++++- .../Availability/MultiGroupMultiplexer.cs | 6 +++++- .../Availability/MultiGroupSubscriber.cs | 4 ++++ src/StackExchange.Redis/Availability/RetryDatabase.cs | 6 +++++- src/StackExchange.Redis/Availability/RetryTransaction.cs | 4 ++++ src/StackExchange.Redis/CursorEnumerable.cs | 4 ++++ src/StackExchange.Redis/KeyspaceIsolation/KeyPrefixed.cs | 4 ++++ src/StackExchange.Redis/RedisBase.cs | 4 ++++ tests/StackExchange.Redis.Tests/AggressiveTests.cs | 8 ++++++++ tests/StackExchange.Redis.Tests/AsyncTests.cs | 2 ++ tests/StackExchange.Redis.Tests/ClusterTests.cs | 4 ++++ tests/StackExchange.Redis.Tests/HashTests.cs | 6 ++++++ .../Helpers/SharedConnectionFixture.cs | 3 +++ tests/StackExchange.Redis.Tests/Issues/SO10504853Tests.cs | 2 ++ tests/StackExchange.Redis.Tests/PerformanceTests.cs | 2 ++ tests/StackExchange.Redis.Tests/ProfilingTests.cs | 2 ++ tests/StackExchange.Redis.Tests/PubSubTests.cs | 2 ++ tests/StackExchange.Redis.Tests/ScriptingTests.cs | 4 ++++ 18 files changed, 70 insertions(+), 3 deletions(-) diff --git a/src/StackExchange.Redis/Availability/MultiGroupDatabase.cs b/src/StackExchange.Redis/Availability/MultiGroupDatabase.cs index dd51c47c0..72ebc8901 100644 --- a/src/StackExchange.Redis/Availability/MultiGroupDatabase.cs +++ b/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(Task task) => GetActiveDatabase().Wait(task); public void WaitAll(params Task[] tasks) => GetActiveDatabase().WaitAll(tasks); + #pragma warning restore SER308 } diff --git a/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs b/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs index 86cbb6922..0b87ad359 100644 --- a/src/StackExchange.Redis/Availability/MultiGroupMultiplexer.cs +++ b/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 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(Task task) => Active.Wait(task); public void WaitAll(params Task[] tasks) => Active.WaitAll(tasks); + #pragma warning restore SER308 private EventHandler? _hashSlotMoved; diff --git a/src/StackExchange.Redis/Availability/MultiGroupSubscriber.cs b/src/StackExchange.Redis/Availability/MultiGroupSubscriber.cs index 4dbb8e9e3..9f83b145d 100644 --- a/src/StackExchange.Redis/Availability/MultiGroupSubscriber.cs +++ b/src/StackExchange.Redis/Availability/MultiGroupSubscriber.cs @@ -15,6 +15,9 @@ 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); @@ -22,6 +25,7 @@ internal sealed partial class MultiGroupSubscriber(MultiGroupMultiplexer parent, public T Wait(Task 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); diff --git a/src/StackExchange.Redis/Availability/RetryDatabase.cs b/src/StackExchange.Redis/Availability/RetryDatabase.cs index ea037d786..9eab5af67 100644 --- a/src/StackExchange.Redis/Availability/RetryDatabase.cs +++ b/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 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(Task task) => _inner.Wait(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 diff --git a/src/StackExchange.Redis/Availability/RetryTransaction.cs b/src/StackExchange.Redis/Availability/RetryTransaction.cs index 3b1fec6bd..ac0e4cd3e 100644 --- a/src/StackExchange.Redis/Availability/RetryTransaction.cs +++ b/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(Task task) => _source.Wait(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); diff --git a/src/StackExchange.Redis/CursorEnumerable.cs b/src/StackExchange.Redis/CursorEnumerable.cs index 8f9863bb3..bf776bbc1 100644 --- a/src/StackExchange.Redis/CursorEnumerable.cs +++ b/src/StackExchange.Redis/CursorEnumerable.cs @@ -211,7 +211,11 @@ private bool SlowNextSync() private protected TResult Wait(Task 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; } diff --git a/src/StackExchange.Redis/KeyspaceIsolation/KeyPrefixed.cs b/src/StackExchange.Redis/KeyspaceIsolation/KeyPrefixed.cs index c889a2c57..c92f24d5c 100644 --- a/src/StackExchange.Redis/KeyspaceIsolation/KeyPrefixed.cs +++ b/src/StackExchange.Redis/KeyspaceIsolation/KeyPrefixed.cs @@ -885,6 +885,9 @@ public Task KeyTouchAsync(RedisKey key, CommandFlags flags = CommandFlags. Inner.KeyTouchAsync(ToInner(key), flags); public bool TryWait(Task task) => + // 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 Inner.TryWait(task); public TResult Wait(Task task) => @@ -895,6 +898,7 @@ public void Wait(Task task) => public void WaitAll(params Task[] tasks) => Inner.WaitAll(tasks); + #pragma warning restore SER308 protected internal RedisKey ToInner(RedisKey outer) => RedisKey.WithPrefix(Prefix, outer); diff --git a/src/StackExchange.Redis/RedisBase.cs b/src/StackExchange.Redis/RedisBase.cs index 095835efd..84981b71f 100644 --- a/src/StackExchange.Redis/RedisBase.cs +++ b/src/StackExchange.Redis/RedisBase.cs @@ -34,11 +34,15 @@ public virtual Task PingAsync(CommandFlags flags = CommandFlags.None) public bool TryWait(Task task) => task.Wait(multiplexer.TimeoutMilliseconds); + // 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) => multiplexer.Wait(task); public T Wait(Task task) => multiplexer.Wait(task); public void WaitAll(params Task[] tasks) => multiplexer.WaitAll(tasks); + #pragma warning restore SER308 internal virtual Task ExecuteAsync(Message? message, ResultProcessor? processor, T defaultValue, ServerEndPoint? server = null) { diff --git a/tests/StackExchange.Redis.Tests/AggressiveTests.cs b/tests/StackExchange.Redis.Tests/AggressiveTests.cs index f0ba91f16..8c79e5677 100644 --- a/tests/StackExchange.Redis.Tests/AggressiveTests.cs +++ b/tests/StackExchange.Redis.Tests/AggressiveTests.cs @@ -109,7 +109,9 @@ private void BatchRunIntegers(IDatabase db) tasks[j] = batch.StringIncrementAsync(key); } batch.Execute(); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not db.Multiplexer.WaitAll(tasks); + #pragma warning restore SER308 } var count = (long)db.StringGet(key); @@ -127,7 +129,9 @@ private static void BatchRunPings(IDatabase db) tasks[j] = batch.PingAsync(); } batch.Execute(); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not db.Multiplexer.WaitAll(tasks); + #pragma warning restore SER308 } } @@ -228,7 +232,9 @@ private void TranRunIntegers(IDatabase db) tasks[j] = batch.StringIncrementAsync(key); } batch.Execute(); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not db.Multiplexer.WaitAll(tasks); + #pragma warning restore SER308 } var count = (long)db.StringGet(key); @@ -249,7 +255,9 @@ private void TranRunPings(IDatabase db) tasks[j] = batch.PingAsync(); } batch.Execute(); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not db.Multiplexer.WaitAll(tasks); + #pragma warning restore SER308 } } diff --git a/tests/StackExchange.Redis.Tests/AsyncTests.cs b/tests/StackExchange.Redis.Tests/AsyncTests.cs index 1cbce25a3..fbf980add 100644 --- a/tests/StackExchange.Redis.Tests/AsyncTests.cs +++ b/tests/StackExchange.Redis.Tests/AsyncTests.cs @@ -25,8 +25,10 @@ public async Task AsyncTasksReportFailureIfServerUnavailable() var a = db.SetAddAsync(key, "a"); var b = db.SetAddAsync(key, "b"); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not Assert.True(conn.Wait(a)); Assert.True(conn.Wait(b)); + #pragma warning restore SER308 conn.AllowConnect = false; diff --git a/tests/StackExchange.Redis.Tests/ClusterTests.cs b/tests/StackExchange.Redis.Tests/ClusterTests.cs index 5db215731..5f364bd06 100644 --- a/tests/StackExchange.Redis.Tests/ClusterTests.cs +++ b/tests/StackExchange.Redis.Tests/ClusterTests.cs @@ -511,8 +511,10 @@ public async Task TransactionWithSameSlotKeys() Assert.False(setY.IsCanceled, "set y cancelled"); var existsX = cluster.KeyExistsAsync(x); var existsY = cluster.KeyExistsAsync(y); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not Assert.True(cluster.Wait(existsX), "x exists"); Assert.True(cluster.Wait(existsY), "y exists"); + #pragma warning restore SER308 } [Theory] @@ -667,7 +669,9 @@ public async Task AccessRandomKeys() actual[index] = cluster.StringGetAsync(pair.Key); index++; } + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not cluster.WaitAll(actual); + #pragma warning restore SER308 for (int i = 0; i < COUNT; i++) { Assert.Equal(expected[i], actual[i].Result); diff --git a/tests/StackExchange.Redis.Tests/HashTests.cs b/tests/StackExchange.Redis.Tests/HashTests.cs index 9523ca102..9252cb669 100644 --- a/tests/StackExchange.Redis.Tests/HashTests.cs +++ b/tests/StackExchange.Redis.Tests/HashTests.cs @@ -214,8 +214,10 @@ public async Task TestIncrementOnHashThatDoesntExist() var db = conn.GetDatabase(); _ = db.KeyDeleteAsync("keynotexist"); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not var result1 = db.Wait(db.HashIncrementAsync("keynotexist", "fieldnotexist", 1)); var result2 = db.Wait(db.HashIncrementAsync("keynotexist", "anotherfieldnotexist", 1)); + #pragma warning restore SER308 Assert.Equal(1, result1); Assert.Equal(1, result2); } @@ -636,8 +638,10 @@ public async Task TestGetPairs() var result1 = db.HashGetAllAsync(hashkey); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not Assert.Empty(conn.Wait(result0)); var result = conn.Wait(result1).ToStringDictionary(); + #pragma warning restore SER308 Assert.Equal(2, result.Count); Assert.Equal("abc", result["foo"]); Assert.Equal("def", result["bar"]); @@ -664,7 +668,9 @@ public async Task TestSetPairs() }; _ = db.HashSetAsync(hashkey, data).ForAwait(); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not var result1 = db.Wait(db.HashGetAllAsync(hashkey)); + #pragma warning restore SER308 Assert.Empty(result0.Result); var result = result1.ToStringDictionary(); diff --git a/tests/StackExchange.Redis.Tests/Helpers/SharedConnectionFixture.cs b/tests/StackExchange.Redis.Tests/Helpers/SharedConnectionFixture.cs index 27a676a5e..9da583442 100644 --- a/tests/StackExchange.Redis.Tests/Helpers/SharedConnectionFixture.cs +++ b/tests/StackExchange.Redis.Tests/Helpers/SharedConnectionFixture.cs @@ -203,11 +203,14 @@ public event EventHandler ServerMaintenanceEvent public void ResetStormLog() => _inner.ResetStormLog(); + // forwarding is not using: this wrapper must implement IConnectionMultiplexer in full + #pragma warning disable SER308 // Blocking on a task through the library's Wait helpers public void Wait(Task task) => _inner.Wait(task); public T Wait(Task task) => _inner.Wait(task); public void WaitAll(params Task[] tasks) => _inner.WaitAll(tasks); + #pragma warning restore SER308 public void ExportConfiguration(Stream destination, ExportOptions options = ExportOptions.All) => _inner.ExportConfiguration(destination, options); diff --git a/tests/StackExchange.Redis.Tests/Issues/SO10504853Tests.cs b/tests/StackExchange.Redis.Tests/Issues/SO10504853Tests.cs index 7d4276e9d..fce249439 100644 --- a/tests/StackExchange.Redis.Tests/Issues/SO10504853Tests.cs +++ b/tests/StackExchange.Redis.Tests/Issues/SO10504853Tests.cs @@ -72,7 +72,9 @@ await Assert.ThrowsAsync(async () => try { + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not db.Wait(taskResult); + #pragma warning restore SER308 Assert.Fail("Should throw a WRONGTYPE"); } catch (AggregateException ex) diff --git a/tests/StackExchange.Redis.Tests/PerformanceTests.cs b/tests/StackExchange.Redis.Tests/PerformanceTests.cs index b308bf0ac..df1529e4c 100644 --- a/tests/StackExchange.Redis.Tests/PerformanceTests.cs +++ b/tests/StackExchange.Redis.Tests/PerformanceTests.cs @@ -40,7 +40,9 @@ public async Task VerifyPerformanceImprovement() var final = new Task[5]; for (int db = 0; db < 5; db++) final[db] = conn.GetDatabase(db).StringGetAsync(key); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not conn.WaitAll(final); + #pragma warning restore SER308 timer.Stop(); asyncTimer = (int)timer.ElapsedMilliseconds; Log("async to completion (local): {0}ms", timer.ElapsedMilliseconds); diff --git a/tests/StackExchange.Redis.Tests/ProfilingTests.cs b/tests/StackExchange.Redis.Tests/ProfilingTests.cs index 2ce18684d..65bb09bde 100644 --- a/tests/StackExchange.Redis.Tests/ProfilingTests.cs +++ b/tests/StackExchange.Redis.Tests/ProfilingTests.cs @@ -248,7 +248,9 @@ public async Task LowAllocationEnumerable() allTasks.Add(finalResult); } + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not conn.WaitAll(allTasks.ToArray()); + #pragma warning restore SER308 var res = session.FinishProfiling(); Assert.True(res.GetType().IsValueType); diff --git a/tests/StackExchange.Redis.Tests/PubSubTests.cs b/tests/StackExchange.Redis.Tests/PubSubTests.cs index 6d5219908..7f6935e7f 100644 --- a/tests/StackExchange.Redis.Tests/PubSubTests.cs +++ b/tests/StackExchange.Redis.Tests/PubSubTests.cs @@ -371,7 +371,9 @@ private void TestMassivePublish(ISubscriber sub, string channel, string caption) tasks[i] = sub.PublishAsync(channel, "bar"); #pragma warning restore CS0618 } + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not sub.WaitAll(tasks); + #pragma warning restore SER308 withAsync.Stop(); Log($"{caption}: {withFAF.ElapsedMilliseconds}ms (F+F) vs {withAsync.ElapsedMilliseconds}ms (async)"); diff --git a/tests/StackExchange.Redis.Tests/ScriptingTests.cs b/tests/StackExchange.Redis.Tests/ScriptingTests.cs index 6f6ad319b..f203ff521 100644 --- a/tests/StackExchange.Redis.Tests/ScriptingTests.cs +++ b/tests/StackExchange.Redis.Tests/ScriptingTests.cs @@ -290,7 +290,9 @@ public async Task ScriptThrowsErrorInsideTransaction() var c = tran.StringIncrementAsync(key); var complete = tran.ExecuteAsync(); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not Assert.True(conn.Wait(complete)); + #pragma warning restore SER308 Assert.True(QuickWait(a).IsCompleted, a.Status.ToString()); Assert.True(QuickWait(c).IsCompleted, "State: " + c.Status); Assert.Equal(1L, a.Result); @@ -305,7 +307,9 @@ public async Task ScriptThrowsErrorInsideTransaction() Assert.Contains(ex.Message, new[] { "ERR oops", "oops" }); } var afterTran = db.StringGetAsync(key); + #pragma warning disable SER308 // deliberate: test code blocking on a task, and the Wait helpers apply the configured timeout that a bare await would not Assert.Equal(2L, (long)db.Wait(afterTran)); + #pragma warning restore SER308 } private static Task QuickWait(Task task) {