diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md new file mode 100644 index 000000000..672687619 --- /dev/null +++ b/docs/SyncOverAsync.md @@ -0,0 +1,191 @@ +# 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. + +## The short version + +Calling `.Result`, `.Wait()` or `.GetAwaiter().GetResult()` on an `async` method is *sync over async*. It looks +like a small convenience. What it actually does is hold one thread hostage while waiting for a reply — and +processing that reply **also needs a thread**. Do this enough times concurrently and the pool runs out of +threads to process replies with, so nothing completes, so nothing releases a thread. The client is not slow; +it is stuck. + +```csharp +// the problem +var value = db.StringGetAsync(key).Result; + +// the fix +var value = await db.StringGetAsync(key); +``` + +There is no second option. In particular, switching to the synchronous API is **not** a fix — see below. + +## Why it fails so badly, rather than just being slow + +A multiplexed client makes this worse than the general case, because the thing you are waiting for needs the +same resource you are consuming while you wait. + +1. Your code calls `.Result` on a redis command, and the calling thread blocks. +2. Redis replies. The bytes arrive at the socket promptly — this part is almost never the problem. +3. To turn those bytes into a completed `Task`, the client needs a thread. +4. If every thread is blocked at step 1, step 3 cannot happen. +5. Nothing completes, so no thread from step 1 is ever released. + +The result is timeouts that look like a network or server problem and are neither. A characteristic sign is a +timeout message reporting **data already sitting in the socket** — the reply is *there*, it simply cannot be +processed. + +## Why adding threads does not rescue you + +The .NET thread-pool creates threads on demand up to its **minimum**, then throttles hard: beyond that point +it adds threads slowly and deliberately, on the order of one or two per second, using a hill-climbing +heuristic that is trying to find a *throughput* optimum. That heuristic assumes threads are working. Here they +are blocked, so more threads simply means more blocked threads. + +This is why raising `MinThreads` is not a fix: + +- it moves the wall further away rather than removing it, and under load you will arrive at it anyway; +- the pile of blocked threads grows with it, so the failure is bigger when it comes; +- and the underlying call pattern is unchanged. + +Raising the minimum can be a reasonable *stopgap* while you fix the call sites, and it does help a genuine +burst of short-lived work. It does not help an application that is systematically blocking on I/O. + +## Confirming it is this + +Look at a timeout message from the client — see [Timeouts](Timeouts) for how to read one in full. The +combination that points here is: + +- `Busy` at or above `Min` for `WORKER` or `IOCP`, meaning the pool is in its throttled, slow-growth regime; +- bytes waiting unread on the connection, meaning the server already answered; +- and timeouts that get *worse* under load rather than better as caches warm. + +A message showing all of it at once looks something like: + + Timeout performing GET MyKey (5000ms), inst: 0, qs: 84, in: 487312, + IOCP: (Busy=0,Free=1000,Min=8,Max=1000), + WORKER: (Busy=73,Free=32694,Min=64,Max=32767), + POOL: (Threads=73,QueuedItems=612,CompletedItems=418327,Timers=14) + +Read that as: 84 commands are awaiting replies (`qs`); **476KiB has already arrived and is sitting unread** +(`in`); the worker pool has more busy threads than its minimum, so it is now injecting new ones a couple per +second at most; and 612 work items are queued behind them. + +The `in` figure is the tell. The server has answered and the bytes are in the buffer, so nothing is wrong with +the network, the server, or the connection — there is simply no thread available to pick the reply up. +`QueuedItems` says the same thing from the other direction: work is arriving faster than the pool can start it. + +(`POOL` is only reported on .NET 5 and above; on .NET Framework it reads `n/a`.) + +If the pool is healthy and you still see timeouts, this is not your problem — look at +[Timeouts](Timeouts) and [Thread Theft](ThreadTheft) instead. + +## Fixing it + +In order of preference: + +1. **Make the call path async.** `await` the call, all the way up. This is the only change that removes the + problem rather than accommodating it, and it is worth pushing further up the call stack than feels + convenient — a single blocking frame anywhere in the path reintroduces it for everything below. +2. **Take the client off the thread-pool**, as a mitigation while you do 1 (see below). + +### The synchronous API is not an escape hatch + +StackExchange.Redis does have a synchronous API, and `db.StringGet(key)` is not *literally* sync-over-async: +it does not wrap an asynchronous call and block on the result. That distinction is about mechanism, and it +buys you nothing here, because the consequence is identical. + +The calling thread still blocks until redis replies, and processing that reply still needs a thread. If the +calling thread came from the thread-pool — as it does under ASP.NET, in hosted services, in timer callbacks, +in continuations, in almost anything you did not start yourself — then you are still occupying exactly the +resource the reply needs in order to arrive. The pool starves the same way, for the same reason. + +So the synchronous API is not a safe way to keep blocking, and this page is not telling you to reach for it. +If a caller genuinely cannot be made async, the honest position is that you are trading against the pool and +should size and isolate accordingly — not that you have avoided the problem. + +### Mitigation: give the client its own threads + +```csharp +ConnectionMultiplexer.SetFeatureFlag("DedicatedThreads", true); // early in application startup +``` + +This makes the library read and write on threads it owns rather than borrowing the thread-pool, so redis +traffic keeps flowing even while the pool is saturated. + +Be clear about what this does and does not do. It **does not fix the thread-pool** — nothing in this library +can, because the blocked threads are in your code. What it does is stop redis from being caught in the jam, +which usually converts "everything times out" into "the application is slow, and one part of it is obviously +blocking". That is a much better place to debug from, and for many applications it is enough to restore +service while the real fix is made. It is not a licence to keep the blocking calls. + +Two caveats worth knowing before you enable it: + +- it costs a reader and a writer thread **for each node you connect to** (not RESP2 pub/sub connections, which + stay on the thread-pool; RESP3 does not use separate pub/sub connections), so think about it before enabling + it against a very wide cluster, where that scales with the number of shards; +- it is deliberately opt-in, and set process-wide at startup rather than per-connection. + +Neither is meant to be permanent, and the first one especially. Work is in progress on dedicated readers built +over the platform's native completion machinery — `io_uring` on Linux, IOCP on Windows — which would service +many connections from a small fixed set of threads rather than a pair per connection. That is the thing that +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". + +## Fire-and-forget is a special case + +`CommandFlags.FireAndForget` returns an already-completed task carrying the default value, so blocking on one +does not wait for anything and cannot starve the pool. It is still not doing what it looks like: the value is +fixed before the call returns and is never the server's answer. Discard it, or drop the fire-and-forget flag +if you actually want a result — see `SER306`. + +## The analyzer + +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. +- **`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 + +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: + +For a single call site you have decided about — a legacy entry point, an interface you do not control: + +```c# +#pragma warning disable SER307 // blocking: called from +``` + +For a project, while you work through it: + +```xml +$(NoWarn);SER307 +``` + +Or turn it down rather than off, so it stays visible in the IDE without failing builds — in `.editorconfig`: + +```ini +dotnet_diagnostic.SER307.severity = suggestion # or none, silent, warning, error +``` + +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. + +## See also + +- [Timeouts](Timeouts) — reading a timeout message, and the thread-pool statistics in it +- [Thread Theft](ThreadTheft) — a different problem with a similar smell, where the reader thread is hijacked + by continuations rather than starved of threads. Largely historical: the client asks for + `RunContinuationsAsynchronously` by default, so on modern .NET this is handled for you — the mitigation + there exists for hosts whose synchronization context cannot be trusted to honour it, which in practice + means classic ASP.NET on .NET Framework +- [Pipelines and multiplexers](PipelinesMultiplexers) — why the client is shaped this way in the first place diff --git a/docs/ThreadTheft.md b/docs/ThreadTheft.md index d5b8e717e..a3fdf692e 100644 --- a/docs/ThreadTheft.md +++ b/docs/ThreadTheft.md @@ -1,4 +1,11 @@ -# Thread Theft +# Thread Theft + +> **Check this first: are you on .NET Framework?** Everything below needs a `SynchronizationContext` whose +> `Post` runs the callback *synchronously*, and in practice that means classic ASP.NET. .NET Core and .NET 5+ +> do not install a `SynchronizationContext` for server workloads at all, so the default protection described +> below is sufficient there and the flag will not help you. If you have timeouts on a modern runtime, start +> with [Sync over async](SyncOverAsync) and [Timeouts](Timeouts) instead - the symptoms overlap, but the +> causes do not. If you're here because you followed a link in an exception and you just want your code to work, the short version is: try adding the following *early on* in your application startup: @@ -66,3 +73,10 @@ both appropriate and necessary, and does not represent additional overhead. The library will attempt to detect `LegacyAspNetSynchronizationContext` in particular, but this is not always reliable. The flag is also available for manual use with other similar scenarios. + +## If that was not it + +The distinctive marker for thread theft is `rs: CompletePendingMessage` showing up often in timeout messages - +the reader caught running application logic. If you are seeing timeouts *without* that, the reader is more +likely to be starved than stolen: it cannot get a thread at all, rather than being hijacked once it has one. +That is [Sync over async](SyncOverAsync), and it applies on every runtime. diff --git a/docs/Timeouts.md b/docs/Timeouts.md index ea9830041..0aaf68ed4 100644 --- a/docs/Timeouts.md +++ b/docs/Timeouts.md @@ -19,13 +19,16 @@ The parameter “`qs`” in the error message tells you how many requests were s Are you seeing a high number of busyio or busyworker threads in the timeout exception? --------------- -Asynchronous operations in StackExchange.Redis can come back in 3 different ways: +Asynchronous operations in StackExchange.Redis can come back in 2 different ways: - IOCP threads are used when asynchronous IO happens (e.g. reading from the network). -- From 2.0 onwards, StackExchange.Redis maintains a dedicated thread-pool that it uses for completing most `async` operations; the error message may include an indication of how many of these workers are currently available - if this is zero, it may suggest that your system is particularly busy with asynchronous operations. -- .NET also has a global thread-pool; if the dedicated thread-pool is failing to keep up, additional work will be offered to the global thread-pool, so the message may include details of the global thread-pool. +- .NET's global thread-pool runs the continuations, so the message includes its statistics. -The StackExchange.Redis dedicated thread-pool has a fixed size suitable for many common scenarios, which is shared between multiple connection instances (this can be customized by explicitly providing a `SocketManager` when creating a `ConnectionMultiplexer`). In many scenarios when using 2.0 and above, the vast majority of asynchronous operations will be serviced by this dedicated pool. This pool exists to avoid contention, as we've frequently seen cases where the global thread-pool becomes jammed with threads that need redis results to unblock them. +2.x maintained a dedicated thread-pool of its own for completing `async` operations, reported as `mgr` and configurable via `SocketManager`. **That is gone as of 3.0** - `ConfigurationOptions.SocketManager` is obsolete and unused - so neither appears any more, and the global thread-pool statistics are what matter. If you need redis serviced off the global pool, see [Sync over async](SyncOverAsync#mitigation-give-the-client-its-own-threads). + +If the global thread-pool is saturated, the most common cause by far is blocking on asynchronous calls +somewhere in the application - see [Sync over async](SyncOverAsync), which covers how to confirm that, why +raising the minimum thread count does not fix it, and how to keep redis traffic flowing meanwhile. .NET itself provides new global thread pool worker threads or I/O completion threads on demand (without any throttling) until it reaches the "Minimum" setting for each type of thread. By default, the minimum number of threads is set to the number of processors on a system. @@ -33,26 +36,18 @@ For these .NET-provided global thread pools: once the number of existing (busy) 1. An existing thread becomes free to process the work 2. No existing thread becomes free for 500ms, so a new thread is created. -Basically, *if* you're hitting the global thread pool (rather than the dedicated StackExchange.Redis thread-pool) it means that when the number of Busy threads is greater than Min threads, you are likely paying a 500ms delay before network traffic is processed by the application. Also, it is important to note that when an existing thread stays idle for longer than 15 seconds (based on what I remember), it will be cleaned up and this cycle of growth and shrinkage can repeat. - -If we look at an example error message from StackExchange.Redis 2.0, you will see that it now prints ThreadPool statistics (see IOCP and WORKER details below). - - Timeout performing GET MyKey (1000ms), inst: 2, qs: 6, in: 0, mgr: 9 of 10 available, - IOCP: (Busy=6,Free=994,Min=4,Max=1000), - WORKER: (Busy=3,Free=997,Min=4,Max=1000) - -In the above example, there are 6 operations currently awaiting replies from redis ("`qs`"), there are 0 bytes waiting to be read from the input stream from redis ("`in`"), and the dedicated thread-pool is almost fully available to service asynchronous completions ("`mgr`"). You can also see that for IOCP thread there are 6 busy threads and the system is configured to allow 4 minimum threads. +Basically, when the number of Busy threads is greater than Min threads, you are likely paying a 500ms delay before network traffic is processed by the application. Also, it is important to note that when an existing thread stays idle for longer than 15 seconds (based on what I remember), it will be cleaned up and this cycle of growth and shrinkage can repeat. -In 1.*, the information is similar but slightly different: +An error message carries ThreadPool statistics alongside the connection state (see IOCP, WORKER and POOL below). - System.TimeoutException: Timeout performing GET MyKey, inst: 2, mgr: Inactive, - queue: 6, qu: 0, qs: 6, qc: 0, wr: 0, wq: 0, in: 0, ar: 0, + Timeout performing GET MyKey (1000ms), inst: 2, qs: 6, in: 0, IOCP: (Busy=6,Free=994,Min=4,Max=1000), - WORKER: (Busy=3,Free=997,Min=4,Max=1000) + WORKER: (Busy=3,Free=997,Min=4,Max=1000), + POOL: (Threads=8,QueuedItems=0,CompletedItems=42,Timers=10) -It may seem contradictory that there are *less* numbers in 2.0 - this is because the 2.0 code has been redesigned not to require some additional steps. +In the above example, there are 6 operations currently awaiting replies from redis ("`qs`") and 0 bytes waiting to be read from the input stream from redis ("`in`"). You can also see that for IOCP thread there are 6 busy threads and the system is configured to allow 4 minimum threads. (`POOL` is reported on .NET 5 and above; elsewhere it reads `n/a`.) -Note that StackExchange.Redis can hit timeouts if either the IOCP threads or the worker threads (.NET global thread-pool, or the dedicated thread-pool) become saturated without the ability to grow. +Note that StackExchange.Redis can hit timeouts if either the IOCP threads or the worker threads become saturated without the ability to grow. Also note that the IOCP and WORKER threads will not be shown on .NET Core if using `netstandard` < 2.0. @@ -93,7 +88,6 @@ By default Redis Timeout exception(s) includes useful information, which can hel |in | Inbound-Bytes : {long}|there are x bytes waiting to be read from the input stream from redis| |in-pipe | Inbound-Pipe-Bytes: {long}|Bytes waiting to be read| |out-pipe| Outbound-Pipe-Bytes: {long}|Bytes waiting to be sent| -|mgr | 8 of 10 available|Redis Internal Dedicated Thread Pool State| |IOCP | IOCP: (Busy=0,Free=500,Min=248,Max=500)| Runtime Global Thread Pool IO Threads. | |WORKER | WORKER: (Busy=170,Free=330,Min=248,Max=500)| Runtime Global Thread Pool Worker Threads.| |POOL | POOL: (Threads=8,QueuedItems=0,CompletedItems=42,Timers=10)| Thread Pool Work Item Stats.| diff --git a/docs/index.md b/docs/index.md index d8aea9a54..36c032c57 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1,4 +1,4 @@ -StackExchange.Redis +StackExchange.Redis =================== - [Release Notes](ReleaseNotes) @@ -57,6 +57,7 @@ Documentation - [Testing](Testing) - running the `StackExchange.Redis.Tests` suite to validate changes - [Timeouts](Timeouts) - guidance on dealing with timeout problems - [Thread Theft](ThreadTheft) - guidance on avoiding TPL threading problems +- [Sync over async](SyncOverAsync) - why blocking on an async redis call starves the thread-pool, and what to do - [RESP Logging](RespLogging) - capturing and validating RESP streams - [Analyzer rules](rules/) - the `SER3xx` suggestions reported by the analyzer shipped in the package diff --git a/docs/rules/index.md b/docs/rules/index.md index 452ada41b..a9ccfe750 100644 --- a/docs/rules/index.md +++ b/docs/rules/index.md @@ -1,4 +1,4 @@ -# Analyzer rules +# Analyzer rules StackExchange.Redis ships a Roslyn analyzer inside the package, so these rules are reported in your own build with no extra reference. Each diagnostic links here from its message. @@ -34,6 +34,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") ## Usage @@ -78,9 +79,9 @@ rare, not impossible. If one of these rules flags something it should not have, than something to work around: please [report it](https://github.com/StackExchange/StackExchange.Redis/issues/new) with the transaction as written. -Three rules are not in this family, because they are not suggesting an improvement to working code: `SER350` -reports a build problem, and [SER305](SER305)/[SER306](SER306) report code that does not work. Those two have -their own, much narrower quiet-lists on their own pages - they are flow-insensitive by design, so most of the +Four rules are not in this family, because they are not suggesting an improvement to working code: `SER350` +reports a build problem, and [SER305](SER305)/[SER306](SER306)/[SER307](../SyncOverAsync) report code that does +not do what it looks like. Those have their own, much narrower quiet-lists on their own pages - they are flow-insensitive by design, so most of the caveats above (a loop, an `if`, a transaction passed elsewhere) simply do not arise. ## Declaring your server version diff --git a/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md index abbe418de..6e1fd21b9 100644 --- a/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md @@ -1,4 +1,4 @@ -; Unshipped analyzer release +; Unshipped analyzer release ; Tracks the diagnostics reported by the analyzers/generators shipped inside the StackExchange.Redis package. ; This is the analyzer equivalent of PublicAPI.Unshipped.txt: a diagnostic ID is a public contract once ; released, because consumers put them in NoWarn and .editorconfig. See Diagnostics.cs for the SER3xx map. @@ -13,3 +13,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 diff --git a/eng/StackExchange.Redis.Build/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs index b627fa4ae..2454253b0 100644 --- a/eng/StackExchange.Redis.Build/Diagnostics.cs +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -1,4 +1,4 @@ -using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis; namespace StackExchange.Redis.Build; @@ -212,13 +212,46 @@ internal static class Diagnostics public static readonly DiagnosticDescriptor AwaitFireAndForgetResult = new( id: "SER306", title: "Waiting for a fire-and-forget result yields nothing", - messageFormat: "'{0}' is queued on {1} as fire-and-forget, so its result is the default value whenever it is read, not the server's answer; discard it instead", + messageFormat: "'{0}' is fire-and-forget, so its result is the default value whenever it is read, not the server's answer; discard it, or drop the fire-and-forget flag if you want the result", category: UsageCategory, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, description: "A fire-and-forget command completes immediately with the default value and never carries a result from the server, so awaiting or blocking on it reads nothing meaningful.", helpLinkUri: HelpLink("SER306")); + /// + /// Blocking a thread on a redis call instead of awaiting it - "sync over async". + /// + /// + /// + /// The failure this leads to is systemic rather than local, which is why the rule is worth having: the + /// blocked thread is holding a thread-pool thread hostage while it waits for a reply that *also* needs a + /// thread-pool thread to be processed. Enough of those and the pool cannot make progress at all, and the + /// symptom - timeouts, with data sitting unread in the socket - looks nothing like its cause. + /// + /// + /// The help link goes to the explainer rather than to a rule page, deliberately: what people need here is + /// the thread-pool story, not a description of the squiggle. Fire-and-forget is excluded, since its task is + /// already complete and blocking on it does not wait for anything - that is + /// 's business instead. + /// + /// + /// The message says "await it instead" and stops there, deliberately. The synchronous API is *not* the + /// answer: it is not literally sync-over-async, but a blocked thread is a blocked thread, and if the caller + /// is on the thread-pool - which it is in ASP.NET and anything else pool-driven - the starvation is + /// identical. An analyzer that pointed people at it would be sending them somewhere just as bad. + /// + /// + public static readonly DiagnosticDescriptor BlockingOnRedisCall = new( + id: "SER307", + title: "Blocking on a redis call instead of awaiting it", + messageFormat: "'{0}' is being waited on synchronously, which ties up a thread until redis replies - and the reply needs a thread of its own to be processed; await it instead", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + 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://stackexchange.github.io/StackExchange.Redis/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 cd5f38aed..fff6eae99 100644 --- a/eng/StackExchange.Redis.Build/QueuedResultAnalyzer.cs +++ b/eng/StackExchange.Redis.Build/QueuedResultAnalyzer.cs @@ -1,4 +1,4 @@ -using System.Collections.Immutable; +using System.Collections.Immutable; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Operations; @@ -6,15 +6,22 @@ namespace StackExchange.Redis.Build; /// -/// Spots code that waits for the result of a command queued on an ITransaction or IBatch at the -/// point of queueing, before Execute[Async] has sent anything (SER305, SER306). +/// Spots the ways of consuming the task from a redis call that do not do what they look like: waiting on a +/// queued command before it has been sent (SER305), reading a fire-and-forget result (SER306), and blocking a +/// thread rather than awaiting (SER307). /// /// /// -/// This is the mistake that "always await your tasks" teaches people to make, and it does not fail loudly: the -/// task simply never completes, so the caller hangs. Unlike the suggestion rules in -/// , this one is reported as an error - see -/// for why that is safe here and is not a hedge being dropped. +/// SER305 is the mistake that "always await your tasks" teaches people to make, and it does not fail loudly: +/// the task simply never completes, so the caller hangs. Unlike the suggestion rules in +/// , it is reported as an error - see +/// for why that is safe and is not a hedge being dropped. +/// +/// +/// The three rules share one traversal because they share one question - what is being done with the task - +/// and differ only in the answer. The receiver decides which world we are in: on a transaction or batch +/// nothing has been sent yet, so *any* wait is the problem; elsewhere the call is already in flight, and only +/// blocking is. Fire-and-forget cuts across both, because there the value is settled before the call returns. /// /// /// The shapes covered are the ones that are certain without any flow analysis, because the wait is written at @@ -38,7 +45,8 @@ public sealed class QueuedResultAnalyzer : DiagnosticAnalyzer public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create( Diagnostics.AwaitBeforeExecute, - Diagnostics.AwaitFireAndForgetResult); + Diagnostics.AwaitFireAndForgetResult, + Diagnostics.BlockingOnRedisCall); /// public override void Initialize(AnalysisContext context) @@ -60,34 +68,53 @@ public override void Initialize(AnalysisContext context) private static void Analyze(OperationAnalysisContext context, KnownSymbols known) { - // what is being waited for, if this operation waits for anything at all + // 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; + if (known.IsTerminator(call.TargetMethod)) return; - // ...and is that a command queued on a transaction or batch? - if (waited is not IInvocationOperation queued) return; - if (!known.IsQueueing(queued.Instance?.Type)) return; - if (known.IsTerminator(queued.TargetMethod)) return; - - // The command flags decide which of the two rules applies, and an unknown value picks neither: a - // non-constant argument may or may not carry FireAndForget at run-time, and we would be guessing in - // both directions. Guessing wrong on SER305 means an error on legal code, so silence is the answer. - var flags = FireAndForget(queued, known); - if (flags == FireAndForgetState.Unknown) return; + var flags = FireAndForget(call, known); + var queued = known.IsQueueing(call.Instance?.Type); - var descriptor = flags == FireAndForgetState.Set - ? Diagnostics.AwaitFireAndForgetResult - : Diagnostics.AwaitBeforeExecute; + var descriptor = (queued, flags, waited.Blocking) switch + { + // Queued on a transaction or batch: nothing has been sent, so the task cannot complete and this + // waits forever - unless fire-and-forget already completed it. An unknown flags value picks + // neither, because guessing wrong on SER305 is a build error on legal code. + (true, FireAndForgetState.Set, _) => Diagnostics.AwaitFireAndForgetResult, + (true, FireAndForgetState.Clear, _) => Diagnostics.AwaitBeforeExecute, + (true, _, _) => null, + + // An ordinary redis call, already in flight: only *blocking* is a problem, and awaiting is exactly + // what you should be doing. Blocking on fire-and-forget is its own case - it waits for nothing, + // since the task was complete before the call returned - so it gets SER306's advice, not SER307's. + // + // Note the await of a fire-and-forget result is deliberately NOT reported here, though it is when + // queued. `await db.KeyDeleteAsync(key, FireAndForget);` is a no-op await rather than a mistake, + // and this repo alone contains 416 of them: a rule that noisy would be turned off wholesale, and + // would take SER305 with it. + (false, FireAndForgetState.Set, true) => Diagnostics.AwaitFireAndForgetResult, + (false, _, true) => Diagnostics.BlockingOnRedisCall, + (false, _, false) => null, + }; + + if (descriptor is null) return; // Reported on the whole wait - "await tran.StringGetAsync(key)" - because that expression is the thing // that is wrong; the call inside it is fine. The call is carried as an additional location so the code // fix can find it without re-deriving the unwrapping below in a second assembly. + // only SER305 names the receiver ("queued on a transaction"); the other two are about the call itself, + // and passing them a description they do not use would put "a batch" behind a plain database + var args = ReferenceEquals(descriptor, Diagnostics.AwaitBeforeExecute) + ? new object[] { call.TargetMethod.Name, Describe(call.Instance?.Type, known) } + : new object[] { call.TargetMethod.Name }; + context.ReportDiagnostic(Diagnostic.Create( descriptor, context.Operation.Syntax.GetLocation(), - additionalLocations: new[] { queued.Syntax.GetLocation() }, + additionalLocations: new[] { call.Syntax.GetLocation() }, properties: null, - queued.TargetMethod.Name, - Describe(queued.Instance?.Type, known))); + args)); } /// @@ -96,24 +123,28 @@ private static void Analyze(OperationAnalysisContext context, KnownSymbols known /// /// The three await-pattern members are matched by name rather than by symbol, which is what the language /// itself does for GetAwaiter/GetResult - and is safe regardless, because the caller still - /// requires the unwrapped operation to be a queued command before anything is reported. + /// requires the unwrapped operation to be a redis call before anything is reported. + /// + /// Blocking is the distinction that matters away from transactions: awaiting is correct usage of an + /// async API, while blocking on it holds a thread for the round-trip (SER307). + /// /// - private static IOperation? Waited(IOperation operation, KnownSymbols known) => operation switch + private static (IInvocationOperation? Call, bool Blocking)? Waited(IOperation operation, KnownSymbols known) => operation switch { // await tran.StringGetAsync(key) - IAwaitOperation await => Unwrap(await.Operation), + IAwaitOperation await => (Unwrap(await.Operation) as IInvocationOperation, false), - // tran.StringGetAsync(key).Result + // db.StringGetAsync(key).Result IPropertyReferenceOperation { Property.Name: "Result", Instance: { } instance } when known.IsTask(instance.Type) - => Unwrap(instance), + => (Unwrap(instance) as IInvocationOperation, true), - // tran.StringGetAsync(key).Wait() + // db.StringGetAsync(key).Wait() IInvocationOperation { TargetMethod.Name: "Wait", Instance: { } instance } when known.IsTask(instance.Type) - => Unwrap(instance), + => (Unwrap(instance) as IInvocationOperation, true), - // tran.StringGetAsync(key).GetAwaiter().GetResult() + // db.StringGetAsync(key).GetAwaiter().GetResult() IInvocationOperation { TargetMethod.Name: "GetResult", Instance: IInvocationOperation { TargetMethod.Name: "GetAwaiter", Instance: { } instance } } - => Unwrap(instance), + => (Unwrap(instance) as IInvocationOperation, true), _ => null, }; @@ -204,8 +235,9 @@ private static string Describe(ITypeSymbol? type, KnownSymbols known) private sealed class KnownSymbols { - private KnownSymbols(INamedTypeSymbol? batch, INamedTypeSymbol? transactionAsync, INamedTypeSymbol? transaction, INamedTypeSymbol? commandFlags, INamedTypeSymbol? task, int? fireAndForgetValue) + private KnownSymbols(INamedTypeSymbol? redisAsync, INamedTypeSymbol? batch, INamedTypeSymbol? transactionAsync, INamedTypeSymbol? transaction, INamedTypeSymbol? commandFlags, INamedTypeSymbol? task, int? fireAndForgetValue) { + RedisAsync = redisAsync; Batch = batch; TransactionAsync = transactionAsync; Transaction = transaction; @@ -214,6 +246,9 @@ private KnownSymbols(INamedTypeSymbol? batch, INamedTypeSymbol? transactionAsync FireAndForgetValue = fireAndForgetValue; } + /// The root of every async redis surface: IDatabaseAsync, IServer and ISubscriber all derive from it. + private INamedTypeSymbol? RedisAsync { get; } + private INamedTypeSymbol? Batch { get; } private INamedTypeSymbol? TransactionAsync { get; } private INamedTypeSymbol? Transaction { get; } @@ -230,9 +265,10 @@ private KnownSymbols(INamedTypeSymbol? batch, INamedTypeSymbol? transactionAsync // IBatch and ITransactionAsync are the two roots of the queueing surface, and neither derives from // the other: ITransaction implements both, while IBatch is reachable on its own from CreateBatch. // No queueing type at all => not our library, or not a version with one; either way, nothing here. + var redisAsync = compilation.GetTypeByMetadataName("StackExchange.Redis.IRedisAsync"); var batch = compilation.GetTypeByMetadataName("StackExchange.Redis.IBatch"); var transactionAsync = compilation.GetTypeByMetadataName("StackExchange.Redis.ITransactionAsync"); - if (batch is null && transactionAsync is null) return null; + if (redisAsync is null && batch is null && transactionAsync is null) return null; var commandFlags = compilation.GetTypeByMetadataName("StackExchange.Redis.CommandFlags"); int? fireAndForget = null; @@ -249,6 +285,7 @@ private KnownSymbols(INamedTypeSymbol? batch, INamedTypeSymbol? transactionAsync } return new KnownSymbols( + redisAsync, batch, transactionAsync, compilation.GetTypeByMetadataName("StackExchange.Redis.ITransaction"), @@ -257,6 +294,9 @@ private KnownSymbols(INamedTypeSymbol? batch, INamedTypeSymbol? transactionAsync fireAndForget); } + /// Whether this receiver is any asynchronous redis surface at all. + public bool IsRedis(ITypeSymbol? type) => Implements(type, RedisAsync); + /// Whether commands on this receiver are queued rather than sent. public bool IsQueueing(ITypeSymbol? type) => Implements(type, Batch) || Implements(type, TransactionAsync); diff --git a/src/StackExchange.Redis/Availability/RetryTransaction.cs b/src/StackExchange.Redis/Availability/RetryTransaction.cs index cd45ab57e..3b1fec6bd 100644 --- a/src/StackExchange.Redis/Availability/RetryTransaction.cs +++ b/src/StackExchange.Redis/Availability/RetryTransaction.cs @@ -282,7 +282,9 @@ public void ForwardSuccess() // for correctness - there is no point registering one to settle a proxy that does not exist if (_proxy is null) return; - var attempt = _attempt!; + // ForwardSuccess only runs after Replay, so this cannot be null; `?? throw` says that to the + // compiler as well as the reader, and keeps the failure loud if the order ever changes + var attempt = _attempt ?? throw new InvalidOperationException("Cannot forward a result before the operation has been replayed"); if (attempt.IsCompleted) { Forward(attempt); @@ -346,7 +348,9 @@ public void ForwardSuccess() { if (_proxy is null) return; // fire-and-forget: see RecordedOp.ForwardSuccess - var attempt = _attempt!; + // ForwardSuccess only runs after Replay, so this cannot be null; `?? throw` says that to the + // compiler as well as the reader, and keeps the failure loud if the order ever changes + var attempt = _attempt ?? throw new InvalidOperationException("Cannot forward a result before the operation has been replayed"); if (attempt.IsCompleted) { Forward(attempt); diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.FeatureFlags.cs b/src/StackExchange.Redis/ConnectionMultiplexer.FeatureFlags.cs index 975da5de1..980b97545 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.FeatureFlags.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.FeatureFlags.cs @@ -1,5 +1,6 @@ using System; using System.ComponentModel; +using System.Net; using System.Threading; namespace StackExchange.Redis; @@ -13,6 +14,22 @@ private enum FeatureFlags { None, PreventThreadTheft = 1, + + /// + /// Service connections from threads this library owns, rather than from the global thread-pool. + /// + /// + /// For an application whose thread-pool is saturated - most often by sync-over-async somewhere, though + /// the cause does not matter here - the reply from redis cannot be processed, because processing it + /// needs a thread and every thread is waiting on one. Owning the reader and writer takes this library + /// out of that queue. It does not *fix* the thread-pool, and nothing here can: it means only that redis + /// traffic keeps flowing while the real problem is found. See docs/SyncOverAsync.md. + /// + /// Costs a reader and a writer thread per connection, so it is worth thinking about before enabling it + /// against a very wide cluster, where connection counts scale with the number of shards. + /// + /// + DedicatedThreads = 2, } private static void SetAutodetectFeatureFlags() @@ -54,4 +71,25 @@ public static bool GetFeatureFlag(string flag) && (s_featureFlags & flags) == flags; internal static bool PreventThreadTheft => (s_featureFlags & FeatureFlags.PreventThreadTheft) != 0; + + internal static bool DedicatedThreads => (s_featureFlags & FeatureFlags.DedicatedThreads) != 0; + + /// + /// Whether the connection of this type to this endpoint is read by a thread we own; null if there + /// is no such connection. + /// + /// + /// For tests and diagnostics: the flag is a request, and this + /// is what actually happened. Note that under RESP3 there is no separate subscription connection, so + /// asking about answers about the shared one. + /// + bool? IInternalConnectionMultiplexer.IsSyncReader(EndPoint endpoint, ConnectionType connectionType) + => GetPhysical(endpoint, connectionType)?.IsSyncReader; + + /// As IsSyncReader, for the writer. + bool? IInternalConnectionMultiplexer.IsSyncWriter(EndPoint endpoint, ConnectionType connectionType) + => GetPhysical(endpoint, connectionType)?.IsSyncWriter; + + private PhysicalConnection? GetPhysical(EndPoint endpoint, ConnectionType connectionType) + => GetServerEndPoint(endpoint, activate: false)?.GetBridge(connectionType, create: false)?.Physical; } diff --git a/src/StackExchange.Redis/Interfaces/IConnectionMultiplexer.cs b/src/StackExchange.Redis/Interfaces/IConnectionMultiplexer.cs index 617a63f25..905bc4352 100644 --- a/src/StackExchange.Redis/Interfaces/IConnectionMultiplexer.cs +++ b/src/StackExchange.Redis/Interfaces/IConnectionMultiplexer.cs @@ -24,6 +24,15 @@ internal interface IInternalConnectionMultiplexer : IConnectionMultiplexer long? GetConnectionId(EndPoint endPoint, ConnectionType type); + /// + /// Whether this connection is read/written by a thread we own rather than by the thread-pool; null + /// if there is no such connection. See the DedicatedThreads feature flag. + /// + bool? IsSyncReader(EndPoint endPoint, ConnectionType type); + + /// + bool? IsSyncWriter(EndPoint endPoint, ConnectionType type); + ServerSelectionStrategy ServerSelectionStrategy { get; } int GetSubscriptionsCount(); diff --git a/src/StackExchange.Redis/PhysicalBridge.cs b/src/StackExchange.Redis/PhysicalBridge.cs index 6a8857a39..e47813786 100644 --- a/src/StackExchange.Redis/PhysicalBridge.cs +++ b/src/StackExchange.Redis/PhysicalBridge.cs @@ -51,6 +51,9 @@ internal sealed class PhysicalBridge : IDisposable private long operationCount, socketCount; private volatile PhysicalConnection? physical; + /// The current connection, if any; for diagnostics and tests. + internal PhysicalConnection? Physical => physical; + private long profileLastLog; private int profileLogIndex; diff --git a/src/StackExchange.Redis/PhysicalConnection.Read.cs b/src/StackExchange.Redis/PhysicalConnection.Read.cs index e30102fe3..e65941021 100644 --- a/src/StackExchange.Redis/PhysicalConnection.Read.cs +++ b/src/StackExchange.Redis/PhysicalConnection.Read.cs @@ -27,6 +27,16 @@ internal static PhysicalConnection Dummy(Stream stream, BufferedStreamWriter.Wri private BufferedStreamWriter.WriteMode WriteMode { get; } + /// + /// Whether this connection is read by a thread of our own rather than by the thread-pool. + /// + /// + /// Deliberately mirrors the branching in rather than restating the rule: a + /// transport-backed connection is in push mode and has no reader of either kind, and otherwise the reader + /// follows the writer's mode. + /// + internal bool IsSyncReader => _transport is null && _output is { IsSync: true }; + internal void StartReading(CancellationToken cancellation = default) { if (_transport is { } transport) diff --git a/src/StackExchange.Redis/PhysicalConnection.Write.cs b/src/StackExchange.Redis/PhysicalConnection.Write.cs index 08edefd68..a47b6b5eb 100644 --- a/src/StackExchange.Redis/PhysicalConnection.Write.cs +++ b/src/StackExchange.Redis/PhysicalConnection.Write.cs @@ -19,14 +19,50 @@ public IBufferWriter Output } } + /// + /// Which writer this connection should use, given how it is configured and what it is for. + /// + /// + /// Policy rather than plumbing, so it is a function that can be tested rather than a condition buried in + /// : three rules interact here, and getting the precedence subtly wrong would show + /// up only as a performance characteristic, which is the kind of bug nobody notices for a year. + /// + internal static BufferedStreamWriter.WriteMode ResolveWriteMode( + ConnectionType connectionType, + BufferedStreamWriter.WriteMode configured, + bool dedicatedThreads) + { + // Redis policy over the generic writer: sync-mode targets latency, which pub/sub never needs. + if (connectionType is ConnectionType.Subscription) return BufferedStreamWriter.WriteMode.Async; + + // Sync-mode also owns its reader and writer threads rather than borrowing the thread-pool, which is + // what the DedicatedThreads flag is asking for: on a saturated pool the reply cannot be processed, + // because processing it needs a thread and every thread is waiting on one. Note this promotes an + // *unstated* preference only - anything explicitly configured still wins. + if (configured == BufferedStreamWriter.WriteMode.Default && dedicatedThreads) + { + return BufferedStreamWriter.WriteMode.Sync; + } + + return configured; + } + + /// + /// Whether this connection is written by a thread of our own rather than by the thread-pool. + /// + /// + /// Sync-mode is what owns the threads (see ), so this is simply what the + /// writer ended up in. Note it can change after connect: a switchable writer may transition to async. + /// + internal bool IsSyncWriter => _output is { IsSync: true }; + private void InitOutput(Stream? stream) { if (stream is null) return; _ioStream = stream; var config = BridgeCouldBeNull?.Multiplexer?.RawConfig; - // Redis policy over the generic writer: sync-mode targets latency, which pub/sub never needs. - var mode = connectionType is ConnectionType.Subscription ? BufferedStreamWriter.WriteMode.Async : WriteMode; + var mode = ResolveWriteMode(connectionType, WriteMode, ConnectionMultiplexer.DedicatedThreads); _output = BufferedStreamWriter.Create(mode, stream, config?.RequestBufferPool, OutputCancel); // Nothing awaits WriteComplete in production (it is mostly a test affordance); observe it so a diff --git a/tests/StackExchange.Redis.Build.Tests/SER305CodeFix.cs b/tests/StackExchange.Redis.Build.Tests/SER305CodeFix.cs index f7665503f..2155e3dce 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER305CodeFix.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER305CodeFix.cs @@ -178,5 +178,5 @@ public async Task M(IDatabase db, RedisKey key) } """, DiscardFix, - Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringSetAsync", "a transaction")); + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringSetAsync")); } diff --git a/tests/StackExchange.Redis.Build.Tests/SER306.cs b/tests/StackExchange.Redis.Build.Tests/SER306.cs index 4905e8095..5a4497364 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER306.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER306.cs @@ -27,7 +27,7 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync")); /// /// Combined flags are still a compile-time constant, so the bit is visible through the |. @@ -47,7 +47,7 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync")); [Fact] public Task BlockingOnFireAndForgetResult_IsFlagged() => VerifyAsync( @@ -64,7 +64,7 @@ public void M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync")); [Fact] public Task AwaitedFireAndForgetOnBatch_IsFlagged() => VerifyAsync( @@ -81,7 +81,7 @@ public async Task M(IDatabase db, RedisKey key) } } """, - Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringSetAsync", "a batch")); + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringSetAsync")); /// /// Not a constant, but | only sets bits - so the flag is there whatever flags holds. @@ -101,7 +101,7 @@ public async Task M(IDatabase db, RedisKey key, CommandFlags flags) } } """, - Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync")); /// /// The other side of that coin, and the reason the test exists: &~ would prove the flag *absent*, @@ -157,7 +157,11 @@ public async Task M(IDatabase db, RedisKey key) } """); - /// Fire-and-forget straight to the database is ordinary code and says nothing to this rule. + /// + /// Away from a transaction, *awaiting* fire-and-forget is a no-op await rather than a mistake, and is far + /// too common to flag - the library's own tests contain hundreds. Blocking on one is a different matter, + /// and is covered in . + /// [Fact] public Task AwaitedFireAndForgetOnDatabase_IsClean() => VerifyAsync( """ @@ -171,4 +175,19 @@ public async Task M(IDatabase db, RedisKey key) } } """); + + /// ...but an ordinary await, with no fire-and-forget, is exactly right and says nothing. + [Fact] + public Task AwaitedOnDatabase_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + await db.StringSetAsync(key, "value"); + } + } + """); } diff --git a/tests/StackExchange.Redis.Build.Tests/SER307.cs b/tests/StackExchange.Redis.Build.Tests/SER307.cs new file mode 100644 index 000000000..06ec47a31 --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER307.cs @@ -0,0 +1,177 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// Blocking on a redis call rather than awaiting it - sync over async. +/// +/// +/// The rule is about blocking specifically: an ordinary await is correct usage and must never +/// be flagged, which is most of what the negative cases below are for. Transactions and batches are +/// 's, and fire-and-forget is 's; this one owns everything else. +/// +public class SER307 : Verifier +{ + [Fact] + public Task BlockingOnResult_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + class C + { + public void M(IDatabase db, RedisKey key) + { + var value = {|#0:{|#1:db.StringGetAsync(key)|}.Result|}; + } + } + """, + Diagnostic("SER307").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync")); + + [Fact] + public Task BlockingOnWait_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + class C + { + public void M(IDatabase db, RedisKey key) + { + {|#0:{|#1:db.StringSetAsync(key, "value")|}.Wait()|}; + } + } + """, + Diagnostic("SER307").WithLocation(0).WithLocation(1).WithArguments("StringSetAsync")); + + [Fact] + public Task BlockingOnGetAwaiterGetResult_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + class C + { + public void M(IDatabase db, RedisKey key) + { + var value = {|#0:{|#1:db.StringGetAsync(key)|}.GetAwaiter().GetResult()|}; + } + } + """, + Diagnostic("SER307").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync")); + + /// + /// IRedisAsync is the root of every async surface, so a server or subscriber call is covered by the + /// same test rather than needing its own. + /// + [Fact] + public Task BlockingOnServerCall_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + class C + { + public void M(IServer server) + { + {|#0:{|#1:server.PingAsync()|}.Wait()|}; + } + } + """, + Diagnostic("SER307").WithLocation(0).WithLocation(1).WithArguments("PingAsync")); + + [Fact] + public Task BlockingOnSubscriberCall_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + class C + { + public void M(ISubscriber sub, RedisChannel channel) + { + var count = {|#0:{|#1:sub.PublishAsync(channel, "value")|}.Result|}; + } + } + """, + Diagnostic("SER307").WithLocation(0).WithLocation(1).WithArguments("PublishAsync")); + + /// + /// Unlike SER305, an unreadable flags argument still warns: the harmful reading is far the more likely, + /// and being wrong here costs a warning rather than a build. + /// + [Fact] + public Task BlockingWithNonConstantFlags_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + class C + { + public void M(IDatabase db, RedisKey key, CommandFlags flags) + { + var value = {|#0:{|#1:db.StringGetAsync(key, flags)|}.Result|}; + } + } + """, + Diagnostic("SER307").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync")); + + // ---- negative cases ---- + + /// Awaiting is the whole point of the async API, and must never 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) + { + var value = await db.StringGetAsync(key); + await db.StringSetAsync(key, value); + } + } + """); + + /// + /// The synchronous API is out of scope for this rule, which is about blocking on the *async* API. + /// + /// + /// Not an endorsement: a blocked thread is a blocked thread, and a synchronous call from a thread-pool + /// thread starves the pool exactly as sync-over-async does. It is unflagged because deciding when a + /// synchronous call is legitimate needs to know which thread the caller is on, which an analyzer cannot + /// see - and a rule that fired on every synchronous call would be unusable. + /// + [Fact] + public Task SynchronousApi_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + class C + { + public void M(IDatabase db, RedisKey key) + { + var value = db.StringGet(key); + db.StringSet(key, value); + } + } + """); + + /// Fire-and-forget completes before the call returns, so blocking waits for nothing: SER306. + [Fact] + public Task BlockingOnFireAndForget_IsSER306() => VerifyAsync( + """ + using StackExchange.Redis; + class C + { + public void M(IDatabase db, RedisKey key) + { + {|#0:{|#1:db.StringSetAsync(key, "value", flags: CommandFlags.FireAndForget)|}.Wait()|}; + } + } + """, + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringSetAsync")); + + /// Blocking on a non-redis task is somebody else's business. + [Fact] + public Task BlockingOnUnrelatedTask_IsClean() => VerifyAsync( + """ + using System.Threading.Tasks; + class C + { + public void M() + { + Task.Delay(1).Wait(); + } + } + """); +} diff --git a/tests/StackExchange.Redis.Tests/DedicatedThreadsTests.cs b/tests/StackExchange.Redis.Tests/DedicatedThreadsTests.cs new file mode 100644 index 000000000..b982e8289 --- /dev/null +++ b/tests/StackExchange.Redis.Tests/DedicatedThreadsTests.cs @@ -0,0 +1,101 @@ +using System; +using System.Net; +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// End-to-end coverage for the DedicatedThreads feature flag: that asking for it actually moves the +/// reader and writer off the thread-pool, and that pub/sub is left alone. +/// +/// +/// +/// DedicatedThreadsUnitTests pins the decision; this pins the consequence. They are worth having +/// separately, because the decision is a pure function and this needs a live connection to observe. +/// +/// +/// The flag is process-wide static and is read when a connection is *established*, so every test here builds +/// its own unshared connection inside the window where the flag is set, and the class is non-parallel: a +/// shared connection would have been made before the flag was, and would report the old answer. +/// +/// +[RunPerProtocol] +[Collection(NonParallelCollection.Name)] +public class DedicatedThreadsTests(ITestOutputHelper output) : TestBase(output) +{ + private const string Flag = "DedicatedThreads"; + + /// Connect with the flag in a known state, and put it back afterwards. + private async Task WithFlagAsync(bool enabled, Func assert) + { + var wasSet = ConnectionMultiplexer.GetFeatureFlag(Flag); + ConnectionMultiplexer.SetFeatureFlag(Flag, enabled); + try + { + await using var conn = Create(shared: false); + var endpoint = conn.GetEndPoints()[0]; + + // the flag is consumed while the connection is being established, so make sure one exists before + // asking anything about it - and touch pub/sub too, since under RESP2 that is a second connection + // that is not created until it is needed + var db = conn.GetDatabase(); + await db.PingAsync(); + await conn.GetSubscriber().PingAsync(); + + await assert(conn, endpoint); + } + finally + { + ConnectionMultiplexer.SetFeatureFlag(Flag, wasSet); + } + } + + [Fact] + public Task WithoutTheFlag_TheThreadPoolServicesTheConnection() => WithFlagAsync(false, (conn, endpoint) => + { + Assert.False(conn.IsSyncReader(endpoint, ConnectionType.Interactive)); + Assert.False(conn.IsSyncWriter(endpoint, ConnectionType.Interactive)); + return Task.CompletedTask; + }); + + [Fact] + public Task WithTheFlag_TheInteractiveConnectionOwnsItsThreads() => WithFlagAsync(true, (conn, endpoint) => + { + Assert.True(conn.IsSyncReader(endpoint, ConnectionType.Interactive)); + Assert.True(conn.IsSyncWriter(endpoint, ConnectionType.Interactive)); + return Task.CompletedTask; + }); + + /// + /// Pub/sub keeps using the thread-pool, which is the claim the docs make and the reason the flag's cost is + /// quoted per node rather than per connection. + /// + /// + /// Only checkable under RESP2. Under RESP3 there *is* no separate subscription connection - the bridge + /// lookup returns the interactive one - so the question does not arise, and asserting "false" there would + /// be asserting against the shared connection we just required to be true. + /// + [Fact] + public Task WithTheFlag_PubSubStaysOnTheThreadPool() => WithFlagAsync(true, (conn, endpoint) => + { + var protocol = conn.GetServerEndPoint(endpoint).Protocol ?? RedisProtocol.Resp2; + if (protocol >= RedisProtocol.Resp3) + { + // one connection carries both, so the subscription lookup is the interactive connection + Assert.True(conn.IsSyncReader(endpoint, ConnectionType.Subscription)); + Assert.True(conn.IsSyncWriter(endpoint, ConnectionType.Subscription)); + } + else + { + Assert.False(conn.IsSyncReader(endpoint, ConnectionType.Subscription)); + Assert.False(conn.IsSyncWriter(endpoint, ConnectionType.Subscription)); + + // ...and the interactive one is still dedicated, so this is a difference between the two + // connections rather than the flag having failed to apply at all + Assert.True(conn.IsSyncReader(endpoint, ConnectionType.Interactive)); + } + + return Task.CompletedTask; + }); +} diff --git a/tests/StackExchange.Redis.Tests/DedicatedThreadsUnitTests.cs b/tests/StackExchange.Redis.Tests/DedicatedThreadsUnitTests.cs new file mode 100644 index 000000000..4edf8a8b4 --- /dev/null +++ b/tests/StackExchange.Redis.Tests/DedicatedThreadsUnitTests.cs @@ -0,0 +1,83 @@ +using RESPite.Streams; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// The DedicatedThreads feature flag, which takes this library's reader and writer off the global +/// thread-pool and onto threads it owns. +/// +/// +/// Exercised through rather than by connecting: the flag is +/// process-wide static, so a test that set it and then connected would be racing every other test in the run. +/// The policy is the part worth pinning anyway - that sync-mode is what owns the threads is a fact about +/// SwitchableBufferedStreamWriter, and is covered by BufferedStreamWriterTests. +/// +public class DedicatedThreadsUnitTests +{ + [Theory] + [InlineData(ConnectionType.Interactive)] + [InlineData(ConnectionType.Subscription)] + public void WithoutTheFlag_NothingChanges(ConnectionType connectionType) + { + var expected = connectionType is ConnectionType.Subscription + ? BufferedStreamWriter.WriteMode.Async + : BufferedStreamWriter.WriteMode.Default; + + Assert.Equal(expected, PhysicalConnection.ResolveWriteMode(connectionType, BufferedStreamWriter.WriteMode.Default, dedicatedThreads: false)); + } + + [Fact] + public void WithTheFlag_InteractiveConnectionsOwnTheirThreads() + => Assert.Equal( + BufferedStreamWriter.WriteMode.Sync, + PhysicalConnection.ResolveWriteMode(ConnectionType.Interactive, BufferedStreamWriter.WriteMode.Default, dedicatedThreads: true)); + + /// + /// Pub/sub keeps its own rule: the flag is not a licence to reverse a deliberate policy. + /// + [Fact] + public void WithTheFlag_SubscriptionsAreUnaffected() + => Assert.Equal( + BufferedStreamWriter.WriteMode.Async, + PhysicalConnection.ResolveWriteMode(ConnectionType.Subscription, BufferedStreamWriter.WriteMode.Default, dedicatedThreads: true)); + + /// + /// The flag promotes an unstated preference, so an explicit choice still wins - otherwise enabling + /// it under support guidance would silently undo whatever had been configured to get that far. + /// + /// + /// Takes the test project's public mirror rather than the internal enum, which a + /// public test signature cannot name (CS0051) - the same reason that mirror exists at all. + /// + [Theory] + [InlineData(WriteMode.Async)] + [InlineData(WriteMode.Pipe)] + [InlineData(WriteMode.Sync)] + public void WithTheFlag_AnExplicitModeIsKept(WriteMode configured) + { + var expected = (BufferedStreamWriter.WriteMode)configured; + Assert.Equal(expected, PhysicalConnection.ResolveWriteMode(ConnectionType.Interactive, expected, dedicatedThreads: true)); + } + + /// + /// The flag is reachable by name, case-insensitively, exactly as preventthreadtheft is - which is + /// how it will actually be typed into an application's startup under support guidance. + /// + [Fact] + public void TheFlagIsSettableByName() + { + Assert.False(ConnectionMultiplexer.GetFeatureFlag("DedicatedThreads")); + try + { + ConnectionMultiplexer.SetFeatureFlag("dedicatedthreads", true); + Assert.True(ConnectionMultiplexer.GetFeatureFlag("DedicatedThreads")); + } + finally + { + ConnectionMultiplexer.SetFeatureFlag("DedicatedThreads", false); + } + + Assert.False(ConnectionMultiplexer.GetFeatureFlag("DedicatedThreads")); + } +} diff --git a/tests/StackExchange.Redis.Tests/Helpers/SharedConnectionFixture.cs b/tests/StackExchange.Redis.Tests/Helpers/SharedConnectionFixture.cs index 8da84bce6..27a676a5e 100644 --- a/tests/StackExchange.Redis.Tests/Helpers/SharedConnectionFixture.cs +++ b/tests/StackExchange.Redis.Tests/Helpers/SharedConnectionFixture.cs @@ -215,6 +215,12 @@ public void ExportConfiguration(Stream destination, ExportOptions options = Expo public override string ToString() => _inner.ToString(); long? IInternalConnectionMultiplexer.GetConnectionId(EndPoint endPoint, ConnectionType type) => _inner.GetConnectionId(endPoint, type); + + bool? IInternalConnectionMultiplexer.IsSyncReader(EndPoint endPoint, ConnectionType type) + => _inner.IsSyncReader(endPoint, type); + + bool? IInternalConnectionMultiplexer.IsSyncWriter(EndPoint endPoint, ConnectionType type) + => _inner.IsSyncWriter(endPoint, type); } public void Dispose()