From 0fb0348a51143ed69d8f256589e25722ab4a2b7e Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 11:29:27 +0100 Subject: [PATCH 01/17] Add a DedicatedThreads feature flag, and drop an `!` The reader and writer that do not depend on the thread-pool are already built and wired: ConfigurationOptions.WriteMode reaches BufferedStreamWriter.Create, and Sync mode starts a "SE.Redis Sync Writer" thread, which makes IsSync true, which is what makes PhysicalConnection.StartReading take its dedicated "SE.Redis Sync Reader" branch instead of Task.Run. All that was missing was a way for anyone outside this assembly to ask for it. SetFeatureFlag is the route, alongside preventthreadtheft: no new public API to live with, and EditorBrowsable(Never) already says "under support guidance" rather than "reach for this". It matters for an application whose thread-pool is saturated - most often by sync-over-async - where the reply from redis cannot be processed because processing it needs a thread and every thread is waiting on one. 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. Two limits are deliberate. It promotes an *unstated* preference, so anything explicitly configured still wins; and it leaves pub/sub on the existing policy rather than quietly reversing a decision made for latency reasons. Resolving the mode moved out of InitOutput into a function, because three rules now interact and getting their precedence wrong would surface only as a performance characteristic - the kind of bug that goes unnoticed for a year. That also makes it testable without connecting, which matters here: the flag is process-wide static, so a test that set it and then connected would race every other test in the run. Also swaps `_attempt!` for `?? throw`: same loud failure if the order ever changes, but the compiler is convinced rather than told. --- .../Availability/RetryTransaction.cs | 8 +- .../ConnectionMultiplexer.FeatureFlags.cs | 18 ++++ .../PhysicalConnection.Write.cs | 31 ++++++- .../DedicatedThreadsUnitTests.cs | 83 +++++++++++++++++++ 4 files changed, 136 insertions(+), 4 deletions(-) create mode 100644 tests/StackExchange.Redis.Tests/DedicatedThreadsUnitTests.cs diff --git a/src/StackExchange.Redis/Availability/RetryTransaction.cs b/src/StackExchange.Redis/Availability/RetryTransaction.cs index cd45ab57e1..3b1fec6bde 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 975da5de19..bab8d075d1 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.FeatureFlags.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.FeatureFlags.cs @@ -13,6 +13,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 +70,6 @@ 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; } diff --git a/src/StackExchange.Redis/PhysicalConnection.Write.cs b/src/StackExchange.Redis/PhysicalConnection.Write.cs index 08edefd68c..f1bfad71a7 100644 --- a/src/StackExchange.Redis/PhysicalConnection.Write.cs +++ b/src/StackExchange.Redis/PhysicalConnection.Write.cs @@ -19,14 +19,41 @@ 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; + } + 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.Tests/DedicatedThreadsUnitTests.cs b/tests/StackExchange.Redis.Tests/DedicatedThreadsUnitTests.cs new file mode 100644 index 0000000000..4edf8a8b4c --- /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")); + } +} From cc9020e8ce90c28458de0815aee9238f0f42f0af Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 11:40:20 +0100 Subject: [PATCH 02/17] Add SER307: blocking on a redis call instead of awaiting it Sync over async is worse against a multiplexed client than the general case, because the thing being waited for needs the same resource the wait is consuming: the reply arrives at the socket promptly, but turning it into a completed Task needs a thread, and the blocked callers are holding them. Past a certain concurrency nothing completes, so nothing is released. The symptom is timeouts with the data already sitting unread in the socket, which looks like a network or server fault and is neither. The detection is the SER305 machinery re-gated: same unwrapping of .Result, .Wait() and .GetAwaiter().GetResult(), same fire-and-forget test, but keyed on IRedisAsync - the root of every async surface, so IDatabaseAsync, IServer and ISubscriber all come along - rather than on IBatch/ITransactionAsync. Three shapes are deliberately *not* reported: - awaiting. That is correct usage of an async API and is what the rule steers people towards; flagging it would advise them in a circle. - the synchronous API. db.StringGet(key) is a genuinely synchronous path, not sync-over-async, and is the answer for callers that cannot be async. - awaiting a fire-and-forget result away from a transaction. It 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, taking SER305 with it. Blocking on one still reports, as SER306 - it waits for nothing. Unlike SER305, an unreadable flags argument still warns here: the harmful reading is much the more likely, and being wrong costs a warning rather than a build. docs/SyncOverAsync.md is the explainer, and is what the diagnostic links to rather than a rule page - what people need at that moment is the thread-pool story, not a description of the squiggle. It covers why raising MinThreads moves the wall rather than removing it, how to tell this apart from the problems in Timeouts.md and ThreadTheft.md, and the DedicatedThreads flag as a mitigation that explicitly does not fix the thread-pool. Verified by building the whole repo with the rules live: zero hits across src, tests and toys. --- docs/SyncOverAsync.md | 127 +++++++++++++ docs/Timeouts.md | 4 + docs/index.md | 3 +- docs/rules/index.md | 9 +- .../AnalyzerReleases.Unshipped.md | 3 +- eng/StackExchange.Redis.Build/Diagnostics.cs | 31 +++- .../QueuedResultAnalyzer.cs | 112 ++++++++---- .../SER305CodeFix.cs | 2 +- .../StackExchange.Redis.Build.Tests/SER306.cs | 31 +++- .../StackExchange.Redis.Build.Tests/SER307.cs | 172 ++++++++++++++++++ 10 files changed, 443 insertions(+), 51 deletions(-) create mode 100644 docs/SyncOverAsync.md create mode 100644 tests/StackExchange.Redis.Build.Tests/SER307.cs diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md new file mode 100644 index 0000000000..0a113aebd1 --- /dev/null +++ b/docs/SyncOverAsync.md @@ -0,0 +1,127 @@ +# 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, if the caller can be async +var value = await db.StringGetAsync(key); + +// the fix, if it genuinely cannot +var value = db.StringGet(key); +``` + +That last line is the one people miss. **StackExchange.Redis has a real synchronous API**, and it is not +sync-over-async internally — it is a genuinely synchronous path. If you need a blocking call, use it. Blocking +on the *async* method is the only version of this that hurts. + +## 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. + +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. +2. **Use the synchronous API** where the caller genuinely cannot be async — a constructor, an interface you do + not control, a legacy entry point. `db.StringGet(key)` is not sync-over-async. +3. **Take the client off the thread-pool**, as a mitigation while you do 1 or 2 (see below). + +### 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 **per connection**, so think about it before enabling it against a + very wide cluster, where connection count scales with the number of shards; +- it is deliberately opt-in, and set process-wide at startup rather than per-connection. + +## 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 use the synchronous API 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. +- **`SER305`** — waiting on a command queued to a transaction or batch before `Execute[Async]` has sent it. + That one is an *error*, because it cannot ever complete. + +See [Analyzer rules](rules/) for the full set, including how to turn any of them down. + +## 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 +- [Pipelines and multiplexers](PipelinesMultiplexers) — why the client is shaped this way in the first place diff --git a/docs/Timeouts.md b/docs/Timeouts.md index ea9830041c..cedaa666dc 100644 --- a/docs/Timeouts.md +++ b/docs/Timeouts.md @@ -27,6 +27,10 @@ Asynchronous operations in StackExchange.Redis can come back in 3 different ways 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. +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. For these .NET-provided global thread pools: once the number of existing (busy) threads hits the "minimum" number of threads, the ThreadPool will throttle the rate at which it injects new threads to one thread per 500 milliseconds. This means that if your system gets a burst of work needing an IOCP thread, it will process that work very quickly. However, if the burst of work is more than the configured "Minimum" setting, there will be some delay in processing some of the work as the ThreadPool waits for one of two things to happen: diff --git a/docs/index.md b/docs/index.md index d8aea9a548..36c032c571 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 452ada41bb..a9ccfe750f 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 abbe418de7..6e1fd21b96 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 b627fa4aea..f2e3891b8b 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,40 @@ 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 use the synchronous API 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. + /// + /// + 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, or use the synchronous API", + 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 cd5f38aed3..fff6eae993 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/tests/StackExchange.Redis.Build.Tests/SER305CodeFix.cs b/tests/StackExchange.Redis.Build.Tests/SER305CodeFix.cs index f7665503f9..2155e3dce6 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 4905e8095e..5a4497364c 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 0000000000..af42d8a75a --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER307.cs @@ -0,0 +1,172 @@ +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 not sync-over-async: it is a genuinely synchronous path, and is the answer this + /// rule points people at - so flagging it would be advising them into a circle. + /// + [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(); + } + } + """); +} From 14f97d3d569170723088360eac8176b731eedd71 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 11:41:48 +0100 Subject: [PATCH 03/17] Show a worked timeout message in the sync-over-async doc Describing the signature in prose leaves the reader to assemble it from three separate facts; showing one message with all of them makes it a pattern-match against what they are already looking at. The `in` figure is called out as the tell, because it is the one that overturns the natural first conclusion: the server has answered and the bytes are in the buffer, so the network, the server and the connection are all fine - there is simply no thread free to pick the reply up. An idle `mgr` beside a busy `WORKER` says the same from the other side. Numbers are illustrative rather than from any particular incident, and are internally consistent so the example reads as a real message. --- docs/SyncOverAsync.md | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index 0a113aebd1..adb2696c4a 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -1,4 +1,4 @@ -# Sync over async, and thread-pool starvation +# 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 @@ -67,6 +67,21 @@ combination that points here is: - 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, mgr: 10 of 10 available, + IOCP: (Busy=0,Free=1000,Min=8,Max=1000), + WORKER: (Busy=73,Free=32694,Min=64,Max=32767) + +Read that as: 84 commands are awaiting replies (`qs`), **476KiB has already arrived and is sitting unread** +(`in`), the client's own dedicated pool is completely idle (`mgr: 10 of 10`) — and the global worker pool has +more busy threads than its minimum, so it is injecting new ones a couple per second at most. + +The `in` figure is the tell. The server has answered; the bytes are in the buffer. Nothing is wrong with the +network, the server, or the connection — there is simply no thread available to pick the reply up. An idle +`mgr` alongside a busy `WORKER` says the same thing from the other direction: the client is not the bottleneck, +the application's thread-pool is. + If the pool is healthy and you still see timeouts, this is not your problem — look at [Timeouts](Timeouts) and [Thread Theft](ThreadTheft) instead. From 54ee94e8f5df01331aeb13855d87c0488b90ab90 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 11:47:20 +0100 Subject: [PATCH 04/17] Correct: the synchronous API is not a fix for this The page and the SER307 message both offered "or use the synchronous API" as the answer for a caller that cannot be made async. That is wrong, and wrong in the direction that does harm: it would send people to something with the same failure. db.StringGet(key) is not literally sync-over-async - it does not wrap an async call and block on it - but that distinction is about mechanism, not about consequence. The calling thread still blocks until redis replies, and processing that reply still needs a thread; if the caller is on the thread-pool, as it is under ASP.NET and anything else pool-driven, the pool starves exactly the same way. The earlier text reasoned from the mechanism and stopped there. So the shipped message now says "await it instead" and stops, the page has a section saying plainly that the synchronous API is not an escape hatch, and the fire-and-forget advice is "drop the flag" rather than "go synchronous". The rule still does not flag synchronous calls, which is a scoping decision rather than an endorsement: whether one is legitimate depends on which thread the caller is on, and an analyzer cannot see that. The test that pins this now says so, instead of describing the synchronous API as the answer. --- docs/SyncOverAsync.md | 35 ++++++++++++------- eng/StackExchange.Redis.Build/Diagnostics.cs | 10 ++++-- .../StackExchange.Redis.Build.Tests/SER307.cs | 9 +++-- 3 files changed, 37 insertions(+), 17 deletions(-) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index adb2696c4a..43e4e10335 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -16,16 +16,11 @@ it is stuck. // the problem var value = db.StringGetAsync(key).Result; -// the fix, if the caller can be async +// the fix var value = await db.StringGetAsync(key); - -// the fix, if it genuinely cannot -var value = db.StringGet(key); ``` -That last line is the one people miss. **StackExchange.Redis has a real synchronous API**, and it is not -sync-over-async internally — it is a genuinely synchronous path. If you need a blocking call, use it. Blocking -on the *async* method is the only version of this that hurts. +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 @@ -90,10 +85,24 @@ If the pool is healthy and you still see timeouts, this is not your problem — 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. -2. **Use the synchronous API** where the caller genuinely cannot be async — a constructor, an interface you do - not control, a legacy entry point. `db.StringGet(key)` is not sync-over-async. -3. **Take the client off the thread-pool**, as a mitigation while you do 1 or 2 (see below). + 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 @@ -120,8 +129,8 @@ Two caveats worth knowing before you enable it: `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 use the synchronous API if you -actually want a result — see `SER306`. +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 diff --git a/eng/StackExchange.Redis.Build/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs index f2e3891b8b..2454253b08 100644 --- a/eng/StackExchange.Redis.Build/Diagnostics.cs +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -212,7 +212,7 @@ 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 fire-and-forget, so its result is the default value whenever it is read, not the server's answer; discard it, or use the synchronous API if you want the result", + 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, @@ -235,11 +235,17 @@ internal static class Diagnostics /// 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, or use the synchronous API", + 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, diff --git a/tests/StackExchange.Redis.Build.Tests/SER307.cs b/tests/StackExchange.Redis.Build.Tests/SER307.cs index af42d8a75a..06ec47a31d 100644 --- a/tests/StackExchange.Redis.Build.Tests/SER307.cs +++ b/tests/StackExchange.Redis.Build.Tests/SER307.cs @@ -124,9 +124,14 @@ public async Task M(IDatabase db, RedisKey key) """); /// - /// The synchronous API is not sync-over-async: it is a genuinely synchronous path, and is the answer this - /// rule points people at - so flagging it would be advising them into a circle. + /// 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( """ From 2160d2a325d94710422fcf6ac4eae508279f343d Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 11:49:34 +0100 Subject: [PATCH 05/17] Stop leaning on `mgr`, which 3.0 no longer emits The worked example was assembled from the 2.x message documented in Timeouts.md, and included `mgr` - the dedicated SocketManager thread-pool. That machinery is gone: ConfigurationOptions.SocketManager is marked obsolete as "no longer used by StackExchange.Redis", and no part of this assembly emits the field any more. Reading an idle `mgr` as corroboration was advice about a number nobody will find. The current statistics are IOCP, WORKER and - on .NET 5 and above - POOL, which carries thread count, queued and completed work items, and active timers. That is a better signal than `mgr` was for this particular failure: QueuedItems counts work the pool has accepted and not yet started, so a large value says directly that work is arriving faster than threads can be found for it, which is the same story `in` tells from the other side. Timeouts.md keeps its 2.x and 1.x examples, since they are explicitly historical and people do still meet old messages, but now says plainly that `mgr` belongs to 2.x and what replaced it. --- docs/SyncOverAsync.md | 20 +++++++++++--------- docs/Timeouts.md | 5 +++++ 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index 43e4e10335..850a6486a0 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -64,18 +64,20 @@ combination that points here is: A message showing all of it at once looks something like: - Timeout performing GET MyKey (5000ms), inst: 0, qs: 84, in: 487312, mgr: 10 of 10 available, + 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) + 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 client's own dedicated pool is completely idle (`mgr: 10 of 10`) — and the global worker pool has -more busy threads than its minimum, so it is injecting new ones a couple per second at most. +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; the bytes are in the buffer. Nothing is wrong with the -network, the server, or the connection — there is simply no thread available to pick the reply up. An idle -`mgr` alongside a busy `WORKER` says the same thing from the other direction: the client is not the bottleneck, -the application's thread-pool is. +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. diff --git a/docs/Timeouts.md b/docs/Timeouts.md index cedaa666dc..d65e1c03c0 100644 --- a/docs/Timeouts.md +++ b/docs/Timeouts.md @@ -47,6 +47,11 @@ If we look at an example error message from StackExchange.Redis 2.0, you will se 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. +Note that `mgr` describes the dedicated `SocketManager` thread-pool used by 2.x. **That machinery is gone as +of 3.0** - `ConfigurationOptions.SocketManager` is obsolete and unused - so `mgr` no longer appears; current +messages instead carry `POOL` (thread count, queued and completed work items, and active timers) on .NET 5 +and above. The `IOCP` and `WORKER` statistics are unchanged, and are still the ones to read first. + In 1.*, the information is similar but slightly different: System.TimeoutException: Timeout performing GET MyKey, inst: 2, mgr: Inactive, From ab487ad0bd9195403b9b28e6e73f3c4b015b109c Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 11:52:07 +0100 Subject: [PATCH 06/17] Remove the dead SocketManager pool from Timeouts.md Stripping `mgr` from the field list was not enough: the page also *described* the dedicated thread-pool as current machinery - "StackExchange.Redis maintains a dedicated thread-pool", "this can be customized by explicitly providing a SocketManager", "the vast majority of asynchronous operations will be serviced by this dedicated pool". None of that has been true since 3.0. Removing only the field name would have left the explanation behind it intact, which is the worse half. So: the example message is the current one (IOCP/WORKER/POOL, no `mgr`), the `mgr` row is gone from the field table, the "3 different ways" list is now the two that exist, and the paragraph contrasting the global pool against "the dedicated StackExchange.Redis thread-pool" no longer draws a distinction that does not exist. One `mgr` reference is kept deliberately, as a single line saying it belonged to 2.x and is gone: people arrive here holding an old message or old advice, and silence would leave them assuming the page had simply not covered it. It points at the DedicatedThreads mitigation, which is the nearest thing 3.0 has to what they were looking for. Note this also drops the 1.x example message and the sentence contrasting it with 2.0. That is beyond removing `mgr` - it is version archaeology two majors back, and it carried a stale `mgr: Inactive` - but it is easily restored if it is still earning its place. --- docs/Timeouts.md | 35 ++++++++++------------------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/docs/Timeouts.md b/docs/Timeouts.md index d65e1c03c0..0aaf68ed42 100644 --- a/docs/Timeouts.md +++ b/docs/Timeouts.md @@ -19,13 +19,12 @@ 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 @@ -37,31 +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. +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. -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). +An error message carries ThreadPool statistics alongside the connection state (see IOCP, WORKER and POOL below). - Timeout performing GET MyKey (1000ms), inst: 2, qs: 6, in: 0, mgr: 9 of 10 available, + 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) -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. +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 `mgr` describes the dedicated `SocketManager` thread-pool used by 2.x. **That machinery is gone as -of 3.0** - `ConfigurationOptions.SocketManager` is obsolete and unused - so `mgr` no longer appears; current -messages instead carry `POOL` (thread count, queued and completed work items, and active timers) on .NET 5 -and above. The `IOCP` and `WORKER` statistics are unchanged, and are still the ones to read first. - -In 1.*, the information is similar but slightly different: - - 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, - IOCP: (Busy=6,Free=994,Min=4,Max=1000), - WORKER: (Busy=3,Free=997,Min=4,Max=1000) - -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. - -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. @@ -102,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.| From c328a7d528493cbb412f1a3b70ae9f7d67f4658c Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 11:55:50 +0100 Subject: [PATCH 07/17] Say that the thread-per-connection cost is not permanent The wide-cluster caveat reads as a flat "not for you" to precisely the people most likely to need the mitigation, and leaves them with nowhere to go. Naming the direction - dedicated readers over io_uring and IOCP, servicing many connections from a small fixed set of threads rather than a pair each - turns that into "not yet". Deliberately hedged: no date, and an explicit note that nothing on the page depends on it. A roadmap line in documentation is a promise people will hold you to, so it says what is being worked towards and stops there. --- docs/SyncOverAsync.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index 850a6486a0..c16e588243 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -127,6 +127,12 @@ Two caveats worth knowing before you enable it: very wide cluster, where connection count 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 From 30c414723ce589c9ca861f4e58aa9a0a03497c26 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 11:58:42 +0100 Subject: [PATCH 08/17] Stop citing SER305 from the sync-over-async page SER305 links to its own rule page, not to this one, and it describes a different failure: a command queued to a transaction cannot complete because it has not been sent, which is transaction semantics rather than anything to do with the thread-pool. Listing it here borrowed authority the page does not have for it, and invited a reader to think the two were the same story. SER306 stays, on the merits rather than on a link: it is the same subject matter, and SER307 defers the fire-and-forget case to it, so somebody arriving from SER307 needs to know where that went. The pointer to the rules index now says the transaction rules are a different problem, which is where anyone who wants SER305 will find it. --- docs/SyncOverAsync.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index c16e588243..5abbf9e4c2 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -144,12 +144,12 @@ 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. -- **`SER306`** — reading a fire-and-forget result, which is always the default value. -- **`SER305`** — waiting on a command queued to a transaction or batch before `Execute[Async]` has sent it. - That one is an *error*, because it cannot ever complete. +- **`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 how to turn any of them down. +See [Analyzer rules](rules/) for the full set — including the transaction rules, which describe a different +problem — and for how to turn any of them down. ## See also From f946858baf363538e32caafba956677c64b1d5ad Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 12:00:37 +0100 Subject: [PATCH 09/17] Qualify the Thread Theft reference as largely historical Sending a reader on modern .NET to that page unqualified costs them time on a problem they almost certainly do not have. The default path already asks for RunContinuationsAsynchronously, so continuations do not run inline on the reader; the preventthreadtheft flag switches that *off* in favour of dispatching by hand, for hosts whose synchronization context cannot be trusted to honour the option - and the autodetect only enables it for LegacyAspNetSynchronizationContext, i.e. classic ASP.NET on .NET Framework. Kept rather than dropped, because the two failures do look alike from the outside - both end with a reply that is not processed - so it is worth being able to rule it out. It is now labelled well enough to rule out quickly. --- docs/SyncOverAsync.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index 5abbf9e4c2..a4ecdbca09 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -155,5 +155,8 @@ problem — and for how to turn any of them down. - [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 + 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 From 774481225f3e426271b11f285ea5585b08b13911 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 12:02:53 +0100 Subject: [PATCH 10/17] ThreadTheft.md: say who this is for, before saying what to do The page opens by telling the reader to set preventthreadtheft and see if it helps, and only explains several paragraphs later that the situation requires a SynchronizationContext whose Post dispatches synchronously - in practice classic ASP.NET. Somebody on .NET 8 who followed an exception link here will therefore try the flag first and learn nothing from it, because ASP.NET Core installs no SynchronizationContext at all and the default RunContinuationsAsynchronously already covers them. The body needed nothing: it is accurate and explains the mechanism, the exact web.config settings that produce it, and why the flag is not overhead in the case that needs it. Only the ordering was wrong, so this adds the applicability note above the advice rather than rewriting anything. Also adds a closing pointer, because the two failures present alike: thread theft has a distinctive marker in `rs: CompletePendingMessage`, and timeouts *without* it suggest a reader that cannot get a thread rather than one being hijacked - which is sync-over-async, and applies on every runtime. --- docs/ThreadTheft.md | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/docs/ThreadTheft.md b/docs/ThreadTheft.md index d5b8e717e1..a3fdf692e3 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. From bcba95fc4fb52b599497324324d1f371d0a3efbb Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 12:05:18 +0100 Subject: [PATCH 11/17] Tell people how to turn SER307 off SER307's help link goes straight to this page rather than to a rules page, so it was the one rule in the set whose diagnostic led somewhere with no suppression guidance at all - every other ID has a rules page ending in a Suppressing section. It is also the rule most likely to arrive as hundreds of warnings at once on an existing codebase, which is exactly when somebody needs that paragraph and is least inclined to go hunting for it. Offers all three levers, in the order of preference that keeps coverage: a pragma at a site that has been decided about, NoWarn for a project being worked through, and a severity downgrade for teams that want it visible without failing the build. Says plainly that suppressing does not fix anything, but says it after showing how - a rule that lectures before it answers the question gets removed wholesale, which costs more than the warning did. --- docs/SyncOverAsync.md | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index a4ecdbca09..c4c0302bf6 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -148,8 +148,36 @@ The package ships a Roslyn analyzer that flags these at build time: - **`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 — and for how to turn any of them down. +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 From 34b1e8b43f233da9d077927b3421e8109dc49c0d Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 12:08:20 +0100 Subject: [PATCH 12/17] Spell out the connection count per node, and what RESP2 costs here "A reader and a writer per connection" leaves the reader to work out how many connections they have, and the answer is protocol-dependent: RESP3 multiplexes pub/sub over the interactive connection, RESP2 needs a second one per node. The arithmetic is not the interesting part, though, and it does not come out where you would guess. The second RESP2 connection does *not* double the thread cost, because subscription connections are deliberately excluded from sync mode and stay on the thread-pool - which means the flag protects commands and leaves pub/sub exposed. On RESP3 the single connection carries both, so both are covered. That is a behavioural difference rather than a sizing one, and it is the thing somebody on RESP2 with stalling pub/sub needs to know. Says RESP3 is the default "where the server supports it" rather than flatly: TryResp3 leans on the server version when Protocol is unspecified, so it is negotiated via HELLO rather than unconditional. --- docs/SyncOverAsync.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index c4c0302bf6..15f086e60d 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -127,6 +127,16 @@ Two caveats worth knowing before you enable it: very wide cluster, where connection count scales with the number of shards; - it is deliberately opt-in, and set process-wide at startup rather than per-connection. +How many connections that is depends on the protocol, which is worth checking before doing the arithmetic: + +- under **[RESP3](Resp3)**, pub/sub is multiplexed over the same connection, so it is one connection per node + — two threads per node. RESP3 is the default where the server supports it: the client negotiates with + `HELLO` and uses it unless you have pinned `Protocol` lower or the server is too old. +- under **RESP2**, pub/sub needs a *second* connection to each node, so there are two. That does not double + the threads, because subscription connections are deliberately left on the thread-pool — but it does mean + the flag covers your commands and **not** your pub/sub. If you are on RESP2 and it is pub/sub that is + stalling, moving to RESP3 is the more useful change. + 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 From efc862f661aab8cf9f6b939e0af870f42e114f88 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 12:13:41 +0100 Subject: [PATCH 13/17] Trim the RESP2 pub/sub aside back to the sizing fact The previous wording framed subscription connections staying on the thread-pool as a coverage gap - "the flag covers your commands and not your pub/sub" - and suggested moving to RESP3 if pub/sub stalls. That reads as a shortcoming, and it is not one: the failure this page describes needs a caller blocked waiting on a reply, and pub/sub is out-of-band delivery, so the dynamic does not apply. Leaning on it would send people chasing a protocol migration for a problem they do not have. What is left is the part that was actually useful: two threads per node either way, one connection per node under RESP3, two under RESP2 with the second not adding to the count - and why. --- docs/SyncOverAsync.md | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index 15f086e60d..ed048f4c4d 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -127,15 +127,10 @@ Two caveats worth knowing before you enable it: very wide cluster, where connection count scales with the number of shards; - it is deliberately opt-in, and set process-wide at startup rather than per-connection. -How many connections that is depends on the protocol, which is worth checking before doing the arithmetic: - -- under **[RESP3](Resp3)**, pub/sub is multiplexed over the same connection, so it is one connection per node - — two threads per node. RESP3 is the default where the server supports it: the client negotiates with - `HELLO` and uses it unless you have pinned `Protocol` lower or the server is too old. -- under **RESP2**, pub/sub needs a *second* connection to each node, so there are two. That does not double - the threads, because subscription connections are deliberately left on the thread-pool — but it does mean - the flag covers your commands and **not** your pub/sub. If you are on RESP2 and it is pub/sub that is - stalling, moving to RESP3 is the more useful change. +Budget two threads per node. Under **[RESP3](Resp3)** — the default where the server supports it, negotiated +via `HELLO` — pub/sub is multiplexed over the one connection to each node, so that is all there is. Under +**RESP2** there is a second connection per node carrying pub/sub, but it does not add to the count: those stay +on the thread-pool by design, because pub/sub is out-of-band delivery rather than a caller waiting on a reply. 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 From 9a9eeef6230087a9d9221fab49eb856ba910b435 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 12:15:02 +0100 Subject: [PATCH 14/17] Drop the protocol digression from the thread-cost caveat The paragraph explained that RESP2 has a second connection per node and that it does not add threads. Both true, and neither changes the number the reader came for: it is two threads per node on either protocol, because only the interactive connection takes them. Explaining a distinction that does not affect the answer just invites the question of whether it should have. The bullet now says "for each node you connect to", which is accurate on both protocols and needs no accompanying paragraph. --- docs/SyncOverAsync.md | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index ed048f4c4d..ca7f76ccb8 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -123,15 +123,10 @@ service while the real fix is made. It is not a licence to keep the blocking cal Two caveats worth knowing before you enable it: -- it costs a reader and a writer thread **per connection**, so think about it before enabling it against a - very wide cluster, where connection count scales with the number of shards; +- it costs a reader and a writer thread **for each node you connect to**, 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. -Budget two threads per node. Under **[RESP3](Resp3)** — the default where the server supports it, negotiated -via `HELLO` — pub/sub is multiplexed over the one connection to each node, so that is all there is. Under -**RESP2** there is a second connection per node carrying pub/sub, but it does not add to the count: those stay -on the thread-pool by design, because pub/sub is out-of-band delivery rather than a caller waiting on a reply. - 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 From 912c4eab6ef1705bddf0ac785e1684987359a7c7 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 12:15:46 +0100 Subject: [PATCH 15/17] Note that the thread cost excludes pub/sub connections Short of stating it, a reader sizing this against a RESP2 deployment has to guess whether the second connection to each node counts. It does not, and one parenthesis settles it without reopening the protocol discussion that the previous commit removed. --- docs/SyncOverAsync.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index ca7f76ccb8..b8b0e2dff7 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -123,8 +123,9 @@ service while the real fix is made. It is not a licence to keep the blocking cal Two caveats worth knowing before you enable it: -- it costs a reader and a writer thread **for each node you connect to**, so think about it before enabling it - against a very wide cluster, where that scales with the number of shards; +- it costs a reader and a writer thread **for each node you connect to** (not pub/sub connections, which stay + on the thread-pool), 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 From c99fed4b6414210e6f654eb0300e350e0efb0e9a Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 12:23:57 +0100 Subject: [PATCH 16/17] Test that DedicatedThreads actually takes effect, on both protocols The unit tests pinned the decision - which write mode a connection resolves to - and nothing pinned the consequence. That is the half that can rot silently: the policy could stay right while the plumbing under it stopped honouring the mode, and the docs would keep making a promise nothing checked. IsSyncReader/IsSyncWriter on IInternalConnectionMultiplexer answer it per endpoint and connection type, alongside the GetConnectionId that was already there in the same shape. They are named for what the library calls this internally - WriteMode.Sync, BufferedStreamWriter.IsSync, and threads named "SE.Redis Sync Reader"/"Sync Writer" - rather than for the flag that requests it, since the flag is a request and these report what happened. IsSyncReader mirrors the branching in StartReading rather than restating the rule, so a transport-backed connection correctly reports neither. The tests run per protocol, and the protocols differ in a way worth pinning: under RESP2 pub/sub is a second connection that stays on the thread-pool, and under RESP3 there is no second connection at all, so the subscription lookup returns the interactive one and reports dedicated. Asserting "pub/sub is not dedicated" on RESP3 would have been asserting against the shared connection. Each test builds an unshared connection inside the window where the flag is set - it is read at connect time, so a shared connection would answer for whenever it was made - and the class is non-parallel, since the flag is process-wide. Note the two directions validate each other: the same property is asserted false without the flag and true with it, so a stuck value fails one of them, and Assert.True/False(bool?) treats null as failure, so an absent connection cannot pass quietly either. --- .../ConnectionMultiplexer.FeatureFlags.cs | 20 ++++ .../Interfaces/IConnectionMultiplexer.cs | 9 ++ src/StackExchange.Redis/PhysicalBridge.cs | 3 + .../PhysicalConnection.Read.cs | 10 ++ .../PhysicalConnection.Write.cs | 9 ++ .../DedicatedThreadsTests.cs | 101 ++++++++++++++++++ .../Helpers/SharedConnectionFixture.cs | 6 ++ 7 files changed, 158 insertions(+) create mode 100644 tests/StackExchange.Redis.Tests/DedicatedThreadsTests.cs diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.FeatureFlags.cs b/src/StackExchange.Redis/ConnectionMultiplexer.FeatureFlags.cs index bab8d075d1..980b975456 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; @@ -72,4 +73,23 @@ public static bool GetFeatureFlag(string flag) 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 617a63f255..905bc4352e 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 6a8857a399..e478137860 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 e30102fe34..e659410216 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 f1bfad71a7..a47b6b5eb3 100644 --- a/src/StackExchange.Redis/PhysicalConnection.Write.cs +++ b/src/StackExchange.Redis/PhysicalConnection.Write.cs @@ -47,6 +47,15 @@ internal static BufferedStreamWriter.WriteMode ResolveWriteMode( 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; diff --git a/tests/StackExchange.Redis.Tests/DedicatedThreadsTests.cs b/tests/StackExchange.Redis.Tests/DedicatedThreadsTests.cs new file mode 100644 index 0000000000..b982e82890 --- /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/Helpers/SharedConnectionFixture.cs b/tests/StackExchange.Redis.Tests/Helpers/SharedConnectionFixture.cs index 8da84bce6b..27a676a5e4 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() From 72e0906103de94c2a39af8a3ed5e33a56cea6938 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 12:42:15 +0100 Subject: [PATCH 17/17] Be specific that the pub/sub exclusion is a RESP2 concern --- docs/SyncOverAsync.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/SyncOverAsync.md b/docs/SyncOverAsync.md index b8b0e2dff7..6726876190 100644 --- a/docs/SyncOverAsync.md +++ b/docs/SyncOverAsync.md @@ -123,9 +123,9 @@ service while the real fix is made. It is not a licence to keep the blocking cal Two caveats worth knowing before you enable it: -- it costs a reader and a writer thread **for each node you connect to** (not pub/sub connections, which stay - on the thread-pool), so think about it before enabling it against a very wide cluster, where that scales with - the number of shards; +- 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