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/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.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(); + } + } + """); +} 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) {