From 2d3f182a454761d72894c2b06086979a557cbf73 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 17 Aug 2026 20:11:34 +0100 Subject: [PATCH 1/6] Fix: a retrying transaction did not pre-complete fire-and-forget RetryTransaction records each queued call and hands back a durable proxy task, completed later from whichever attempt succeeds. It did that regardless of the flags - which it could not see, since they are captured inside the generated per-command state - so a fire-and-forget operation stayed incomplete until ExecuteAsync forwarded it. A plain RedisTransaction hands back an *already-completed* task there: the caller has explicitly declined the reply, so there is nothing to wait for. The result was that identical caller code returned instantly on one and blocked for good on the other, which defeats the stated goal of letting ITransaction code move onto a retrying database unchanged. The generated captured-arguments struct now exposes its CommandFlags through IFlaggedRedisArgs - following IMappableRedisArgs, which exists for the same reason - and RetryTransaction settles the proxy up front when the flag is set. The operation is still recorded and replayed like any other: fire-and-forget declines the reply, not the command. Because the later forwarding uses TrySet*, it simply finds the proxy already settled, so a discarded attempt can no longer fault a result the caller was told not to expect. The value used is default(TResult) rather than the command's own "no reply" value, which lives inside the command implementation and is not reachable from the funnel - a distinction without a difference, since neither is an answer from the server. --- .../AutoDatabaseGenerator.cs | 34 ++- src/StackExchange.Redis/AutoDatabase.cs | 15 ++ .../Availability/RetryTransaction.cs | 28 +++ .../QueuedResultTests.cs | 203 ++++++++++++++++++ 4 files changed, 278 insertions(+), 2 deletions(-) create mode 100644 tests/StackExchange.Redis.Tests/QueuedResultTests.cs diff --git a/eng/StackExchange.Redis.Build/AutoDatabaseGenerator.cs b/eng/StackExchange.Redis.Build/AutoDatabaseGenerator.cs index a74e4e3e6..c80295733 100644 --- a/eng/StackExchange.Redis.Build/AutoDatabaseGenerator.cs +++ b/eng/StackExchange.Redis.Build/AutoDatabaseGenerator.cs @@ -434,7 +434,7 @@ int GetTupleIndex(BasicArray parameters, bool needsMap) return index; } - // const string CommandFlagsType = "StackExchange.Redis.CommandFlags"; + const string CommandFlagsType = "StackExchange.Redis.CommandFlags"; const string RedisKeyType = "StackExchange.Redis.RedisKey"; const string RedisChannelType = "StackExchange.Redis.RedisChannel"; // types that carry key(s) internally without "RedisKey" in their name, so the @@ -475,11 +475,32 @@ void AppendTupleTypes() } // captured args are a plain struct unless the owning database rewrites keys, in which - // case the mapping members - and the interface that carries them - are emitted below + // case the mapping members - and the interface that carries them - are emitted below. + // A captured CommandFlags is surfaced through IFlaggedRedisArgs regardless, so that a + // funnel holding an opaque TState can still see fire-and-forget; the two compose. writer.Append(")"); + int flagsArg = -1; + for (int p = 0; p < parameters.Length; p++) + { + // suffix rather than substring: this must not match a nullable, an array, or a + // container *of* flags, none of which the funnel could read as a plain value + if (parameters[p].Type.EndsWith(CommandFlagsType, StringComparison.Ordinal)) + { + flagsArg = p; + break; + } + } + + bool firstBase = true; if (cls.IsMutator) { writer.Append(" : global::StackExchange.Redis.IMappableRedisArgs"); + firstBase = false; + } + + if (flagsArg >= 0) + { + writer.Append(firstBase ? " : " : ", ").Append("global::StackExchange.Redis.IFlaggedRedisArgs"); } writer.NewLine().Append("{").Indent(); for (int p = 0; p < parameters.Length; p++) @@ -488,6 +509,15 @@ void AppendTupleTypes() .Append(" Arg").Append(p).Append(" = arg").Append(p).Append(";"); } + if (flagsArg >= 0) + { + // explicit implementation, so it cannot collide with a captured argument's own name; + // readonly only on a mutator's struct, since a readonly struct's members already are + writer.NewLine().Append(cls.IsMutator ? "readonly " : "") + .Append("global::StackExchange.Redis.CommandFlags global::StackExchange.Redis.IFlaggedRedisArgs.Flags => Arg") + .Append(flagsArg).Append(";"); + } + if (!cls.IsMutator) { // nothing rewrites keys here, so there is no Map/UnMapper to emit at all diff --git a/src/StackExchange.Redis/AutoDatabase.cs b/src/StackExchange.Redis/AutoDatabase.cs index 407a2e3fa..9d6195110 100644 --- a/src/StackExchange.Redis/AutoDatabase.cs +++ b/src/StackExchange.Redis/AutoDatabase.cs @@ -35,6 +35,21 @@ internal interface IMappableRedisArgs object? UnMapper { get; } } +// Implemented by a generated captured-arguments struct whenever the captured call has a CommandFlags +// parameter - which is very nearly all of them - so that a funnel holding an opaque TState can still ask the +// one question that changes what it must hand back: was this fire-and-forget? +// +// It exists for RetryTransaction. That funnel records each call and returns a durable proxy task completed +// later from the attempt, which is wrong for fire-and-forget: a plain RedisTransaction hands back an +// *already-completed* task there (the caller has explicitly declined the reply), so without this the same +// source awaiting the same result returns instantly on one and hangs on the other. The flags are captured +// inside TState and are otherwise invisible to a funnel, which is the whole reason this interface is needed +// rather than the funnel simply reading a parameter. +internal interface IFlaggedRedisArgs +{ + CommandFlags Flags { get; } +} + // The projections used by the auto-database funnels. There are exactly four real shapes, so each is named // rather than being expressed generically over the target/return type: that lets the type system encode the // invariants (sync goes to IDatabase, async goes to IDatabaseAsync and returns a Task) instead of leaving diff --git a/src/StackExchange.Redis/Availability/RetryTransaction.cs b/src/StackExchange.Redis/Availability/RetryTransaction.cs index 2ae8dacaf..e1cec855c 100644 --- a/src/StackExchange.Redis/Availability/RetryTransaction.cs +++ b/src/StackExchange.Redis/Availability/RetryTransaction.cs @@ -59,6 +59,7 @@ private Task ExecuteAsync(in TState state, AutoDatabas CheckNotExecuted(); var op = new RecordedOp(state, operation); _ops.Add(op); + if (IsFireAndForget(in state)) op.CompleteFireAndForget(); return op.Proxy; } @@ -68,9 +69,28 @@ private Task ExecuteAsync(in TState state, AutoDatabaseAsyncOperation(state, operation); _ops.Add(op); + if (IsFireAndForget(in state)) op.CompleteFireAndForget(); return op.Proxy; } + // A fire-and-forget operation must hand back an already-completed task, because that is what a plain + // RedisTransaction does (see its ExecuteAsync: F+F never gets a TaskCompletionSource at all). Without this + // the durable proxy stays incomplete until Execute, so identical caller code returns instantly on a plain + // transaction and blocks for good on a retrying one - which defeats this class's stated goal of letting + // ITransaction code move onto a retrying database unchanged. + // + // The op is still recorded and replayed like any other: fire-and-forget declines the *reply*, not the + // command. Completing the proxy up front simply means the later ForwardSuccess/Fault - which use TrySet* - + // find it already settled and do nothing, so a discarded attempt can never fault a result the caller was + // told not to expect. + // + // The flags are captured inside TState, so this is the one thing the generated struct is asked to expose + // (IFlaggedRedisArgs). The `is` test boxes, once per queued operation; that is noise beside the RecordedOp + // and TaskCompletionSource this method already allocates, and it only happens on a retrying transaction. + private static bool IsFireAndForget(in TState state) + where TState : struct + => state is IFlaggedRedisArgs flagged && (flagged.Flags & CommandFlags.FireAndForget) != 0; + public ConditionResult AddCondition(Condition condition) { if (condition is null) throw new ArgumentNullException(nameof(condition)); @@ -228,6 +248,11 @@ public RecordedOp(in TState state, AutoDatabaseAsyncOperation o public Task Proxy => _proxy.Task; + // settled up front for fire-and-forget; see IsFireAndForget. default(TResult) rather than the + // command's own "no reply" value, which lives inside the command implementation and is not reachable + // from here - a distinction without a difference, since neither is an answer from the server + public void CompleteFireAndForget() => _proxy.TrySetResult(default!); + public void Replay(IDatabaseAsync inner) => _attempt = _operation(in _state, inner); public void ForwardSuccess() @@ -285,6 +310,9 @@ public RecordedVoidOp(in TState state, AutoDatabaseAsyncOperation operat public Task Proxy => _proxy.Task; + // see the note on RecordedOp.CompleteFireAndForget + public void CompleteFireAndForget() => _proxy.TrySetResult(true); + public void Replay(IDatabaseAsync inner) => _attempt = _operation(in _state, inner); public void ForwardSuccess() diff --git a/tests/StackExchange.Redis.Tests/QueuedResultTests.cs b/tests/StackExchange.Redis.Tests/QueuedResultTests.cs new file mode 100644 index 000000000..20c389ed3 --- /dev/null +++ b/tests/StackExchange.Redis.Tests/QueuedResultTests.cs @@ -0,0 +1,203 @@ +using System; +using System.Threading.Tasks; +using StackExchange.Redis.Availability; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// What the task handed back by a command queued on an or +/// actually does before the batch is sent. +/// +/// +/// +/// These are the facts the SER305/SER306 analyzer rules are built on, which is why they are pinned here rather +/// than left as received wisdom: awaiting a queued command before Execute hangs forever, *except* under +/// , where the task is already completed and carries nothing. +/// +/// +/// The waits below are deliberately asymmetric. Asserting a task *is* complete is exact and instant; asserting +/// one never completes cannot be, so it is a bounded wait - long enough that a passing run means something, +/// short enough not to dominate the suite. A false pass there would mean the rule is unnecessary, not that it +/// is wrong. +/// +/// +public class QueuedResultTests(ITestOutputHelper output, SharedConnectionFixture fixture) : TestBase(output, fixture) +{ + /// How long to wait before accepting that a task is not going to complete. + private static readonly TimeSpan NeverCompletes = TimeSpan.FromSeconds(2); + + [Fact] + public async Task TransactionQueuedCommandDoesNotCompleteBeforeExecute() + { + await using var conn = Create(); + + RedisKey key = Me(); + var db = conn.GetDatabase(); + db.StringSet(key, "abc", flags: CommandFlags.FireAndForget); + + var tran = db.CreateTransaction(); + var pending = tran.StringGetAsync(key); + + // the whole point of the rule: this is what awaiting here would do + Assert.False(pending.IsCompleted); + Assert.NotSame(pending, await Task.WhenAny(pending, Task.Delay(NeverCompletes, TestContext.Current.CancellationToken))); + Assert.False(pending.IsCompleted); + + Assert.True(await tran.ExecuteAsync()); + Assert.Equal("abc", await pending); + } + + [Fact] + public async Task TransactionQueuedFireAndForgetCompletesImmediatelyWithDefault() + { + await using var conn = Create(); + + RedisKey key = Me(); + var db = conn.GetDatabase(); + db.StringSet(key, "abc", flags: CommandFlags.FireAndForget); + + var tran = db.CreateTransaction(); + var pending = tran.StringGetAsync(key, CommandFlags.FireAndForget); + + // completed before anything has been sent, so awaiting it is legal - but the value is not the + // server's answer and never will be, whenever it is awaited + Assert.True(pending.IsCompleted); + Assert.True((await pending).IsNull); + + Assert.True(await tran.ExecuteAsync()); + Assert.True((await pending).IsNull); + } + + [Fact] + public async Task TransactionQueuedFireAndForgetStillExecutes() + { + await using var conn = Create(); + + RedisKey key = Me(); + var db = conn.GetDatabase(); + db.KeyDelete(key, CommandFlags.FireAndForget); + + var tran = db.CreateTransaction(); + var pending = tran.StringSetAsync(key, "written", flags: CommandFlags.FireAndForget); + Assert.True(pending.IsCompleted); + + // the discarded result is the only thing fire-and-forget gives up; the command itself is queued and + // sent like any other, so "it completed early" must not be read as "it did not happen" + Assert.True(await tran.ExecuteAsync()); + Assert.Equal("written", await db.StringGetAsync(key)); + } + + /// + /// A retrying transaction pre-completes a fire-and-forget operation, exactly as the plain one does. + /// + /// + /// + /// This is a regression test for a real divergence. RetryTransaction records every queued call and + /// hands back a durable proxy task; it used to do so without regard to the flags - which it could not see, + /// as they are captured inside the generated per-command state - so a fire-and-forget result stayed + /// incomplete until ExecuteAsync forwarded it. + /// + /// + /// The effect was that identical caller code returned instantly on a plain transaction and blocked for + /// good on a retrying one, which is precisely the drop-in substitution WithRetry is meant to + /// support. The generated state now exposes its CommandFlags (IFlaggedRedisArgs) so the two + /// agree. + /// + /// + [Fact] + public async Task RetryTransactionQueuedFireAndForgetCompletesImmediatelyWithDefault() + { + await using var conn = Create(); + + RedisKey key = Me(); + var db = conn.GetDatabase(); + db.StringSet(key, "abc", flags: CommandFlags.FireAndForget); + + var tran = db.WithRetry().CreateTransaction(); + var pending = tran.StringGetAsync(key, CommandFlags.FireAndForget); + + Assert.True(pending.IsCompleted); + Assert.True((await pending).IsNull); + + Assert.True(await tran.ExecuteAsync()); + Assert.True((await pending).IsNull); + } + + /// ...and the command itself is still queued, replayed and sent. + [Fact] + public async Task RetryTransactionQueuedFireAndForgetStillExecutes() + { + await using var conn = Create(); + + RedisKey key = Me(); + var db = conn.GetDatabase(); + db.KeyDelete(key, CommandFlags.FireAndForget); + + var tran = db.WithRetry().CreateTransaction(); + var pending = tran.StringSetAsync(key, "written", flags: CommandFlags.FireAndForget); + Assert.True(pending.IsCompleted); + + Assert.True(await tran.ExecuteAsync()); + Assert.Equal("written", await db.StringGetAsync(key)); + } + + /// The non-fire-and-forget case, which behaves the same on both. + [Fact] + public async Task RetryTransactionQueuedCommandDoesNotCompleteBeforeExecute() + { + await using var conn = Create(); + + RedisKey key = Me(); + var db = conn.GetDatabase(); + db.StringSet(key, "abc", flags: CommandFlags.FireAndForget); + + var tran = db.WithRetry().CreateTransaction(); + var pending = tran.StringGetAsync(key); + + Assert.False(pending.IsCompleted); + Assert.NotSame(pending, await Task.WhenAny(pending, Task.Delay(NeverCompletes, TestContext.Current.CancellationToken))); + + Assert.True(await tran.ExecuteAsync()); + Assert.Equal("abc", await pending); + } + + [Fact] + public async Task BatchQueuedCommandDoesNotCompleteBeforeExecute() + { + await using var conn = Create(); + + RedisKey key = Me(); + var db = conn.GetDatabase(); + db.StringSet(key, "abc", flags: CommandFlags.FireAndForget); + + var batch = db.CreateBatch(); + var pending = batch.StringGetAsync(key); + + Assert.False(pending.IsCompleted); + Assert.NotSame(pending, await Task.WhenAny(pending, Task.Delay(NeverCompletes, TestContext.Current.CancellationToken))); + Assert.False(pending.IsCompleted); + + batch.Execute(); + Assert.Equal("abc", await pending); + } + + [Fact] + public async Task BatchQueuedFireAndForgetCompletesImmediatelyWithDefault() + { + await using var conn = Create(); + + RedisKey key = Me(); + var db = conn.GetDatabase(); + db.StringSet(key, "abc", flags: CommandFlags.FireAndForget); + + var batch = db.CreateBatch(); + var pending = batch.StringGetAsync(key, CommandFlags.FireAndForget); + + Assert.True(pending.IsCompleted); + Assert.True((await pending).IsNull); + + batch.Execute(); + Assert.True((await pending).IsNull); + } +} From 93f536bcac5eb1261c8fcca172b14a8ba9b7637c Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 17 Aug 2026 20:11:34 +0100 Subject: [PATCH 2/6] Add SER305/SER306: waiting for a queued transaction result Commands queued on an ITransaction or IBatch are buffered, not sent; the task comes back incomplete and stays that way until Execute[Async]. So awaiting one at the point of queueing waits for something that can only happen further down the method, and the caller hangs for good. This is the mistake "always await your tasks" actively teaches, and it fails silently, so SER305 is an error rather than a suggestion. It can be, without hedging, because the shape is certain by construction: the wait sits at the queueing site, so no arrangement of the surrounding code rescues it - even a prior Execute is no help, since this command was queued after it. That is also why the flow-sensitive shapes (a task captured to a local, or gathered by Task.WhenAll, then awaited) are deliberately not reported: awaiting a captured task *after* Execute is the recommended fix, and an error that gets the order wrong condemns correct code. SER306 is the one case that survives the wait. Fire-and-forget hands back an already-completed task carrying the default value, so awaiting it is legal - it just never carries the server's answer, whenever it is read. Separate ID and a warning rather than an error, so that deliberate fire-and-forget code can silence it without silencing the rule that says a transaction cannot work. Two decisions that were not obvious: - terminators are recognised by *declaring interface*, never by name. ITransaction[Async].ExecuteAsync(CommandFlags) must be awaited, while IDatabaseAsync.ExecuteAsync(string, ...) - the raw-command escape hatch - is an ordinary queued command that must still be reported; a name match gets both exactly the wrong way round. The receiver is not always the interface either: a decorator class implementing it resolves to the class's member, which this repo's own build caught on KeyPrefixedTransaction. - a non-constant flags expression is read only far enough to prove FireAndForget *present* (`flags | CommandFlags.FireAndForget` - `|` only sets bits). There is deliberately no matching treatment of &/~ to prove it absent: that would feed SER305, and an error must not rest on an expression we only partly understood. The code fixes live in a new StackExchange.Redis.CodeFixes assembly, packed alongside the analyzer. Separate because a fix needs Workspaces, which csc does not ship and the IDE does - what RS1038 exists to say, though the pinned analyzer version predates that rule, so the layout is chosen rather than prompted. --- Directory.Packages.props | 3 + StackExchange.Redis.slnx | 1 + docs/Transactions.md | 32 ++ docs/rules/SER305.md | 84 +++++ docs/rules/SER306.md | 65 ++++ docs/rules/index.md | 26 +- .../AnalyzerReleases.Unshipped.md | 11 +- eng/StackExchange.Redis.Build/Diagnostics.cs | 80 +++- .../QueuedResultAnalyzer.cs | 343 ++++++++++++++++++ .../QueuedResultCodeFixProvider.cs | 251 +++++++++++++ .../StackExchange.Redis.CodeFixes.csproj | 30 ++ .../StackExchange.Redis.csproj | 21 +- .../CodeFixVerifier.cs | 73 ++++ .../StackExchange.Redis.Build.Tests/SER305.cs | 330 +++++++++++++++++ .../SER305CodeFix.cs | 182 ++++++++++ .../StackExchange.Redis.Build.Tests/SER306.cs | 174 +++++++++ .../StackExchange.Redis.Build.Tests.csproj | 4 + .../TestSetup.cs | 75 ++++ .../Verifier.cs | 53 +-- 19 files changed, 1773 insertions(+), 65 deletions(-) create mode 100644 docs/rules/SER305.md create mode 100644 docs/rules/SER306.md create mode 100644 eng/StackExchange.Redis.Build/QueuedResultAnalyzer.cs create mode 100644 eng/StackExchange.Redis.CodeFixes/QueuedResultCodeFixProvider.cs create mode 100644 eng/StackExchange.Redis.CodeFixes/StackExchange.Redis.CodeFixes.csproj create mode 100644 tests/StackExchange.Redis.Build.Tests/CodeFixVerifier.cs create mode 100644 tests/StackExchange.Redis.Build.Tests/SER305.cs create mode 100644 tests/StackExchange.Redis.Build.Tests/SER305CodeFix.cs create mode 100644 tests/StackExchange.Redis.Build.Tests/SER306.cs create mode 100644 tests/StackExchange.Redis.Build.Tests/TestSetup.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index 66df6c65b..234266bb1 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -33,6 +33,9 @@ + + diff --git a/StackExchange.Redis.slnx b/StackExchange.Redis.slnx index f981b8ef0..8b5b222aa 100644 --- a/StackExchange.Redis.slnx +++ b/StackExchange.Redis.slnx @@ -1,6 +1,7 @@ + diff --git a/docs/Transactions.md b/docs/Transactions.md index 7f78db126..62303be57 100644 --- a/docs/Transactions.md +++ b/docs/Transactions.md @@ -117,6 +117,38 @@ var wasSet = (bool) db.ScriptEvaluate(@"if redis.call('hexists', KEYS[1], 'Uniqu (note that the response from `ScriptEvaluate` and `ScriptEvaluateAsync` is variable depending on your exact script; the response can be interpreted by casting - in this case as a `bool`) +Don't await inside the transaction +--- + +This is the most common mistake with transactions, and it does not fail loudly - it hangs. + +Commands queued on an `ITransaction` (or an `IBatch`) are *buffered*, not sent. They go to the server only when +you call `Execute`/`ExecuteAsync`, so the `Task` handed back at the point of queueing cannot complete until +after that call. Awaiting it there waits for something that can only happen further down the method: + +```csharp +var tran = db.CreateTransaction(); +var value = await tran.StringGetAsync(key); // never completes - nothing has been sent yet +await tran.ExecuteAsync(); // never reached +``` + +Everything else in the async API rewards awaiting promptly, and `ITransaction` offers the same method names, so +this reads as perfectly ordinary code. Instead, capture the tasks and await them *after* `Execute`: + +```csharp +var tran = db.CreateTransaction(); +var pending = tran.StringGetAsync(key); +_ = tran.StringSetAsync(other, "value"); // discard the ones you do not need +if (await tran.ExecuteAsync()) +{ + var value = await pending; +} +``` + +The analyzer reports this as [SER305](rules/SER305) - an **error**, not a suggestion - and offers both fixes. +The one case that does not hang is `CommandFlags.FireAndForget`, which completes immediately carrying the +default value; awaiting that is legal but tells you nothing, which is [SER306](rules/SER306). + Do you need a transaction at all? --- diff --git a/docs/rules/SER305.md b/docs/rules/SER305.md new file mode 100644 index 000000000..1137971f5 --- /dev/null +++ b/docs/rules/SER305.md @@ -0,0 +1,84 @@ +# SER305: waiting for a queued command before it is executed will never complete + +Commands queued on an `ITransaction` or `IBatch` are **not sent** when you call them - they are buffered, and +go to the server only when you call `Execute`/`ExecuteAsync`. The `Task` you get back therefore cannot complete +until then, so awaiting it at the point of queueing waits for something that can only happen after the line +that is waiting. Your code stops there, permanently. + +```c# +// flagged - this hangs, for good +var tran = db.CreateTransaction(); +var value = await tran.StringGetAsync(key); // SER305: nothing has been sent yet +await tran.ExecuteAsync(); // never reached + +// suggested - capture the task, await it after Execute +var tran = db.CreateTransaction(); +var pending = tran.StringGetAsync(key); +await tran.ExecuteAsync(); +var value = await pending; + +// or, if you do not want the result at all +_ = tran.StringSetAsync(key, value); +await tran.ExecuteAsync(); +``` + +This is the one rule here reported as an **error**, because unlike the rest of the family it does not describe +code that could be better - it describes code that cannot work. There is no arrangement of the surrounding +lines that rescues it: the wait sits at the queueing site, so even a previous `Execute` does not help, because +*this* command was queued after it and will never be sent. + +It applies equally to a batch (`db.CreateBatch()`), which buffers in exactly the same way, and to every way of +waiting - `await`, `.Result`, `.Wait()`, `.GetAwaiter().GetResult()`. + +## Why this happens so easily + +It is the one place where the advice everyone has absorbed - *await your tasks, and await them promptly* - is +exactly wrong. Every other `IDatabaseAsync` method returns a task you should await straight away, and +`ITransaction` offers the same method names on what looks like the same interface. Nothing about the call site +suggests that this one is a buffer rather than a dispatch. + +## Fixes + +Two are offered in the IDE: + +- **Discard the queued result** - `_ = tran.StringSetAsync(...)`. Right when you only wanted the write. +- **Capture it and await after Execute** - introduces a local for the task and moves the `await` below the + `Execute[Async]` call. + +The second is only offered where it is unambiguously safe: the `await` form, with an `Execute[Async]` on the +same transaction later in the same block, and - if the wait declares a variable - nothing between the two +already using that variable, since the declaration is what moves. + +## Cases that are deliberately not flagged + +- **A task captured to a local, then awaited.** `var pending = tran.StringGetAsync(key); await pending;` is + broken if it is above the `Execute` and *correct* if it is below - it is the recommended fix. Telling those + apart means reasoning about statement order, and this rule is an error: it must not guess. The same goes for + `await Task.WhenAll(...)` over queued tasks. +- **Fire-and-forget.** `CommandFlags.FireAndForget` returns an already-completed task, so awaiting it does not + hang. It is still not giving you an answer, which is [SER306](SER306) - a warning, not an error. +- **Flags this rule cannot read.** `tran.StringGetAsync(key, flags)`, where `flags` is a variable, might carry + `FireAndForget` at runtime, so nothing is reported at all. +- **A transaction reached through `IDatabaseAsync`.** A helper taking `IDatabaseAsync` cannot tell a + transaction from a plain database, and neither can this rule. + +## Suppressing + +Reported as an **error**, so it fails the build. If you are suppressing it, please check first that the code +really does complete - this rule fires only on shapes that cannot. + +```xml +$(NoWarn);SER305 +``` + +or locally: + +```c# +#pragma warning disable SER305 +``` + +If you believe it has flagged something that works, that is a bug in the rule and one that reaches every +consumer of the package - please +[report it](https://github.com/StackExchange/StackExchange.Redis/issues/new) with the code as written. + +See also [Transactions](../Transactions) and [SER306](SER306). diff --git a/docs/rules/SER306.md b/docs/rules/SER306.md new file mode 100644 index 000000000..a97f8b342 --- /dev/null +++ b/docs/rules/SER306.md @@ -0,0 +1,65 @@ +# SER306: waiting for a fire-and-forget result yields nothing + +`CommandFlags.FireAndForget` means "send this, and do not report back". The command is queued and sent like any +other, but the task you get is **already completed, carrying the default value** - it is not, and never will +be, the server's answer. Reading it is not an error, it is just always `null`/`0`/`false`. + +```c# +// flagged - value is always RedisValue.Null, whenever you read it +var tran = db.CreateTransaction(); +var value = await tran.StringGetAsync(key, CommandFlags.FireAndForget); +await tran.ExecuteAsync(); + +// suggested - if you want the result, do not ask for fire-and-forget +var pending = tran.StringGetAsync(key); +await tran.ExecuteAsync(); +var value = await pending; + +// or, if fire-and-forget is what you meant, discard it +_ = tran.StringSetAsync(key, value, flags: CommandFlags.FireAndForget); +await tran.ExecuteAsync(); +``` + +Unlike [SER305](SER305) this does not hang, which is why it is a **warning** rather than an error, and why it +has its own ID: deliberate fire-and-forget code needs to be able to silence this without silencing the rule +that says a transaction cannot work. + +The only fix offered is **discard the queued result**. Capturing the task and awaiting it after `Execute` - +which is the fix for SER305 - gains nothing here, because the value does not improve with waiting. + +## Where it applies + +Anywhere the flag is visible at compile time, on a transaction or a batch: + +```c# +tran.StringGetAsync(key, CommandFlags.FireAndForget) // flagged +tran.StringGetAsync(key, CommandFlags.FireAndForget | CommandFlags.DemandMaster) // flagged +tran.StringGetAsync(key, flags | CommandFlags.FireAndForget) // flagged - `|` only sets bits +tran.StringGetAsync(key, flags) // not flagged - unknowable +``` + +The third case works because `|` can only ever *add* the flag, whatever `flags` holds at runtime. There is no +matching treatment of `&`/`~` to prove the flag *absent*: that would feed [SER305](SER305), which is an error, +and an error must not rest on a partly-understood expression. + +## Cases that are deliberately not flagged + +- **Fire-and-forget straight to the database.** `db.StringSetAsync(key, value, flags: FireAndForget)` is + ordinary code; this rule is only about waiting for a result you have declined. +- **A discarded result.** `_ = tran.StringSetAsync(...)` is exactly what the rule asks for. + +## Suppressing + +Reported as a **warning**, so `TreatWarningsAsErrors` builds fail until you act on it or turn it down. + +```xml +$(NoWarn);SER306 +``` + +or locally: + +```c# +#pragma warning disable SER306 +``` + +See also [Transactions](../Transactions) and [SER305](SER305). diff --git a/docs/rules/index.md b/docs/rules/index.md index 722fbb0fb..452ada41b 100644 --- a/docs/rules/index.md +++ b/docs/rules/index.md @@ -8,7 +8,7 @@ separately: | Range | Meaning | |---|---| -| `SER300`-`SER349` | usage guidance about your code | +| `SER300`-`SER349` | rules about your code - mostly guidance, but see the correctness rules below | | `SER350`-`SER399` | build-level problems from the source generators | Note that `SER0xx` is a different thing entirely: those are the [`[Experimental]` API gates](../exp/SER004), @@ -28,6 +28,13 @@ want `StringSet`. Reach for the async form in new code. Argument names in the suggestion (`key`, `value`, `entries`) are a sketch of the shape, not literal text - substitute your own expressions. +## Correctness + +Unlike everything under [Usage](#usage), these describe code that does not do what it looks like it does. + +- [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 + ## Usage - [SER300](SER300) - transaction may be replaceable by a conditional argument (any server version) @@ -70,7 +77,11 @@ These are heuristics, and the list above is where the effort has gone - but it i rare, not impossible. If one of these rules flags something it should not have, that is a bug in the rule rather than something to work around: please [report it](https://github.com/StackExchange/StackExchange.Redis/issues/new) with the transaction as written. -`SER350` is not in this family - it reports a build problem rather than offering guidance. + +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 +caveats above (a loop, an `if`, a transaction passed elsewhere) simply do not arise. ## Declaring your server version @@ -94,10 +105,13 @@ rule's message names the version it needs, so you can tell at a glance whether i ## Severity, and turning it down -These are **warnings** by default. The code they flag is correct - it works, and it will keep working - so a -warning is arguably strong; they are warnings anyway because information-level diagnostics are not printed by -`dotnet build`, which means outside an IDE they are invisible, and a suggestion nobody ever sees is not worth -shipping. +These are **warnings** by default, with one exception: [SER305](SER305) is an **error**, because the code it +flags cannot work rather than merely being improvable. It is the only one here that is not a matter of taste, +and it is worth reading before suppressing. + +For the rest, the code they flag is correct - it works, and it will keep working - so a warning is arguably +strong; they are warnings anyway because information-level diagnostics are not printed by `dotnet build`, which +means outside an IDE they are invisible, and a suggestion nobody ever sees is not worth shipping. The consequence worth knowing before you upgrade: if you build with `TreatWarningsAsErrors`, these **will fail your build** on code that previously compiled. Nothing is broken - you have a choice of acting on them or diff --git a/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md index b9d09f18c..abbe418de 100644 --- a/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md +++ b/eng/StackExchange.Redis.Build/AnalyzerReleases.Unshipped.md @@ -4,5 +4,12 @@ ; released, because consumers put them in NoWarn and .editorconfig. See Diagnostics.cs for the SER3xx map. ; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md ; -; Empty: the initial set is recorded directly in AnalyzerReleases.Shipped.md under 3.1. New rules added after -; that release go here first, under a "### New Rules" table, and move across when they ship. +; The initial set is recorded directly in AnalyzerReleases.Shipped.md under 3.1. New rules added after that +; release go here first, and move across when they ship. + +### New Rules + +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 diff --git a/eng/StackExchange.Redis.Build/Diagnostics.cs b/eng/StackExchange.Redis.Build/Diagnostics.cs index b930b125f..b627fa4ae 100644 --- a/eng/StackExchange.Redis.Build/Diagnostics.cs +++ b/eng/StackExchange.Redis.Build/Diagnostics.cs @@ -20,8 +20,8 @@ namespace StackExchange.Redis.Build; /// them in NoWarn and .editorconfig. /// /// -/// Everything here defaults to , including the usage rules, whose code -/// is correct rather than broken. That is a deliberate change from an earlier or above, including the suggestion +/// rules, whose code is correct rather than broken. That is a deliberate change from an earlier default: information-level diagnostics are not printed by dotnet /// build at all, so outside an IDE the rules simply did not exist, and a suggestion nobody sees is not /// worth shipping. The cost is real and should be understood rather than discovered: a consumer building with @@ -30,10 +30,18 @@ namespace StackExchange.Redis.Build; /// experience is a broken build, and that is the trade being made on purpose. /// /// -/// Which is also why the usage rules hedge - "may be replaceable", "looks like", "consider" - rather than -/// asserting. They are heuristics over source text, so a false positive is rare rather than impossible, and -/// arriving as a warning already overstates the case; wording them as findings of fact would overstate it -/// twice. The build-level does not hedge, because it is not guessing. +/// Which is also why the *suggestion* rules hedge - "may be replaceable", "looks like", "consider" - rather +/// than asserting. They are heuristics over source text, so a false positive is rare rather than impossible, +/// and arriving as a warning already overstates the case; wording them as findings of fact would overstate it +/// twice. +/// +/// +/// Two rules here are not of that kind and are worded flatly, because they are not guessing: the build-level +/// , and - the only in the set - which describes code that cannot work rather than code that +/// could be better. Severity here is a statement about certainty, so keep the two kinds apart when adding an +/// ID: an error that can be wrong is a broken build on correct code, which is a far worse trade than a +/// warning that can be wrong. /// /// internal static class Diagnostics @@ -151,6 +159,66 @@ internal static class Diagnostics description: "The same command queued several times over can usually be a single variadic call, which is one round-trip and needs no transaction to be atomic.", helpLinkUri: HelpLink("SER304")); + /// + /// Waiting on a command queued to a transaction or batch, before anything has been sent. + /// + /// + /// + /// The one in the set, and the only rule here that describes broken + /// code rather than improvable code. A command queued on an ITransaction/IBatch is not sent + /// until Execute[Async], so the task it hands back cannot complete before then: awaiting it at the + /// point of queueing deadlocks the caller for good. + /// + /// + /// It can be an error without hedging because there is no arrangement of the surrounding code that makes it + /// work. The wait sits at the queueing site, so even a prior Execute is no help - *this* command was + /// queued after it and will never be sent. That is what makes it flow-insensitive, and so certain; the + /// shapes that would need ordering to judge (a task captured to a local, or gathered by Task.WhenAll, + /// and then awaited) are deliberately not reported, because awaiting a captured task *after* Execute + /// is the recommended fix and getting the order wrong would error on correct code. + /// + /// + /// is the one case that survives the wait, and is split off rather + /// than excluded silently. + /// + /// + public static readonly DiagnosticDescriptor AwaitBeforeExecute = new( + id: "SER305", + title: "Waiting for a queued command before it is executed will never complete", + messageFormat: "'{0}' is queued on {1} and is not sent until Execute[Async](), so waiting for its result here will never complete; capture the task and await it after Execute[Async](), or discard it", + category: UsageCategory, + defaultSeverity: DiagnosticSeverity.Error, + isEnabledByDefault: true, + description: "Commands queued on a transaction or batch are only sent when Execute[Async]() is called, so the task returned at the point of queueing cannot complete until then; awaiting or blocking on it there deadlocks.", + helpLinkUri: HelpLink("SER305")); + + /// + /// As , but with CommandFlags.FireAndForget, which does not deadlock. + /// + /// + /// + /// Fire-and-forget hands back an already-completed task carrying the default value (see + /// RedisTransaction.ExecuteAsync), so this one is legal: it returns immediately, and the command is + /// still queued and sent by Execute like any other. What it cannot do is produce the server's answer + /// - not later, not after Execute, not ever - so the value read from it is always default. + /// + /// + /// Hence a warning rather than an error, and its own ID: somebody using fire-and-forget deliberately needs + /// to be able to silence this without silencing , which is the one that says + /// their code cannot work. It is also the only rule here whose advice is *only* "discard it" - capturing the + /// task to await after Execute is the fix for the other one and gains nothing at all here. + /// + /// + 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", + 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")); + /// /// 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 new file mode 100644 index 000000000..cd5f38aed --- /dev/null +++ b/eng/StackExchange.Redis.Build/QueuedResultAnalyzer.cs @@ -0,0 +1,343 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +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). +/// +/// +/// +/// 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. +/// +/// +/// The shapes covered are the ones that are certain without any flow analysis, because the wait is written at +/// the queueing site and so cannot be reached after an Execute of that same command: +/// +/// +/// await tran.StringGetAsync(key) +/// tran.StringGetAsync(key).Result +/// tran.StringGetAsync(key).Wait(), and .GetAwaiter().GetResult() +/// +/// +/// A task captured to a local first is out of scope on purpose: awaiting one *after* Execute is the +/// recommended fix, so telling the two apart needs ordering, and an error that gets ordering wrong condemns +/// correct code. The same goes for Task.WhenAll(...) over queued commands. +/// +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class QueuedResultAnalyzer : DiagnosticAnalyzer +{ + /// + public override ImmutableArray SupportedDiagnostics { get; } + = ImmutableArray.Create( + Diagnostics.AwaitBeforeExecute, + Diagnostics.AwaitFireAndForgetResult); + + /// + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + + // as TransactionAnalyzer: resolve once per compilation and bail, so the overwhelming majority of + // compilations - which have never heard of this library - pay a couple of metadata lookups and nothing + context.RegisterCompilationStartAction(static ctx => + { + if (KnownSymbols.TryCreate(ctx.Compilation) is not { } known) return; + + // every shape we report is an await or a blocking read written directly around the queued call, so + // the operation kinds are known up front and there is no need to walk whole blocks + ctx.RegisterOperationAction(opCtx => Analyze(opCtx, known), OperationKind.Await, OperationKind.Invocation, OperationKind.PropertyReference); + }); + } + + private static void Analyze(OperationAnalysisContext context, KnownSymbols known) + { + // what is being waited for, if this operation waits for anything at all + if (Waited(context.Operation, known) is not { } waited) 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 descriptor = flags == FireAndForgetState.Set + ? Diagnostics.AwaitFireAndForgetResult + : Diagnostics.AwaitBeforeExecute; + + // 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. + context.ReportDiagnostic(Diagnostic.Create( + descriptor, + context.Operation.Syntax.GetLocation(), + additionalLocations: new[] { queued.Syntax.GetLocation() }, + properties: null, + queued.TargetMethod.Name, + Describe(queued.Instance?.Type, known))); + } + + /// + /// The operation whose result this one waits for, if it is one of the waiting shapes. + /// + /// + /// 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. + /// + private static IOperation? Waited(IOperation operation, KnownSymbols known) => operation switch + { + // await tran.StringGetAsync(key) + IAwaitOperation await => Unwrap(await.Operation), + + // tran.StringGetAsync(key).Result + IPropertyReferenceOperation { Property.Name: "Result", Instance: { } instance } when known.IsTask(instance.Type) + => Unwrap(instance), + + // tran.StringGetAsync(key).Wait() + IInvocationOperation { TargetMethod.Name: "Wait", Instance: { } instance } when known.IsTask(instance.Type) + => Unwrap(instance), + + // tran.StringGetAsync(key).GetAwaiter().GetResult() + IInvocationOperation { TargetMethod.Name: "GetResult", Instance: IInvocationOperation { TargetMethod.Name: "GetAwaiter", Instance: { } instance } } + => Unwrap(instance), + + _ => null, + }; + + /// + /// Strips the conversions the compiler may have inserted around the awaited expression. + /// + private static IOperation Unwrap(IOperation operation) + { + while (operation is IConversionOperation { IsImplicit: true, Operand: { } operand }) operation = operand; + return operation; + } + + /// Whether the call carries CommandFlags.FireAndForget, if that can be known. + private enum FireAndForgetState + { + /// The flags are known and do not include it: waiting here can never complete. + Clear, + + /// The flags are known to include it: waiting completes, with nothing in it. + Set, + + /// The flags are not a compile-time constant, so neither rule can be claimed. + Unknown, + } + + private static FireAndForgetState FireAndForget(IInvocationOperation invocation, KnownSymbols known) + { + if (known.FireAndForgetValue is not { } fireAndForget) return FireAndForgetState.Unknown; + + foreach (var argument in invocation.Arguments) + { + if (!known.IsCommandFlags(argument.Parameter?.Type)) continue; + + // An omitted argument is the parameter's default, which is CommandFlags.None throughout the API; + // ConstantValue reports that for us rather than it being assumed here. Enums arrive as their + // underlying value, and CommandFlags is int-backed; "A | B" over two constants is itself a + // constant, so the fully-spelled-out combined forms land here too. + var value = Unwrap(argument.Value); + if (value.ConstantValue is { HasValue: true, Value: int constant }) + { + return (constant & fireAndForget) != 0 ? FireAndForgetState.Set : FireAndForgetState.Clear; + } + + return CarriesFireAndForget(value, fireAndForget) ? FireAndForgetState.Set : FireAndForgetState.Unknown; + } + + // no flags parameter at all: nothing can have asked for fire-and-forget + return FireAndForgetState.Clear; + } + + /// + /// Whether a flags expression that is not a constant is nonetheless certain to carry fire-and-forget. + /// + /// + /// + /// Only the obvious shape - flags | CommandFlags.FireAndForget, and chains of it - because | + /// can only ever *set* bits, so a constant operand carrying the flag settles the question whatever the rest + /// of the expression turns out to hold. Anything less obvious is left as unknown, which is silence; this is + /// deliberately not an expression evaluator. + /// + /// + /// One-directional on purpose, and it must stay that way. There is no matching treatment of & + /// or ~ to prove the flag *absent*, tempting as the symmetry looks: concluding "set" only ever + /// produces , a warning, whereas concluding "clear" + /// produces - an error, on a build, from an expression we only + /// partly understood. The cheap half of this problem is also the safe half. + /// + /// + private static bool CarriesFireAndForget(IOperation operation, int fireAndForget) + { + operation = Unwrap(operation); + + if (operation.ConstantValue is { HasValue: true, Value: int value }) return (value & fireAndForget) != 0; + + return operation is IBinaryOperation { OperatorKind: BinaryOperatorKind.Or } binary + && (CarriesFireAndForget(binary.LeftOperand, fireAndForget) + || CarriesFireAndForget(binary.RightOperand, fireAndForget)); + } + + /// How to name the receiver in the message. + /// + /// Worth the words: "a transaction" and "a batch" behave identically here but read very differently to + /// somebody who only knows they used one of them, and ITransaction is both. + /// + private static string Describe(ITypeSymbol? type, KnownSymbols known) + => known.IsTransactional(type) ? "a transaction" : "a batch"; + + private sealed class KnownSymbols + { + private KnownSymbols(INamedTypeSymbol? batch, INamedTypeSymbol? transactionAsync, INamedTypeSymbol? transaction, INamedTypeSymbol? commandFlags, INamedTypeSymbol? task, int? fireAndForgetValue) + { + Batch = batch; + TransactionAsync = transactionAsync; + Transaction = transaction; + CommandFlags = commandFlags; + Task = task; + FireAndForgetValue = fireAndForgetValue; + } + + private INamedTypeSymbol? Batch { get; } + private INamedTypeSymbol? TransactionAsync { get; } + private INamedTypeSymbol? Transaction { get; } + private INamedTypeSymbol? CommandFlags { get; } + private INamedTypeSymbol? Task { get; } + + /// + /// The value of CommandFlags.FireAndForget, read from the referenced library rather than assumed. + /// + public int? FireAndForgetValue { get; } + + public static KnownSymbols? TryCreate(Compilation compilation) + { + // 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 batch = compilation.GetTypeByMetadataName("StackExchange.Redis.IBatch"); + var transactionAsync = compilation.GetTypeByMetadataName("StackExchange.Redis.ITransactionAsync"); + if (batch is null && transactionAsync is null) return null; + + var commandFlags = compilation.GetTypeByMetadataName("StackExchange.Redis.CommandFlags"); + int? fireAndForget = null; + if (commandFlags is not null) + { + foreach (var member in commandFlags.GetMembers("FireAndForget")) + { + if (member is IFieldSymbol { HasConstantValue: true, ConstantValue: int value }) + { + fireAndForget = value; + break; + } + } + } + + return new KnownSymbols( + batch, + transactionAsync, + compilation.GetTypeByMetadataName("StackExchange.Redis.ITransaction"), + commandFlags, + compilation.GetTypeByMetadataName("System.Threading.Tasks.Task"), + fireAndForget); + } + + /// Whether commands on this receiver are queued rather than sent. + public bool IsQueueing(ITypeSymbol? type) + => Implements(type, Batch) || Implements(type, TransactionAsync); + + /// Whether this receiver is a transaction, as opposed to a plain batch. + public bool IsTransactional(ITypeSymbol? type) + => Implements(type, TransactionAsync) || Implements(type, Transaction); + + public bool IsCommandFlags(ITypeSymbol? type) + => CommandFlags is not null && SymbolEqualityComparer.Default.Equals(type, CommandFlags); + + /// Task or Task<T>, which is all the queueing surface returns. + /// + /// Walks the base chain rather than testing one type, because Task<T> is not Task - + /// it derives from it - and .Result is declared on the generic one. + /// + public bool IsTask(ITypeSymbol? type) + { + if (Task is null) return false; + for (var candidate = type as INamedTypeSymbol; candidate is not null; candidate = candidate.BaseType) + { + if (SymbolEqualityComparer.Default.Equals(candidate.ConstructedFrom, Task)) return true; + } + + return false; + } + + /// + /// Whether this is one of the methods that ends the batch, rather than a command queued into it. + /// + /// + /// Decided by *declaring interface*, never by name, and that is the whole trap in this rule. + /// ITransaction[Async].ExecuteAsync(CommandFlags) is the one call here that must be awaited, + /// while IDatabaseAsync.ExecuteAsync(string command, ...) - the raw-command escape hatch - is an + /// ordinary queued command that must still be reported. A name match on "ExecuteAsync" gets both of + /// those exactly the wrong way round. + /// + /// The receiver is not always the interface, though: a call on a *class* that implements it - a + /// decorator such as KeyPrefixedTransaction, or a caller's own wrapper - resolves to the class's + /// member, whose containing type is the class. So where the direct test fails, ask the type which of + /// its members implements each terminator. This repo's own build found that one, which is a fair + /// preview of how a consumer would have. + /// + /// + public bool IsTerminator(IMethodSymbol method) + { + var containing = method.ContainingType; + if (containing is null) return false; + if (Same(containing, Batch) || Same(containing, TransactionAsync) || Same(containing, Transaction)) return true; + + foreach (var iface in containing.AllInterfaces) + { + if (!Same(iface, Batch) && !Same(iface, TransactionAsync) && !Same(iface, Transaction)) continue; + + foreach (var member in iface.GetMembers()) + { + if (member is IMethodSymbol interfaceMethod + && SymbolEqualityComparer.Default.Equals(containing.FindImplementationForInterfaceMember(interfaceMethod), method)) + { + return true; + } + } + } + + return false; + } + + private static bool Same(ITypeSymbol? type, INamedTypeSymbol? target) + => target is not null && SymbolEqualityComparer.Default.Equals(type, target); + + private static bool Implements(ITypeSymbol? type, INamedTypeSymbol? target) + { + if (type is null || target is null) return false; + if (SymbolEqualityComparer.Default.Equals(type, target)) return true; + + foreach (var iface in type.AllInterfaces) + { + if (SymbolEqualityComparer.Default.Equals(iface, target)) return true; + } + + return false; + } + } +} diff --git a/eng/StackExchange.Redis.CodeFixes/QueuedResultCodeFixProvider.cs b/eng/StackExchange.Redis.CodeFixes/QueuedResultCodeFixProvider.cs new file mode 100644 index 000000000..8b0aeb287 --- /dev/null +++ b/eng/StackExchange.Redis.CodeFixes/QueuedResultCodeFixProvider.cs @@ -0,0 +1,251 @@ +using System.Collections.Immutable; +using System.Composition; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Editing; +using Microsoft.CodeAnalysis.Formatting; + +namespace StackExchange.Redis.CodeFixes; + +/// +/// Fixes SER305/SER306 - waiting for the result of a command queued on a transaction or batch - by either +/// discarding the result, or capturing it and awaiting it after Execute[Async]. +/// +/// +/// +/// The IDs are literals rather than a reference to Diagnostics: that type is internal to the analyzer +/// assembly, which this deliberately does not reference (see the csproj). They are a published contract in any +/// case, so they cannot drift without a deliberate decision. +/// +/// +/// Both fixes work from the diagnostic's additional location, which the analyzer sets to the queued +/// call inside the wait. That avoids re-deriving "which invocation is being waited for" here, where getting it +/// wrong on a nested call - await tran.StringGetAsync(GetKey()) - would rewrite the wrong expression. +/// +/// +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(QueuedResultCodeFixProvider))] +[Shared] +public sealed class QueuedResultCodeFixProvider : CodeFixProvider +{ + private const string AwaitBeforeExecuteId = "SER305", FireAndForgetId = "SER306"; + + /// + public override ImmutableArray FixableDiagnosticIds { get; } + = ImmutableArray.Create(AwaitBeforeExecuteId, FireAndForgetId); + + /// + // No FixAllProvider: the two fixes are alternatives rather than one obvious answer, and "capture it and + // await it after Execute" moves statements around, which is not something to apply unread across a + // solution. Offering BatchFixer here would make "fix all occurrences" appear and quietly pick discard. + public override FixAllProvider? GetFixAllProvider() => null; + + /// + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + if (root is null) return; + + foreach (var diagnostic in context.Diagnostics) + { + if (diagnostic.AdditionalLocations.Count == 0) continue; + if (root.FindNode(diagnostic.AdditionalLocations[0].SourceSpan, getInnermostNodeForTie: true) + is not InvocationExpressionSyntax queued) continue; + + var wait = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + if (wait is null) continue; + + // Every rewrite below replaces or moves a whole statement, so a wait buried in a larger expression + // - an argument, a ternary arm - is left alone: there is no edit that keeps the surrounding + // expression meaningful, and a partial one would be worse than the squiggle. + if (StatementFor(wait) is not { } statement) continue; + + context.RegisterCodeFix( + CodeAction.Create( + "Discard the queued result", + token => DiscardAsync(context.Document, root, statement, queued), + equivalenceKey: nameof(DiscardAsync)), + diagnostic); + + // Capturing and awaiting later is only meaningful for SER305: a fire-and-forget result is the + // default value whenever it is read, so moving the read later buys nothing at all (see SER306). + // Restricted to the `await` form, since the sync shapes are in a method with no await to move to. + if (diagnostic.Id != AwaitBeforeExecuteId || wait is not AwaitExpressionSyntax) continue; + + var model = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); + if (model is null) continue; + if (!CanCaptureAndAwaitLater(model, statement, queued, context.CancellationToken, out var execute)) continue; + + context.RegisterCodeFix( + CodeAction.Create( + "Capture it and await after Execute", + token => CaptureAsync(context.Document, model, statement, queued, execute, token), + equivalenceKey: nameof(CaptureAsync)), + diagnostic); + } + } + + /// The statement the wait forms on its own, if it forms one. + private static StatementSyntax? StatementFor(SyntaxNode wait) => wait.Parent switch + { + // await tran.StringSetAsync(key, value); + ExpressionStatementSyntax statement => statement, + + // var value = await tran.StringGetAsync(key); + EqualsValueClauseSyntax { Parent: VariableDeclaratorSyntax { Parent: VariableDeclarationSyntax { Parent: LocalDeclarationStatementSyntax local } declaration } } + when declaration.Variables.Count == 1 => local, + + _ => null, + }; + + private static Task DiscardAsync(Document document, SyntaxNode root, StatementSyntax statement, InvocationExpressionSyntax queued) + { + // "_ = tran.StringGetAsync(key);" rather than a bare expression statement, which would be CS0201 for + // anything but an invocation - and reads as deliberate, which is the point of the fix + var discard = SyntaxFactory.ExpressionStatement( + SyntaxFactory.AssignmentExpression( + SyntaxKind.SimpleAssignmentExpression, + SyntaxFactory.IdentifierName("_"), + queued.WithoutTrivia())) + .WithTriviaFrom(statement) + .WithAdditionalAnnotations(Formatter.Annotation); + + return Task.FromResult(document.WithSyntaxRoot(root.ReplaceNode(statement, discard))); + } + + /// + /// Whether the capture-and-await-later rewrite is safe here, and if so which statement executes the batch. + /// + /// + /// Two things have to hold, and the second is the subtle one: the Execute[Async] must be a later + /// statement in the same block, and - where the wait declares a variable - nothing between the two may + /// already refer to that variable, since the declaration is what moves. Rewriting regardless would produce + /// CS0841 ("cannot use before it is declared"), i.e. a fix that breaks the build it was offered to repair. + /// + private static bool CanCaptureAndAwaitLater( + SemanticModel model, + StatementSyntax statement, + InvocationExpressionSyntax queued, + CancellationToken cancellationToken, + out StatementSyntax execute) + { + execute = null!; + if (statement.Parent is not BlockSyntax block) return false; + if (queued.Expression is not MemberAccessExpressionSyntax { Expression: { } receiver }) return false; + if (model.GetSymbolInfo(receiver, cancellationToken).Symbol is not { } receiverSymbol) return false; + + var index = block.Statements.IndexOf(statement); + if (index < 0) return false; + + for (int i = index + 1; i < block.Statements.Count; i++) + { + var candidate = block.Statements[i]; + foreach (var invocation in candidate.DescendantNodes().OfType()) + { + if (invocation.Expression is not MemberAccessExpressionSyntax { Name.Identifier.ValueText: "Execute" or "ExecuteAsync" } member) continue; + if (model.GetSymbolInfo(member.Expression, cancellationToken).Symbol is not { } candidateSymbol) continue; + if (!SymbolEqualityComparer.Default.Equals(candidateSymbol, receiverSymbol)) continue; + + execute = candidate; + return DeclaredLocalIsUnusedBefore(model, statement, block, index, i, cancellationToken); + } + } + + return false; + } + + private static bool DeclaredLocalIsUnusedBefore( + SemanticModel model, + StatementSyntax statement, + BlockSyntax block, + int from, + int to, + CancellationToken cancellationToken) + { + if (statement is not LocalDeclarationStatementSyntax local) return true; // nothing moves + if (model.GetDeclaredSymbol(local.Declaration.Variables[0], cancellationToken) is not { } declared) return false; + + for (int i = from + 1; i <= to; i++) + { + foreach (var identifier in block.Statements[i].DescendantNodes().OfType()) + { + if (SymbolEqualityComparer.Default.Equals(model.GetSymbolInfo(identifier, cancellationToken).Symbol, declared)) return false; + } + } + + return true; + } + + private static async Task CaptureAsync( + Document document, + SemanticModel model, + StatementSyntax statement, + InvocationExpressionSyntax queued, + StatementSyntax execute, + CancellationToken cancellationToken) + { + var name = UniqueName(model, statement, cancellationToken); + + // the queueing call stays exactly where it was - only the *waiting* moves, which is the whole point + var capture = SyntaxFactory.LocalDeclarationStatement( + SyntaxFactory.VariableDeclaration( + SyntaxFactory.IdentifierName("var"), + SyntaxFactory.SingletonSeparatedList( + SyntaxFactory.VariableDeclarator(SyntaxFactory.Identifier(name)) + .WithInitializer(SyntaxFactory.EqualsValueClause(queued.WithoutTrivia()))))) + .WithTriviaFrom(statement) + .WithAdditionalAnnotations(Formatter.Annotation); + + var awaited = SyntaxFactory.AwaitExpression(SyntaxFactory.IdentifierName(name)); + + // an expression statement becomes a bare "await pending;"; a declaration keeps its variable, which is + // why it had to be safe to move (see DeclaredLocalIsUnusedBefore) + StatementSyntax resumed = statement is LocalDeclarationStatementSyntax local + ? local.WithDeclaration(local.Declaration.WithVariables( + SyntaxFactory.SingletonSeparatedList( + local.Declaration.Variables[0].WithInitializer(SyntaxFactory.EqualsValueClause(awaited))))) + : SyntaxFactory.ExpressionStatement(awaited); + + // Trivia copied from the Execute statement it lands beside, rather than left to the formatter: the + // newline that separates two statements is the *trailing* trivia of the first, so a node built without + // any would run straight into whatever follows it ("await pending; return value;" on one line). + resumed = resumed + .WithLeadingTrivia(execute.GetLeadingTrivia()) + .WithTrailingTrivia(execute.GetTrailingTrivia()); + + // an editor rather than two root rewrites: the wait and the Execute are separate statements in the + // same block, and InsertAfter puts the resumed wait beside Execute rather than nesting a new block + // around it, which is what replacing the node outright would do + var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); + editor.ReplaceNode(statement, capture); + editor.InsertAfter(execute, resumed); + return editor.GetChangedDocument(); + } + + /// A name for the captured task that is not already in scope at this point. + private static string UniqueName(SemanticModel model, StatementSyntax statement, CancellationToken cancellationToken) + { + const string Preferred = "pending"; + var taken = model.LookupSymbols(statement.SpanStart); + + string candidate = Preferred; + for (int suffix = 2; Contains(taken, candidate); suffix++) + { + candidate = Preferred + suffix.ToString(System.Globalization.CultureInfo.InvariantCulture); + } + + return candidate; + + static bool Contains(ImmutableArray symbols, string name) + { + foreach (var symbol in symbols) + { + if (string.Equals(symbol.Name, name, System.StringComparison.Ordinal)) return true; + } + + return false; + } + } +} diff --git a/eng/StackExchange.Redis.CodeFixes/StackExchange.Redis.CodeFixes.csproj b/eng/StackExchange.Redis.CodeFixes/StackExchange.Redis.CodeFixes.csproj new file mode 100644 index 000000000..6c75a83c0 --- /dev/null +++ b/eng/StackExchange.Redis.CodeFixes/StackExchange.Redis.CodeFixes.csproj @@ -0,0 +1,30 @@ + + + + + netstandard2.0 + enable + enable + + + + + + + + diff --git a/src/StackExchange.Redis/StackExchange.Redis.csproj b/src/StackExchange.Redis/StackExchange.Redis.csproj index c7acece30..2d04242ab 100644 --- a/src/StackExchange.Redis/StackExchange.Redis.csproj +++ b/src/StackExchange.Redis/StackExchange.Redis.csproj @@ -65,16 +65,18 @@ @@ -84,6 +86,13 @@ + + + + + +/// Base for code-fix verification: the source with its markers, the diagnostics expected, and the source the +/// fix is expected to produce. +/// +/// +/// The fixed source is compared literally, so these tests pin the emitted *formatting* as well as the rewrite. +/// That is deliberate - a fix that produces correct but mangled code is a fix people undo - but it does mean a +/// whitespace change in the fixer shows up as a test failure to read rather than a mystery. +/// +public abstract class CodeFixVerifier + where TAnalyzer : DiagnosticAnalyzer, new() + where TCodeFix : CodeFixProvider, new() +{ + /// The fix offered first: discard the queued result. + protected const int DiscardFix = 0; + + /// The fix offered second: capture the task and await it after Execute. + protected const int CaptureFix = 1; + + /// + protected static DiagnosticResult Diagnostic(string id, DiagnosticSeverity severity = DiagnosticSeverity.Warning) + => new(id, severity); + + /// + /// Verify that applying to yields + /// . + /// + protected static Task VerifyFixAsync(string source, string fixedSource, int codeActionIndex, params DiagnosticResult[] expected) + { + var test = new CSharpCodeFixTest + { + TestCode = TestSetup.WithPreamble(source), + FixedCode = TestSetup.WithPreamble(fixedSource), + CodeActionIndex = codeActionIndex, + }; + + TestSetup.Configure(test, referenceLibrary: true, minServerVersion: null); + test.ExpectedDiagnostics.AddRange(expected); + return test.RunAsync(TestContext.Current.CancellationToken); + } + + /// + /// Verify that no fix is offered for , which still reports + /// . + /// + /// + /// Expressed as "the code is unchanged by fixing it", which is how the harness spells "nothing on offer". + /// Worth asserting explicitly: the shapes with no safe rewrite are a decision, and a fix quietly appearing + /// for one of them later would be a rewrite nobody reasoned about. + /// + protected static Task VerifyNoFixAsync(string source, params DiagnosticResult[] expected) + { + var test = new CSharpCodeFixTest + { + TestCode = TestSetup.WithPreamble(source), + FixedCode = TestSetup.WithPreamble(source), + }; + + TestSetup.Configure(test, referenceLibrary: true, minServerVersion: null); + test.ExpectedDiagnostics.AddRange(expected); + return test.RunAsync(TestContext.Current.CancellationToken); + } +} diff --git a/tests/StackExchange.Redis.Build.Tests/SER305.cs b/tests/StackExchange.Redis.Build.Tests/SER305.cs new file mode 100644 index 000000000..7818df2c3 --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER305.cs @@ -0,0 +1,330 @@ +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// Waiting for a command queued on a transaction or batch, at the point of queueing - which never completes. +/// +/// +/// The only error-severity rule in the set, so the negative cases matter more here than anywhere else: a false +/// positive is a broken build on code that works. covers the fire-and-forget case, which +/// is the one shape that survives the wait. +/// +public class SER305 : Verifier +{ + [Fact] + public Task AwaitedTransactionCommand_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var value = {|#0:await {|#1:tran.StringGetAsync(key)|}|}; + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + + [Fact] + public Task AwaitedBatchCommand_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var batch = db.CreateBatch(); + {|#0:await {|#1:batch.StringSetAsync(key, "value")|}|}; + batch.Execute(); + } + } + """, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringSetAsync", "a batch")); + + [Fact] + public Task BlockingOnResult_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public void M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var value = {|#0:{|#1:tran.StringGetAsync(key)|}.Result|}; + tran.Execute(); + } + } + """, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + + [Fact] + public Task BlockingOnWait_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public void M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:{|#1:tran.StringSetAsync(key, "value")|}.Wait()|}; + tran.Execute(); + } + } + """, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringSetAsync", "a transaction")); + + [Fact] + public Task BlockingOnGetAwaiterGetResult_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public void M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var value = {|#0:{|#1:tran.StringGetAsync(key)|}.GetAwaiter().GetResult()|}; + tran.Execute(); + } + } + """, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + + /// + /// The trap the rule is written around: this ExecuteAsync is IDatabaseAsync's raw-command + /// escape hatch, which queues like anything else - not ITransaction's terminator of the same name. + /// + [Fact] + public Task AwaitedRawCommand_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db) + { + var tran = db.CreateTransaction(); + var result = {|#0:await {|#1:tran.ExecuteAsync("PING")|}|}; + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("ExecuteAsync", "a transaction")); + + [Fact] + public Task AwaitedWithUnrelatedFlags_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var value = {|#0:await {|#1:tran.StringGetAsync(key, CommandFlags.DemandMaster)|}|}; + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + + [Fact] + public Task AwaitedOnTransactionAsyncInterface_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(ITransactionAsync tran, RedisKey key) + { + var value = {|#0:await {|#1:tran.StringGetAsync(key)|}|}; + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + + // ---- negative cases: everything below must stay silent ---- + + /// + /// A terminator reached through an interface that derives from ITransaction still resolves to + /// ITransaction's member, so it is recognised for free. + /// + /// + /// The case this *cannot* reach from here is a decorator **class** implementing ITransaction, where + /// the resolved member belongs to the class rather than the interface - which is a real false positive, and + /// is what KnownSymbols.IsTerminator's implementation walk exists for. There is no golden test for it + /// because no public type in the library implements ITransaction (they are all internal) and a test + /// source cannot declare one: an abstract class may not leave interface members unimplemented (CS0535), so + /// it would have to spell out the whole of IDatabaseAsync. It is covered instead by this repo's own + /// build, where KeyPrefixedTransactionTests.ExecuteAsync failing to compile is exactly that + /// regression - which is how the bug was found in the first place. + /// + [Fact] + public Task AwaitedTerminatorOnDerivedInterface_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + interface IMyTransaction : ITransaction { } + class C + { + public async Task M(IMyTransaction tran, RedisKey key) + { + _ = tran.StringSetAsync(key, "value"); + var committed = await tran.ExecuteAsync(); + } + } + """); + + /// ...but a queued command on that same interface is still a queued command. + [Fact] + public Task AwaitedCommandOnDerivedInterface_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + interface IMyTransaction : ITransaction { } + class C + { + public async Task M(IMyTransaction tran, RedisKey key) + { + var value = {|#0:await {|#1:tran.StringGetAsync(key)|}|}; + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + + /// The shape the rule is steering people towards, which must never be flagged. + [Fact] + public Task CapturedAndAwaitedAfterExecute_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var pending = tran.StringGetAsync(key); + await tran.ExecuteAsync(); + var value = await pending; + } + } + """); + + /// + /// The same capture, awaited *before* Execute - genuinely broken, but only knowable by ordering, which is + /// out of scope on purpose. Pinned so that "no diagnostic" here is a decision rather than an oversight. + /// + [Fact] + public Task CapturedAndAwaitedBeforeExecute_IsNotReported() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var pending = tran.StringGetAsync(key); + var value = await pending; + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + public Task AwaitedTerminator_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringSetAsync(key, "value"); + var committed = await tran.ExecuteAsync(); + } + } + """); + + [Fact] + public Task AwaitedOnDatabase_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"); + } + } + """); + + /// + /// A transaction reached through IDatabaseAsync - which a helper method taking one cannot tell from + /// a plain database, and neither can we. + /// + [Fact] + public Task AwaitedThroughDatabaseAsyncParameter_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabaseAsync db, RedisKey key) + { + var value = await db.StringGetAsync(key); + } + } + """); + + /// + /// Non-constant flags may or may not carry FireAndForget at run-time, and an error must not guess: this is + /// the wrapper-method shape, where the caller's flags are passed straight through. + /// + [Fact] + public Task AwaitedWithNonConstantFlags_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, CommandFlags flags) + { + var tran = db.CreateTransaction(); + var value = await tran.StringGetAsync(key, flags); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + public Task DiscardedQueuedCommand_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringSetAsync(key, "value"); + tran.AddCondition(Condition.KeyExists(key)); + await tran.ExecuteAsync(); + } + } + """); +} diff --git a/tests/StackExchange.Redis.Build.Tests/SER305CodeFix.cs b/tests/StackExchange.Redis.Build.Tests/SER305CodeFix.cs new file mode 100644 index 000000000..f7665503f --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER305CodeFix.cs @@ -0,0 +1,182 @@ +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using StackExchange.Redis.CodeFixes; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// The two fixes for SER305/SER306: discard the queued result, or capture it and await after Execute. +/// +public class SER305CodeFix : CodeFixVerifier +{ + [Fact] + public Task Discard_RewritesToDiscardAssignment() => VerifyFixAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:await {|#1:tran.StringSetAsync(key, "value")|}|}; + await tran.ExecuteAsync(); + } + } + """, + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + } + } + """, + DiscardFix, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringSetAsync", "a transaction")); + + [Fact] + public Task Capture_MovesTheAwaitAfterExecute() => VerifyFixAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:await {|#1:tran.StringSetAsync(key, "value")|}|}; + await tran.ExecuteAsync(); + } + } + """, + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var pending = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + await pending; + } + } + """, + CaptureFix, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringSetAsync", "a transaction")); + + /// + /// The value-returning shape: the declaration moves with the await, so the variable is still there. + /// + [Fact] + public Task Capture_MovesADeclarationAfterExecute() => VerifyFixAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var value = {|#0:await {|#1:tran.StringGetAsync(key)|}|}; + await tran.ExecuteAsync(); + return value; + } + } + """, + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var pending = tran.StringGetAsync(key); + await tran.ExecuteAsync(); + var value = await pending; + return value; + } + } + """, + CaptureFix, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + + /// A name already in scope is stepped over rather than shadowed. + [Fact] + public Task Capture_AvoidsAnExistingName() => VerifyFixAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var pending = 1; + var tran = db.CreateTransaction(); + {|#0:await {|#1:tran.StringSetAsync(key, "value")|}|}; + await tran.ExecuteAsync(); + } + } + """, + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var pending = 1; + var tran = db.CreateTransaction(); + var pending2 = tran.StringSetAsync(key, "value"); + await tran.ExecuteAsync(); + await pending2; + } + } + """, + CaptureFix, + Diagnostic("SER305", DiagnosticSeverity.Error).WithLocation(0).WithLocation(1).WithArguments("StringSetAsync", "a transaction")); + + /// + /// Fire-and-forget gets the discard fix only: awaiting later would still read the default value, so + /// "capture it and await after Execute" is not offered - index 1 does not exist here. + /// + [Fact] + public Task FireAndForget_OffersDiscardOnly() => VerifyFixAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + {|#0:await {|#1:tran.StringSetAsync(key, "value", flags: CommandFlags.FireAndForget)|}|}; + await tran.ExecuteAsync(); + } + } + """, + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringSetAsync(key, "value", flags: CommandFlags.FireAndForget); + await tran.ExecuteAsync(); + } + } + """, + DiscardFix, + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringSetAsync", "a transaction")); +} diff --git a/tests/StackExchange.Redis.Build.Tests/SER306.cs b/tests/StackExchange.Redis.Build.Tests/SER306.cs new file mode 100644 index 000000000..4905e8095 --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/SER306.cs @@ -0,0 +1,174 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// The fire-and-forget case: waiting completes, but reads the default value rather than the server's answer. +/// +/// +/// A warning rather than 's error because the code runs - it just cannot tell you anything. +/// QueuedResultTests in the main suite pins the behaviour this rests on against a live server. +/// +public class SER306 : Verifier +{ + [Fact] + public Task AwaitedFireAndForget_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var value = {|#0:await {|#1:tran.StringGetAsync(key, CommandFlags.FireAndForget)|}|}; + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + + /// + /// Combined flags are still a compile-time constant, so the bit is visible through the |. + /// + [Fact] + public Task AwaitedCombinedFireAndForget_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var value = {|#0:await {|#1:tran.StringGetAsync(key, CommandFlags.FireAndForget | CommandFlags.DemandMaster)|}|}; + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + + [Fact] + public Task BlockingOnFireAndForgetResult_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public void M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + var value = {|#0:{|#1:tran.StringGetAsync(key, CommandFlags.FireAndForget)|}.Result|}; + tran.Execute(); + } + } + """, + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + + [Fact] + public Task AwaitedFireAndForgetOnBatch_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var batch = db.CreateBatch(); + {|#0:await {|#1:batch.StringSetAsync(key, "value", flags: CommandFlags.FireAndForget)|}|}; + batch.Execute(); + } + } + """, + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringSetAsync", "a batch")); + + /// + /// Not a constant, but | only sets bits - so the flag is there whatever flags holds. + /// + [Fact] + public Task AwaitedNonConstantOrFireAndForget_IsFlagged() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, CommandFlags flags) + { + var tran = db.CreateTransaction(); + var value = {|#0:await {|#1:tran.StringGetAsync(key, flags | CommandFlags.FireAndForget)|}|}; + await tran.ExecuteAsync(); + } + } + """, + Diagnostic("SER306").WithLocation(0).WithLocation(1).WithArguments("StringGetAsync", "a transaction")); + + /// + /// The other side of that coin, and the reason the test exists: &~ would prove the flag *absent*, + /// which would promote a partly-understood expression to SER305 - an error. We decline to reason about it, + /// so this stays silent rather than becoming a build break. + /// + [Fact] + public Task AwaitedNonConstantWithoutFireAndForget_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, CommandFlags flags) + { + var tran = db.CreateTransaction(); + var value = await tran.StringGetAsync(key, flags & ~CommandFlags.FireAndForget); + await tran.ExecuteAsync(); + } + } + """); + + /// An | of something else says nothing either way, so nothing is said. + [Fact] + public Task AwaitedNonConstantOrUnrelatedFlag_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key, CommandFlags flags) + { + var tran = db.CreateTransaction(); + var value = await tran.StringGetAsync(key, flags | CommandFlags.DemandMaster); + await tran.ExecuteAsync(); + } + } + """); + + [Fact] + public Task DiscardedFireAndForget_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + var tran = db.CreateTransaction(); + _ = tran.StringSetAsync(key, "value", flags: CommandFlags.FireAndForget); + await tran.ExecuteAsync(); + } + } + """); + + /// Fire-and-forget straight to the database is ordinary code and says nothing to this rule. + [Fact] + public Task AwaitedFireAndForgetOnDatabase_IsClean() => VerifyAsync( + """ + using StackExchange.Redis; + using System.Threading.Tasks; + class C + { + public async Task M(IDatabase db, RedisKey key) + { + await db.StringSetAsync(key, "value", flags: CommandFlags.FireAndForget); + } + } + """); +} diff --git a/tests/StackExchange.Redis.Build.Tests/StackExchange.Redis.Build.Tests.csproj b/tests/StackExchange.Redis.Build.Tests/StackExchange.Redis.Build.Tests.csproj index 8abbe3b2c..14db431aa 100644 --- a/tests/StackExchange.Redis.Build.Tests/StackExchange.Redis.Build.Tests.csproj +++ b/tests/StackExchange.Redis.Build.Tests/StackExchange.Redis.Build.Tests.csproj @@ -12,6 +12,9 @@ + + @@ -19,6 +22,7 @@ + diff --git a/tests/StackExchange.Redis.Build.Tests/TestSetup.cs b/tests/StackExchange.Redis.Build.Tests/TestSetup.cs new file mode 100644 index 000000000..2bbddd39b --- /dev/null +++ b/tests/StackExchange.Redis.Build.Tests/TestSetup.cs @@ -0,0 +1,75 @@ +using System.IO; +using System.Text; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Testing; +using Microsoft.CodeAnalysis.Text; + +namespace StackExchange.Redis.Build.Tests; + +/// +/// The compilation setup shared by and +/// . +/// +/// +/// Shared rather than copied because the two must agree: a code-fix test that assembled its compilation even +/// slightly differently from the analyzer test would be verifying the fix against a different world than the +/// one the diagnostic came from. +/// +internal static class TestSetup +{ + /// + /// Test sources use string literals for keys/values, which trips the library's own [Experimental] + /// gate on the implicit string -> RedisValue conversion. + /// + /// + /// Unrelated to anything under test here, and it surfaces as an *error* in the test compilation, so it is + /// opted out of for every case. Prepended to fixed sources too, since the harness compares them literally. + /// + public const string Preamble = "#pragma warning disable StringToRedisValue"; + + /// + /// Reference assemblies matching the library build we load below. + /// + /// + /// The harness only ships well-known sets up to a point, and mismatching them against the + /// StackExchange.Redis build we reference gives CS1705 (assembly wants a newer System.Runtime), so + /// describe the current target explicitly rather than pinning to whatever the harness happens to know. + /// + public static readonly ReferenceAssemblies Net10 = new( + "net10.0", + new PackageIdentity("Microsoft.NETCore.App.Ref", "10.0.0"), + Path.Combine("ref", "net10.0")); + + /// Prefix a test source with . + public static string WithPreamble(string source) => Preamble + System.Environment.NewLine + source; + + /// + /// Apply the shared reference set and options to a test. + /// + /// + /// The analyzers here resolve StackExchange.Redis types by metadata name and do nothing at all when they + /// are absent - so without the positive cases would fail to compile + /// rather than silently pass, but the *negative* cases would trivially "pass" by finding no diagnostics. + /// Hence both this and NoLibrary.cs, which asserts the absent case deliberately rather than by accident. + /// + public static void Configure(AnalyzerTest test, bool referenceLibrary, string? minServerVersion) + { + test.ReferenceAssemblies = Net10; + + if (referenceLibrary) + { + test.TestState.AdditionalReferences.Add( + MetadataReference.CreateFromFile(typeof(StackExchange.Redis.ConnectionMultiplexer).Assembly.Location)); + } + + if (minServerVersion is not null) + { + // written as a .globalconfig entry, which is also how the MSBuild property arrives once + // CompilerVisibleProperty has translated it - so this covers both spellings' consumption path + test.TestState.AnalyzerConfigFiles.Add(("/.globalconfig", SourceText.From( + "is_global = true" + System.Environment.NewLine + + "redis.min_server_version = " + minServerVersion + System.Environment.NewLine, + Encoding.UTF8))); + } + } +} diff --git a/tests/StackExchange.Redis.Build.Tests/Verifier.cs b/tests/StackExchange.Redis.Build.Tests/Verifier.cs index c18d74020..8b7e8d4a2 100644 --- a/tests/StackExchange.Redis.Build.Tests/Verifier.cs +++ b/tests/StackExchange.Redis.Build.Tests/Verifier.cs @@ -1,11 +1,8 @@ -using System.IO; -using System.Text; using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp.Testing; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Testing; -using Microsoft.CodeAnalysis.Text; using Xunit; namespace StackExchange.Redis.Build.Tests; @@ -14,26 +11,17 @@ namespace StackExchange.Redis.Build.Tests; /// Base for analyzer verification, in the shape used by DapperAOT: a source string with {|#0:...|} /// markers, plus the diagnostics expected at those locations. /// +/// +/// The compilation itself is assembled by , shared with +/// . +/// public abstract class Verifier where TAnalyzer : DiagnosticAnalyzer, new() { - /// - /// Reference assemblies matching the library build we load below. - /// - /// - /// The harness only ships well-known sets up to a point, and mismatching them against the - /// StackExchange.Redis build we reference gives CS1705 (assembly wants a newer System.Runtime), so - /// describe the current target explicitly rather than pinning to whatever the harness happens to know. - /// - private static readonly ReferenceAssemblies Net10 = new( - "net10.0", - new PackageIdentity("Microsoft.NETCore.App.Ref", "10.0.0"), - Path.Combine("ref", "net10.0")); - /// Expect a diagnostic with this id at the marked location. /// - /// Defaults to because that is what the rules ship as; the harness - /// checks severity, so this is also what stops the default being changed without anyone noticing. + /// Defaults to because that is what most of the rules ship as; the + /// harness checks severity, so this is also what stops a default being changed without anyone noticing. /// protected static DiagnosticResult Diagnostic(string id, DiagnosticSeverity severity = DiagnosticSeverity.Warning) => new(id, severity); @@ -45,10 +33,6 @@ protected static Task VerifyAsync(string source, params DiagnosticResult[] expec /// /// As , but with the project declaring a minimum server version. /// - /// - /// Written as a .globalconfig entry, which is also how the MSBuild property arrives once - /// CompilerVisibleProperty has translated it - so this covers both spellings' consumption path. - /// protected static Task VerifyWithMinServerVersionAsync(string source, string minServerVersion, params DiagnosticResult[] expected) => RunAsync(source, referenceLibrary: true, minServerVersion, expected); @@ -65,33 +49,12 @@ protected static Task VerifyWithoutLibraryAsync(string source) private static Task RunAsync(string source, bool referenceLibrary, string? minServerVersion, params DiagnosticResult[] expected) { - // Test sources use string literals for keys/values, which trips the library's own [Experimental] - // gate on the implicit string -> RedisValue conversion. That is unrelated to what we are testing, and - // surfaces as an error in the test compilation, so opt out of it for every case. var test = new CSharpAnalyzerTest { - TestCode = "#pragma warning disable StringToRedisValue" + System.Environment.NewLine + source, - ReferenceAssemblies = Net10, + TestCode = TestSetup.WithPreamble(source), }; - // The analyzer resolves StackExchange.Redis.Condition by metadata name and does nothing at all if it - // is absent - so without this reference the positive cases would fail to compile rather than silently - // pass, but the *negative* cases would trivially "pass" by finding no diagnostics. Hence both this and - // NoLibrary.cs, which asserts the absent case deliberately rather than by accident. - if (referenceLibrary) - { - test.TestState.AdditionalReferences.Add( - MetadataReference.CreateFromFile(typeof(StackExchange.Redis.ConnectionMultiplexer).Assembly.Location)); - } - - if (minServerVersion is not null) - { - test.TestState.AnalyzerConfigFiles.Add(("/.globalconfig", SourceText.From( - "is_global = true" + System.Environment.NewLine - + "redis.min_server_version = " + minServerVersion + System.Environment.NewLine, - Encoding.UTF8))); - } - + TestSetup.Configure(test, referenceLibrary, minServerVersion); test.ExpectedDiagnostics.AddRange(expected); return test.RunAsync(TestContext.Current.CancellationToken); } From 487333208dc5f892d9a07d15916baa43aa7f9517 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 10:27:10 +0100 Subject: [PATCH 3/6] Keep the fire-and-forget probe allocation-free Testing a generic value against an interface boxes in IL, and that test sits on every operation queued to a retrying transaction. Value-type generics are not shared, though, so the JIT knows the exact TState: the optimizing JIT devirtualizes the property to a field load and removes the allocation completely - 11 bytes of machine code, no allocator call. It only does that in optimized code, and measured under default tiering the method stayed in tier-0 long enough to cost 24 bytes on every call. AggressiveOptimization compiles it straight to full opts, which takes that to zero from the first call. Guarded on #if NET because the enum member does not exist on the netfx targets, which keep the box. Recorded because it is counter-intuitive: rewriting the `is` pattern as a cast does *not* avoid the box. Both spellings emit the same box, and both were measured - the cast form only looked better because it happened to promote to tier-1 sooner, still leaving a startup window that this does not. --- .../Availability/RetryTransaction.cs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/StackExchange.Redis/Availability/RetryTransaction.cs b/src/StackExchange.Redis/Availability/RetryTransaction.cs index e1cec855c..c0f154a0f 100644 --- a/src/StackExchange.Redis/Availability/RetryTransaction.cs +++ b/src/StackExchange.Redis/Availability/RetryTransaction.cs @@ -1,5 +1,6 @@ -using System; +using System; using System.Collections.Generic; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using StackExchange.Redis.Interfaces; @@ -85,8 +86,16 @@ private Task ExecuteAsync(in TState state, AutoDatabaseAsyncOperation(in TState state) where TState : struct => state is IFlaggedRedisArgs flagged && (flagged.Flags & CommandFlags.FireAndForget) != 0; From 4d53f1827432b0c18992c4f874c246acbe2ba55f Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 10:46:49 +0100 Subject: [PATCH 4/6] Record the measurements behind the fire-and-forget probe, not just the conclusion --- .../Availability/RetryTransaction.cs | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/StackExchange.Redis/Availability/RetryTransaction.cs b/src/StackExchange.Redis/Availability/RetryTransaction.cs index c0f154a0f..4784a44c6 100644 --- a/src/StackExchange.Redis/Availability/RetryTransaction.cs +++ b/src/StackExchange.Redis/Availability/RetryTransaction.cs @@ -86,13 +86,21 @@ private Task ExecuteAsync(in TState state, AutoDatabaseAsyncOperation Date: Tue, 18 Aug 2026 11:00:17 +0100 Subject: [PATCH 5/6] Build no proxy at all for a fire-and-forget operation Settling a TaskCompletionSource up front was the wrong shape: it still built one, and a TaskCompletionSource constructs its Task in its own constructor, so every fire-and-forget operation queued cost two objects - 96 bytes measured - to carry a value that is fixed before the call returns. The flag now goes to RecordedOp, which builds no proxy when the reply has been declined and hands back the shared completed task instead: CompletedTask for the typed shape, Task.CompletedTask for the void one. That is the same instance a plain RedisTransaction returns for this case, so the two paths now agree by construction rather than by producing equivalent values. Forwarding and faulting become no-ops for such an operation, which is what the already-settled proxy achieved before by relying on TrySet* finding it settled; this states it directly instead. The operation is still recorded and replayed exactly as any other, since fire-and-forget declines the reply, not the command. Pinned by identity rather than by a byte count, which would be brittle: two fire-and-forget calls hand back the same task instance, the void shape hands back Task.CompletedTask, and an operation that did not decline its reply still gets a durable proxy of its own. --- .../Availability/RetryTransaction.cs | 62 ++++++++++--------- .../QueuedResultTests.cs | 36 +++++++++++ 2 files changed, 69 insertions(+), 29 deletions(-) diff --git a/src/StackExchange.Redis/Availability/RetryTransaction.cs b/src/StackExchange.Redis/Availability/RetryTransaction.cs index 4784a44c6..9f83ceefc 100644 --- a/src/StackExchange.Redis/Availability/RetryTransaction.cs +++ b/src/StackExchange.Redis/Availability/RetryTransaction.cs @@ -58,9 +58,8 @@ private Task ExecuteAsync(in TState state, AutoDatabas where TState : struct { CheckNotExecuted(); - var op = new RecordedOp(state, operation); + var op = new RecordedOp(state, operation, IsFireAndForget(in state)); _ops.Add(op); - if (IsFireAndForget(in state)) op.CompleteFireAndForget(); return op.Proxy; } @@ -68,9 +67,8 @@ private Task ExecuteAsync(in TState state, AutoDatabaseAsyncOperation(state, operation); + var op = new RecordedVoidOp(state, operation, IsFireAndForget(in state)); _ops.Add(op); - if (IsFireAndForget(in state)) op.CompleteFireAndForget(); return op.Proxy; } @@ -81,9 +79,8 @@ private Task ExecuteAsync(in TState state, AutoDatabaseAsyncOperation : IRecordedOp { private readonly TState _state; private readonly AutoDatabaseAsyncOperation _operation; - private readonly TaskCompletionSource _proxy = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource? _proxy; private Task? _attempt; - public RecordedOp(in TState state, AutoDatabaseAsyncOperation operation) + public RecordedOp(in TState state, AutoDatabaseAsyncOperation operation, bool fireAndForget) { _state = state; _operation = operation; - } - public Task Proxy => _proxy.Task; + // Fire-and-forget declines the reply, so there is nothing for a proxy to carry and none is built - + // saving *both* objects, the TaskCompletionSource and the Task it creates in its own constructor, + // on every fire-and-forget operation queued. Proxy hands back the shared completed task instead, + // which is the same instance a plain RedisTransaction returns for this case. + // + // The value is default(TResult) rather than the command's own "no reply" value, which lives inside + // the command implementation and is not reachable from here - a distinction without a difference, + // since neither is an answer from the server. + _proxy = fireAndForget ? null : new(TaskCreationOptions.RunContinuationsAsynchronously); + } - // settled up front for fire-and-forget; see IsFireAndForget. default(TResult) rather than the - // command's own "no reply" value, which lives inside the command implementation and is not reachable - // from here - a distinction without a difference, since neither is an answer from the server - public void CompleteFireAndForget() => _proxy.TrySetResult(default!); + public Task Proxy => _proxy?.Task ?? CompletedTask.Default(null)!; public void Replay(IDatabaseAsync inner) => _attempt = _operation(in _state, inner); public void ForwardSuccess() { + if (_proxy is null) return; // fire-and-forget: no proxy to forward onto, and nothing to report + var attempt = _attempt!; if (attempt.IsCompleted) { @@ -295,9 +299,9 @@ private void ForwardLater(Task attempt) => attempt.ContinueWith( private void Forward(Task attempt) { - if (attempt.IsCanceled) _proxy.TrySetCanceled(); - else if (attempt.IsFaulted) _proxy.TrySetException(attempt.Exception!.InnerExceptions); - else _proxy.TrySetResult(attempt.GetAwaiter().GetResult()); + if (attempt.IsCanceled) _proxy!.TrySetCanceled(); + else if (attempt.IsFaulted) _proxy!.TrySetException(attempt.Exception!.InnerExceptions); + else _proxy!.TrySetResult(attempt.GetAwaiter().GetResult()); } // observe the (faulted) inner attempt before faulting the durable proxy, so the discarded @@ -305,7 +309,7 @@ private void Forward(Task attempt) public void Fault(Exception ex) { Observe(); - _proxy.TrySetException(ex); + _proxy?.TrySetException(ex); } public void Observe() => _ = _attempt?.Exception; @@ -316,24 +320,24 @@ private sealed class RecordedVoidOp : IRecordedOp { private readonly TState _state; private readonly AutoDatabaseAsyncOperation _operation; - private readonly TaskCompletionSource _proxy = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource? _proxy; private Task? _attempt; - public RecordedVoidOp(in TState state, AutoDatabaseAsyncOperation operation) + public RecordedVoidOp(in TState state, AutoDatabaseAsyncOperation operation, bool fireAndForget) { _state = state; _operation = operation; + _proxy = fireAndForget ? null : new(TaskCreationOptions.RunContinuationsAsynchronously); // see RecordedOp } - public Task Proxy => _proxy.Task; - - // see the note on RecordedOp.CompleteFireAndForget - public void CompleteFireAndForget() => _proxy.TrySetResult(true); + public Task Proxy => _proxy?.Task ?? Task.CompletedTask; public void Replay(IDatabaseAsync inner) => _attempt = _operation(in _state, inner); public void ForwardSuccess() { + if (_proxy is null) return; // fire-and-forget: see RecordedOp.ForwardSuccess + var attempt = _attempt!; if (attempt.IsCompleted) { @@ -355,9 +359,9 @@ private void ForwardLater(Task attempt) => attempt.ContinueWith( private void Forward(Task attempt) { - if (attempt.IsCanceled) _proxy.TrySetCanceled(); - else if (attempt.IsFaulted) _proxy.TrySetException(attempt.Exception!.InnerExceptions); - else _proxy.TrySetResult(true); + if (attempt.IsCanceled) _proxy!.TrySetCanceled(); + else if (attempt.IsFaulted) _proxy!.TrySetException(attempt.Exception!.InnerExceptions); + else _proxy!.TrySetResult(true); } // observe the (faulted) inner attempt before faulting the durable proxy, so the discarded @@ -365,7 +369,7 @@ private void Forward(Task attempt) public void Fault(Exception ex) { Observe(); - _proxy.TrySetException(ex); + _proxy?.TrySetException(ex); } public void Observe() => _ = _attempt?.Exception; diff --git a/tests/StackExchange.Redis.Tests/QueuedResultTests.cs b/tests/StackExchange.Redis.Tests/QueuedResultTests.cs index 20c389ed3..69dde2699 100644 --- a/tests/StackExchange.Redis.Tests/QueuedResultTests.cs +++ b/tests/StackExchange.Redis.Tests/QueuedResultTests.cs @@ -124,6 +124,42 @@ public async Task RetryTransactionQueuedFireAndForgetCompletesImmediatelyWithDef Assert.True((await pending).IsNull); } + /// + /// A fire-and-forget operation builds no at + /// all: every one hands back the same shared completed task. + /// + /// + /// Asserted as identity rather than as a byte count, which would be brittle. There is nothing for a proxy + /// to carry when the reply has been declined, so building one costs two objects per queued operation - the + /// source and the Task it creates in its own constructor - for a value that is fixed in advance. + /// The instance handed back is the one a plain returns for the same case. + /// + [Fact] + public async Task RetryTransactionFireAndForgetSharesOneCompletedTask() + { + await using var conn = Create(); + + RedisKey key = Me(); + var db = conn.GetDatabase(); + var tran = db.WithRetry().CreateTransaction(); + + var first = tran.StringGetAsync(key, CommandFlags.FireAndForget); + var second = tran.StringGetAsync(key, CommandFlags.FireAndForget); + Assert.Same(first, second); + + // the void-returning shape has its own proxy type, and the same applies to it + var firstVoid = tran.HashSetAsync(key, [new HashEntry("f", "v")], CommandFlags.FireAndForget); + Assert.Same(Task.CompletedTask, firstVoid); + + // ...while an operation that did *not* decline its reply still gets a durable proxy of its own + var pending = tran.StringGetAsync(key); + Assert.NotSame(first, pending); + Assert.False(pending.IsCompleted); + + Assert.True(await tran.ExecuteAsync()); + Assert.True(pending.IsCompleted); + } + /// ...and the command itself is still queued, replayed and sent. [Fact] public async Task RetryTransactionQueuedFireAndForgetStillExecutes() From a4a829ff8e8ec3669d63d574b9c77e9230ffbf8d Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 18 Aug 2026 11:13:01 +0100 Subject: [PATCH 6/6] Convince the compiler instead of asserting with `!` Three `_proxy!` in a row asserted an invariant that is not visible at the point of use: the null check lived in ForwardSuccess, which does not flow across a method boundary into Forward. Null-conditional on each branch says the same thing to the reader and to the compiler, with no suppression and no early return. Note `?.` also skips the *argument*, so in the faulted branch attempt.Exception is no longer read when there is no proxy - and reading it is what observes a fault. That is safe only because a fire-and-forget attempt is the shared completed task RedisTransaction hands back, which cannot fault; the comment says so, since it is not visible locally. Checked while here that Observe's discard is not elided in optimized code: a faulted task left alone raises one UnobservedTaskException, and `_ = t.Exception` raises none, so the access is genuinely made. GC.KeepAlive around it measured identically and would only be noise - the getter marks the holder handled, so it has side effects the JIT cannot optimize away. --- .../Availability/RetryTransaction.cs | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/StackExchange.Redis/Availability/RetryTransaction.cs b/src/StackExchange.Redis/Availability/RetryTransaction.cs index 9f83ceefc..cd45ab57e 100644 --- a/src/StackExchange.Redis/Availability/RetryTransaction.cs +++ b/src/StackExchange.Redis/Availability/RetryTransaction.cs @@ -270,13 +270,17 @@ public RecordedOp(in TState state, AutoDatabaseAsyncOperation o _proxy = fireAndForget ? null : new(TaskCreationOptions.RunContinuationsAsynchronously); } + // the ! is an annotation artifact, not a null: TResult is unconstrained, so Task and + // Task are the same type - Default simply spells its return the other way round public Task Proxy => _proxy?.Task ?? CompletedTask.Default(null)!; public void Replay(IDatabaseAsync inner) => _attempt = _operation(in _state, inner); public void ForwardSuccess() { - if (_proxy is null) return; // fire-and-forget: no proxy to forward onto, and nothing to report + // fire-and-forget: Forward guards too, so this is here to skip the continuation below rather than + // for correctness - there is no point registering one to settle a proxy that does not exist + if (_proxy is null) return; var attempt = _attempt!; if (attempt.IsCompleted) @@ -299,9 +303,13 @@ private void ForwardLater(Task attempt) => attempt.ContinueWith( private void Forward(Task attempt) { - if (attempt.IsCanceled) _proxy!.TrySetCanceled(); - else if (attempt.IsFaulted) _proxy!.TrySetException(attempt.Exception!.InnerExceptions); - else _proxy!.TrySetResult(attempt.GetAwaiter().GetResult()); + // a null proxy is fire-and-forget; `?.` then skips the argument too, including the Exception + // access that would otherwise observe a fault. Safe only because such an attempt is the shared + // completed task RedisTransaction hands back for F+F, so it cannot fault - see Observe for the + // attempts that can. + if (attempt.IsCanceled) _proxy?.TrySetCanceled(); + else if (attempt.IsFaulted) _proxy?.TrySetException(attempt.Exception!.InnerExceptions); + else _proxy?.TrySetResult(attempt.GetAwaiter().GetResult()); } // observe the (faulted) inner attempt before faulting the durable proxy, so the discarded @@ -359,9 +367,10 @@ private void ForwardLater(Task attempt) => attempt.ContinueWith( private void Forward(Task attempt) { - if (attempt.IsCanceled) _proxy!.TrySetCanceled(); - else if (attempt.IsFaulted) _proxy!.TrySetException(attempt.Exception!.InnerExceptions); - else _proxy!.TrySetResult(true); + // see RecordedOp.Forward for why `?.` is safe here + if (attempt.IsCanceled) _proxy?.TrySetCanceled(); + else if (attempt.IsFaulted) _proxy?.TrySetException(attempt.Exception!.InnerExceptions); + else _proxy?.TrySetResult(true); } // observe the (faulted) inner attempt before faulting the durable proxy, so the discarded