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/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/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/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..cd45ab57e 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; @@ -57,7 +58,7 @@ 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); return op.Proxy; } @@ -66,11 +67,44 @@ private Task ExecuteAsync(in TState state, AutoDatabaseAsyncOperation(state, operation); + var op = new RecordedVoidOp(state, operation, IsFireAndForget(in state)); _ops.Add(op); 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. What it does not get is a proxy - see RecordedOp - so forwarding and faulting both become + // no-ops for it, and 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). Testing a generic value against an interface boxes in IL, and this sits on every + // queued operation - but value-type generics are not shared, so the JIT knows the exact TState and + // compiles the whole thing to a field load with no allocator call at all (11 bytes of machine code). + // + // Measured on net10.0 rather than reasoned about, because none of it is obvious - bytes per call: + // + // `is` pattern, tier-0 ......... 24 `is` pattern, optimized ....... 0 + // cast instead of `is` ......... 24 ...with AggressiveOptimization 0 + // + // Two findings worth keeping. Rewriting the `is` pattern as a cast does *not* avoid the box - both + // spellings emit the same one - so the attribute below is the fix rather than a different expression. + // And a TState *without* the interface costs nothing even in tier-0: the test folds to a constant and + // the dead box is removed, so the states that captured no flags never pay for this at all. + + // skips tiering, so the above is free from the first call rather than only after tier-1 promotion +#if NET + [MethodImpl(MethodImplOptions.AggressiveOptimization)] +#endif + 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)); @@ -217,21 +251,37 @@ private sealed class RecordedOp : 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; + + // 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); } - public Task Proxy => _proxy.Task; + // 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() { + // 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) { @@ -253,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 @@ -263,7 +317,7 @@ private void Forward(Task attempt) public void Fault(Exception ex) { Observe(); - _proxy.TrySetException(ex); + _proxy?.TrySetException(ex); } public void Observe() => _ = _attempt?.Exception; @@ -274,21 +328,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; + 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) { @@ -310,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 @@ -320,7 +378,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/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); } diff --git a/tests/StackExchange.Redis.Tests/QueuedResultTests.cs b/tests/StackExchange.Redis.Tests/QueuedResultTests.cs new file mode 100644 index 000000000..69dde2699 --- /dev/null +++ b/tests/StackExchange.Redis.Tests/QueuedResultTests.cs @@ -0,0 +1,239 @@ +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); + } + + /// + /// 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() + { + 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); + } +}