diff --git a/src/StackExchange.Redis/MessageWriter.cs b/src/StackExchange.Redis/MessageWriter.cs index 93e63b58d..82870f1a9 100644 --- a/src/StackExchange.Redis/MessageWriter.cs +++ b/src/StackExchange.Redis/MessageWriter.cs @@ -164,14 +164,19 @@ internal void WriteHeader(RedisCommand command, int arguments) // *{argCount}\r\n = 3 + MaxInt32TextLen // ${cmd-len}\r\n = precomputed // {cmd}\r\n = precomputed - var span = _writer.GetSpan(commandBytes.Length + 3 + Format.MaxInt32TextLen); - span[0] = (byte)'*'; - - int offset = WriteRaw(span, arguments + 1, offset: 1); - commandBytes.CopyTo(span.Slice(offset)); - offset += commandBytes.Length; - - _writer.Advance(offset); + var needed = commandBytes.Length + 3 + Format.MaxInt32TextLen; + var span = _writer.GetSpan(needed); + if (span.Length >= needed) + { + span[0] = (byte)'*'; + int offset = WriteRaw(span, arguments + 1, offset: 1); + commandBytes.CopyTo(span.Slice(offset)); + _writer.Advance(offset + commandBytes.Length); + } + else + { + WriteHeaderSlow(_writer, arguments, commandBytes); + } } internal void WriteHeader(RedisCommand command, int arguments, ReadOnlySpan commandBytes) @@ -186,30 +191,28 @@ internal void WriteHeader(RedisCommand command, int arguments, ReadOnlySpan= needed) + { + span[0] = (byte)'*'; + int offset = WriteRaw(span, arguments + 1, offset: 1); + span[offset++] = (byte)'$'; + _writer.Advance(AppendToSpan(span, commandBytes, offset: offset)); + } + else + { + WriteHeaderUnframedSlow(_writer, arguments, commandBytes); + } } internal static void WriteMultiBulkHeader(IBufferWriter writer, long count) { - // *{count}\r\n = 3 + MaxInt32TextLen - var span = writer.GetSpan(3 + Format.MaxInt32TextLen); - span[0] = (byte)'*'; - int offset = WriteRaw(span, count, offset: 1); - writer.Advance(offset); + WriteCountPrefix(writer, (byte)'*', count); } internal static void WriteMultiBulkHeader(IBufferWriter writer, long count, RespPrefix prefix) { - // *{count}\r\n = 3 + MaxInt32TextLen - var span = writer.GetSpan(3 + Format.MaxInt32TextLen); - span[0] = (byte)prefix; if ((prefix is RespPrefix.Map or RespPrefix.Attribute) & count > 0) { if ((count & 1) != 0) Throw(prefix, count); @@ -218,8 +221,207 @@ internal static void WriteMultiBulkHeader(IBufferWriter writer, long count paramName: nameof(count), message: $"{type} data must be in pairs; got {count}"); } - int offset = WriteRaw(span, count, offset: 1); - writer.Advance(offset); + WriteCountPrefix(writer, (byte)prefix, count); + } + + /// + /// Write a {prefix}{count}\r\n header - the * of a multi-bulk and the $ of a bulk + /// string are the same shape. + /// + /// + /// Sized for an int64: , + /// WriteUnifiedPrefixedBlob and WriteUnifiedSequenceIterator all format a long, and + /// previously asked for int32 width. Unreachable in practice - it needs a value above int.MaxValue + /// - but the direct path could be handed exactly the 14 bytes it asked for and then write up to 23. + /// + private static void WriteCountPrefix(IBufferWriter writer, byte prefix, long count) + { + const int Needed = 3 + Format.MaxInt64TextLen; + var span = writer.GetSpan(Needed); + if (span.Length >= Needed) + { + span[0] = prefix; + writer.Advance(WriteRaw(span, count, offset: 1)); + } + else + { + WriteCountPrefixSlow(writer, prefix, count); + } + } + + /// + /// The fallback when the writer declined the hint: compose in a stack buffer and let the looping write + /// place it. + /// + /// + /// Deliberately separate and not inlined. A stackalloc in the caller's cold branch changes + /// codegen for the whole method: written inline, these fallbacks cost ~20% on the simplest command shape. + /// It is the same "inline-optimized for it-fits, pathological case doesn't inline" split the BCL uses, and + /// that describes. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static void WriteCountPrefixSlow(IBufferWriter writer, byte prefix, long count) + { + Span scratch = stackalloc byte[MaxValueScratch]; + scratch[0] = prefix; + writer.Write(scratch.Slice(0, WriteRaw(scratch, count, offset: 1))); + } + + /// + /// The fallback when the writer declined the hint, writing the header in pieces instead of as one burst. + /// + /// + /// Not inlined, for the reason given on . Note that the pieces go + /// through the checked : the ask that was just declined was larger + /// than its 23 bytes, so the writer may well still take those in place. Nothing is composed in a stack + /// buffer here - that only happens if is itself declined in turn. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static void WriteHeaderSlow(IBufferWriter writer, int arguments, ReadOnlySpan commandBytes) + { + // the command bytes are already framed, so they can go straight through the looping write. + // WriteCountPrefix, not …Slow: the ask that was declined was larger than this one, so the writer + // may well be able to take 23 and let the prefix go in place + WriteCountPrefix(writer, (byte)'*', arguments + 1); + writer.Write(commandBytes); + } + + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static void WriteHeaderUnframedSlow(IBufferWriter writer, int arguments, ReadOnlySpan commandBytes) + { + WriteCountPrefix(writer, (byte)'*', arguments + 1); + WriteCountPrefix(writer, (byte)'$', commandBytes.Length); + writer.Write(commandBytes); + WriteCrlf(writer); + } + + /// + /// Write a bulk string in pieces rather than as one burst. + /// + /// + /// + /// Unlike the …Slow methods this is not only a fallback: + /// sends every value over MaxQuickSpanSize here because no single span could hold it, so for large + /// binary values this is the normal path. It therefore uses the checked , + /// which writes the length prefix in place when the writer can take 23 bytes; going straight to + /// would compose into a stack buffer and hand it to a hintless + /// , costing a copy and risking a prefix split across segments. + /// + /// + /// It is still , but not for the reason given on + /// - there is no stackalloc left in here to perturb the + /// caller's codegen. The reason now is simply that 's hot path is the + /// small-value one, and this body has no business being in that frame. + /// + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static void WriteUnifiedSpanPiecewise(IBufferWriter writer, ReadOnlySpan value) + { + WriteCountPrefix(writer, (byte)'$', value.Length); + writer.Write(value); + WriteCrlf(writer); + } + + /// A SHA1 hash as a bulk string: $40\r\n plus 40 hex characters plus CRLF. + private const int Sha1BulkStringLength = 47; + + /// + /// The largest fixed-size burst any single compose-then-write below needs: a prefix byte, a formatted + /// int64 or double payload, and the framing CRLFs - whichever is larger. + /// + private const int MaxValueScratch = 7 + Format.MaxDoubleTextLen > 7 + Format.MaxInt64TextLen + ? 7 + Format.MaxDoubleTextLen + : 7 + Format.MaxInt64TextLen; + + /// + /// Obtain a span of at least bytes, reporting whether the writer obliged. + /// + /// + /// + /// sizeHint is advisory. The "should be at least this size" wording in the docs is left over + /// from when the parameter was called minSize; the guarantee is one element, and consuming code is + /// expected to test what it got and fall back. The BCL does exactly that - + /// loops and copies in slices rather than demanding one contiguous block - which is why the fallbacks here + /// hand their bytes to instead of asking again. + /// See . + /// + /// + /// The point of the looser contract is transports with page limits: they can honour reasonable requests + /// and refuse excessive ones. CycleBuffer is one - it caps the hint at 1k, sizes a fresh segment + /// from the committed total rather than from the hint, and, the case with no floor at all, hands back a + /// dangling recycled segment exactly as it is, whatever length that happens to be. All legitimate; the + /// callers were wrong. + /// + /// + /// Note that a false result is not side-effect free: GetSpan has already run, and on + /// CycleBuffer that may have trimmed the active segment and raised PageComplete. That is + /// fine - the span we then decline is simply left uncommitted, and the subsequent + /// asks again and fills from wherever the writer is now - but it + /// does mean this must not be used to speculatively "probe" a writer for capacity. + /// + /// + private static bool TryGetSpan(IBufferWriter writer, int length, out Span span) + { + span = writer.GetSpan(length); + return span.Length >= length; + } + + /// + /// A destination for a short, fixed-size burst: the writer's own span when it is long enough, and a stack + /// buffer when it is not. + /// + /// + /// + /// Keeps the zero-copy path for the overwhelmingly common case while staying correct when the hint is not + /// honoured, which explains. On the fallback path the composed bytes go through + /// , which loops until everything is written. + /// + /// + /// Writing past a short span was an at best, and - where the + /// destination length was handed to an encoder rather than derived from the span - a buffer overrun. + /// + /// + private readonly ref struct PrefixScratch + { + private readonly IBufferWriter _writer; + private readonly Span _span; + private readonly bool _direct; + + public PrefixScratch(IBufferWriter writer, int length, Span fallback) + { + _writer = writer; + + // both are sliced to exactly length: the slice costs nothing (every write is bounds-checked + // anyway) but it holds in Release, where a Debug.Assert does not. This is the PR's own thesis + // applied to its own buffer - do not trust that it is as long as you think. + if (TryGetSpan(writer, length, out var span)) + { + _span = span.Slice(0, length); + _direct = true; + } + else + { + _span = fallback.Slice(0, length); + _direct = false; + } + } + + /// Where to compose the bytes. + public Span Span => _span; + + /// Hand of to the writer. + public void Commit(int bytes) + { + if (_direct) + { + _writer.Advance(bytes); + } + else + { + _writer.Write(_span.Slice(0, bytes)); + } + } } private static ReadOnlySpan NullBulkString => "$-1\r\n"u8; @@ -247,10 +449,7 @@ internal static void WriteUnifiedPrefixedString(IBufferWriter writer, byte } else { - var span = writer.GetSpan(3 + Format.MaxInt32TextLen); - span[0] = (byte)'$'; - int bytes = WriteRaw(span, totalLength, offset: 1); - writer.Advance(bytes); + WriteCountPrefix(writer, (byte)'$', totalLength); if (prefixLength != 0) writer.Write(prefix); if (encodedLength != 0) WriteRaw(writer, value, encodedLength); @@ -270,10 +469,20 @@ internal static int WriteCrlf(Span span, int offset) [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static void WriteCrlf(IBufferWriter writer) { - var span = writer.GetSpan(2); - span[0] = (byte)'\r'; - span[1] = (byte)'\n'; - writer.Advance(2); + // NOTE the hint stays at 2. BuffersExtensions.Write asks with no hint at all, and CycleBuffer's + // "hint <= 0" branch returns anything non-empty - so a 1-byte segment tail would split the CRLF + // across segments rather than rolling to a fresh one. Protocol-correct either way, but it is a + // fragmentation change on a per-bulk-string path, and not one to make by accident. + if (TryGetSpan(writer, 2, out var span)) + { + span[0] = (byte)'\r'; + span[1] = (byte)'\n'; + writer.Advance(2); + } + else + { + writer.Write("\r\n"u8); // loops; a writer this stingy is pathological, but legal + } } [MethodImpl(MethodImplOptions.AggressiveInlining)] @@ -393,11 +602,16 @@ internal static unsafe void WriteRaw(IBufferWriter writer, string value, i fixed (char* cPtr = value) { int totalBytes; - if (expectedLength <= MaxQuickEncodeSize) + + // NOTE the second condition is load-bearing, not belt-and-braces: GetSpan's size is a hint, and + // expectedLength is handed to the encoder as the destination capacity. Against a shorter span that + // is a buffer OVERRUN rather than an exception - it writes past the end of someone else's memory. + // When the writer declines, fall through to the encoder loop below, which sizes every write from + // span.Length and so copes with whatever it is given. + if (expectedLength <= MaxQuickEncodeSize && TryGetSpan(writer, expectedLength, out var quick)) { // encode directly in one hit - var span = writer.GetSpan(expectedLength); - fixed (byte* bPtr = &MemoryMarshal.GetReference(span)) + fixed (byte* bPtr = &MemoryMarshal.GetReference(quick)) { totalBytes = Encoding.UTF8.GetBytes( cPtr, @@ -467,19 +681,11 @@ private static void WriteUnifiedPrefixedBlob(IBufferWriter writer, byte[]? } else { - var span = writer.GetSpan(3 + - Format - .MaxInt32TextLen); // note even with 2 max-len, we're still in same text range - span[0] = (byte)'$'; - int bytes = WriteRaw(span, prefix.LongLength + value.LongLength, offset: 1); - writer.Advance(bytes); + WriteCountPrefix(writer, (byte)'$', prefix.LongLength + value.LongLength); writer.Write(prefix); writer.Write(value); - - span = writer.GetSpan(2); - WriteCrlf(span, 0); - writer.Advance(2); + WriteCrlf(writer); } } @@ -490,11 +696,10 @@ private static void WriteUnifiedInt64(IBufferWriter writer, long value) // ${asc-len}\r\n = 4/5 (asc-len at most 2 digits) // {asc}\r\n = MaxInt64TextLen + 2 - var span = writer.GetSpan(7 + Format.MaxInt64TextLen); - + var scratch = new PrefixScratch(writer, 7 + Format.MaxInt64TextLen, stackalloc byte[MaxValueScratch]); + var span = scratch.Span; span[0] = (byte)'$'; - var bytes = WriteRaw(span, value, withLengthPrefix: true, offset: 1); - writer.Advance(bytes); + scratch.Commit(WriteRaw(span, value, withLengthPrefix: true, offset: 1)); } private static void WriteUnifiedUInt64(IBufferWriter writer, ulong value) @@ -506,13 +711,13 @@ private static void WriteUnifiedUInt64(IBufferWriter writer, ulong value) var len = Format.FormatUInt64(value, valueSpan); // ${asc-len}\r\n = 4/5 (asc-len at most 2 digits) // {asc}\r\n = {len} + 2 - var span = writer.GetSpan(7 + len); + var scratch = new PrefixScratch(writer, 7 + len, stackalloc byte[MaxValueScratch]); + var span = scratch.Span; span[0] = (byte)'$'; int offset = WriteRaw(span, len, withLengthPrefix: false, offset: 1); valueSpan.Slice(0, len).CopyTo(span.Slice(offset)); offset += len; - offset = WriteCrlf(span, offset); - writer.Advance(offset); + scratch.Commit(WriteCrlf(span, offset)); } private static void WriteUnifiedDouble(IBufferWriter writer, double value) @@ -523,13 +728,13 @@ private static void WriteUnifiedDouble(IBufferWriter writer, double value) // ${asc-len}\r\n = 4/5 (asc-len at most 2 digits) // {asc}\r\n = {len} + 2 - var span = writer.GetSpan(7 + len); + var scratch = new PrefixScratch(writer, 7 + len, stackalloc byte[MaxValueScratch]); + var span = scratch.Span; span[0] = (byte)'$'; int offset = WriteRaw(span, len, withLengthPrefix: false, offset: 1); valueSpan.Slice(0, len).CopyTo(span.Slice(offset)); offset += len; - offset = WriteCrlf(span, offset); - writer.Advance(offset); + scratch.Commit(WriteCrlf(span, offset)); #else // fallback: drop to string WriteUnifiedPrefixedString(writer, null, Format.ToString(value)); @@ -540,11 +745,10 @@ internal static void WriteInteger(IBufferWriter writer, long value) { // note: client should never write integer; only server does this // :{asc}\r\n = MaxInt64TextLen + 3 - var span = writer.GetSpan(3 + Format.MaxInt64TextLen); - + var scratch = new PrefixScratch(writer, 3 + Format.MaxInt64TextLen, stackalloc byte[MaxValueScratch]); + var span = scratch.Span; span[0] = (byte)':'; - var bytes = WriteRaw(span, value, withLengthPrefix: false, offset: 1); - writer.Advance(bytes); + scratch.Commit(WriteRaw(span, value, withLengthPrefix: false, offset: 1)); } private static void WriteUnifiedBlob(IBufferWriter writer, byte[]? value) @@ -570,24 +774,20 @@ private static void WriteUnifiedSpan(IBufferWriter writer, ReadOnlySpan quick; + if (value.Length <= MaxQuickSpanSize && (quick = writer.GetSpan(needed)).Length >= needed) + { + quick[0] = (byte)'$'; + writer.Advance(AppendToSpan(quick, value, 1)); + } + else + { + // too big for one span, or the writer declined the hint + WriteUnifiedSpanPiecewise(writer, value); + } } } @@ -618,10 +818,7 @@ private static void WriteUnifiedSequence(IBufferWriter writer, in ReadOnly private static void WriteUnifiedSequenceIterator(IBufferWriter writer, ReadOnlySequenceSegmentIterator seq) { - var span = writer.GetSpan(3 + Format.MaxInt32TextLen); - span[0] = (byte)'$'; - int bytes = WriteRaw(span, seq.Length, offset: 1); - writer.Advance(bytes); + WriteCountPrefix(writer, (byte)'$', seq.Length); while (seq.TryNext(out var memory)) { @@ -650,7 +847,8 @@ internal void WriteSha1AsHex(byte[]? value) { // $40\r\n = 5 // {40 bytes}\r\n = 42 - var span = writer.GetSpan(47); + var scratch = new PrefixScratch(writer, Sha1BulkStringLength, stackalloc byte[Sha1BulkStringLength]); + var span = scratch.Span; span[0] = (byte)'$'; span[1] = (byte)'4'; span[2] = (byte)'0'; @@ -668,7 +866,7 @@ internal void WriteSha1AsHex(byte[]? value) span[offset++] = (byte)'\r'; span[offset++] = (byte)'\n'; - writer.Advance(offset); + scratch.Commit(offset); } else { diff --git a/tests/StackExchange.Redis.Benchmarks/MessageWriterBenchmarks.cs b/tests/StackExchange.Redis.Benchmarks/MessageWriterBenchmarks.cs new file mode 100644 index 000000000..b95bfd1e5 --- /dev/null +++ b/tests/StackExchange.Redis.Benchmarks/MessageWriterBenchmarks.cs @@ -0,0 +1,112 @@ +using System; +using System.Buffers; +using BenchmarkDotNet.Attributes; +using BenchmarkDotNet.Configs; + +namespace StackExchange.Redis.Benchmarks; + +// Formatting only - no server. Two writers, because they answer different questions: +// +// Reusable - always returns the remainder of a 16KB buffer, so the hint is always honoured and the +// fallback branch is never entered. This measures what the CHECK costs on the hot path. +// Stingy - never returns more than 8 bytes, so every fixed-size burst takes the fallback. This +// measures what the fallback costs, and does not run at all before the fix (it throws). +// +// 8 is not arbitrary, and 64 would have been useless: the largest ask in SET user:1 marc is 23 (WriteHeader +// wants 9 framed command bytes + 3 + MaxInt32TextLen; WriteCountPrefix wants 3 + MaxInt64TextLen), so a +// 64-byte cap declines nothing and measures the direct path twice. 16 declines the two 23-byte asks but +// still does not throw on main, whose WriteHeader writes only 13 of the 23 it asked for. 8 is the smallest +// interesting value: every fallback here is exercised, and main faults on commandBytes.CopyTo(span[4..]). +// +// Run on both sides of the change; the numbers are only meaningful as a before/after pair, and only the +// Reusable ones can be compared against a build that predates the fix. +[Config(typeof(CustomConfig))] +[MemoryDiagnoser] +[GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)] +[CategoriesColumn] +public class MessageWriterBenchmarks +{ + private readonly RedisKey _key = "user:1"; + private readonly RedisValue _short = "marc"; + private RedisValue _long; + private RedisValue[] _several = null!; + private readonly Reusable _target = new(); + private readonly Stingy _stingy = new(); + + [GlobalSetup] + public void Setup() + { + _long = new string('x', 4096); + _several = ["marc", "EX", 300, 1.5, long.MinValue]; + } + + private MessageWriter Writer => new(null, CommandMap.Default, _target); + + [BenchmarkCategory("KeyValue"), Benchmark] + public int KeyValue() + { + _target.Reset(); + Message.Create(0, CommandFlags.None, RedisCommand.SET, _key, _short).WriteTo(Writer); + return _target.Written; + } + + [BenchmarkCategory("Mixed"), Benchmark] + public int MixedValueShapes() + { + _target.Reset(); + Message.Create(0, CommandFlags.None, RedisCommand.SET, _key, _several).WriteTo(Writer); + return _target.Written; + } + + [BenchmarkCategory("Large"), Benchmark] + public int LargeValue() + { + _target.Reset(); + Message.Create(0, CommandFlags.None, RedisCommand.SET, _key, _long).WriteTo(Writer); + return _target.Written; + } + + [BenchmarkCategory("Fallback"), Benchmark] + public int KeyValue_ShortSpans() + { + _stingy.Reset(); + Message.Create(0, CommandFlags.None, RedisCommand.SET, _key, _short) + .WriteTo(new MessageWriter(null, CommandMap.Default, _stingy)); + return _stingy.Written; + } + + /// Hands out at most 8 bytes at a time, so every fixed-size burst takes the fallback. + private sealed class Stingy : IBufferWriter + { + private const int Max = 8; + private readonly byte[] _buffer = new byte[64 * 1024]; + + public int Written { get; private set; } + + public void Reset() => Written = 0; + + public void Advance(int count) => Written += count; + + public Memory GetMemory(int sizeHint = 0) => new(_buffer, Written, Available); + + public Span GetSpan(int sizeHint = 0) => new(_buffer, Written, Available); + + private int Available => Math.Min(Max, _buffer.Length - Written); + } + + /// A reusable buffer writer, so buffer management is not part of the measurement. + private sealed class Reusable : IBufferWriter + { + private readonly byte[] _buffer = new byte[16 * 1024]; + + public int Written { get; private set; } + + public void Reset() => Written = 0; + + public void Advance(int count) => Written += count; + + public Memory GetMemory(int sizeHint = 0) => new(_buffer, Written, _buffer.Length - Written); + + public Span GetSpan(int sizeHint = 0) => new(_buffer, Written, _buffer.Length - Written); + } +} diff --git a/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs new file mode 100644 index 000000000..507e9b60a --- /dev/null +++ b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs @@ -0,0 +1,342 @@ +using System; +using System.Buffers; +using System.Collections.Generic; +using System.Text; +using RESPite.Messages; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// Writing must not assume honoured the size hint. +/// +/// +/// +/// The hint is a hint. CycleBuffer.GetUncommittedMemory caps it at 1k, sizes fresh segments from the +/// committed total rather than the hint, and - the case with no floor at all - hands back a dangling recycled +/// segment as-is, whatever length that happens to be. So a writer may legitimately return less than asked. +/// +/// +/// These use a writer that returns deliberately short spans, which is what the buffer does intermittently +/// under concurrency. Reported as ArgumentOutOfRangeException from MessageWriter.WriteUnifiedSpan. +/// +/// +public class BufferWriterHintTests +{ + /// + /// An that honours at most bytes per span, and + /// detects writes that run past the span it handed out. + /// + /// + /// The span comes from an isolated scratch array with a canary beyond it, checked on every Advance. A + /// plain growing buffer cannot catch this: an overrun would land harmlessly in the slack that follows, + /// and the test would pass while the real writer corrupts whatever is next in the pool. + /// + private sealed class StingyWriter(int max) : IBufferWriter + { + private const int Canary = 64; + private const byte CanaryByte = 0xCD; + + /// A generous but finite ceiling, so the "unconstrained" baseline stays allocatable. + private const int Unconstrained = 64 * 1024; + + private readonly int _max = Math.Min(max, Unconstrained); + private byte[] _scratch = []; + private int _handedOut; + private readonly List _written = []; + + /// Everything advanced so far, with the outstanding span checked first. + public byte[] Written + { + get + { + CheckCanary(); + return _written.ToArray(); + } + } + + public void Advance(int count) + { + if (count < 0 || count > _handedOut) + { + throw new InvalidOperationException($"Advance({count}) but only {_handedOut} was handed out"); + } + + CheckCanary(); + for (var i = 0; i < count; i++) _written.Add(_scratch[i]); + + // retire the scratch rather than just zeroing the count: what is left in it is written data, and + // a later canary check would otherwise read that as corruption + _scratch = []; + _handedOut = 0; + } + + // Two ways this harness is deliberately stricter than a real writer, both of which only matter if a + // caller misbehaves: a partial Advance discards the unadvanced tail rather than keeping it at the + // write position (uncommitted bytes, so nothing legitimate can observe the difference), and a second + // Advance without an intervening hand-out throws rather than silently double-counting. + + /// + /// Verify nothing was written past the span we handed out. + /// + /// + /// Called on Advance, on the next hand-out, and on reading the result - not just on Advance, because + /// a span written past and then abandoned without advancing would otherwise slip through, and this + /// harness is the load-bearing part of these tests. + /// + private void CheckCanary() + { + for (var i = _handedOut; i < _scratch.Length; i++) + { + if (_scratch[i] != CanaryByte) + { + throw new InvalidOperationException( + $"buffer overrun: first byte past a span of {_handedOut} is at +{i - _handedOut}"); + } + } + } + + public Memory GetMemory(int sizeHint = 0) => Hand(sizeHint).AsMemory(0, _handedOut); + + public Span GetSpan(int sizeHint = 0) => Hand(sizeHint).AsSpan(0, _handedOut); + + private byte[] Hand(int sizeHint) + { + CheckCanary(); // the previous span, if it was abandoned rather than advanced + + _handedOut = Math.Max(1, Math.Min(_max, sizeHint <= 0 ? _max : sizeHint)); + _scratch = new byte[_handedOut + Canary]; + _scratch.AsSpan(_handedOut).Fill(CanaryByte); + return _scratch; + } + } + + /// + /// Built directly rather than through TestHarness.Write, which pins the database to -1 and so + /// cannot issue the key-bearing commands this needs. + /// + private static string Render(int max, string command, params object[] args) + { + var writer = new StingyWriter(max); + var message = new RedisDatabase.ExecuteMessage(CommandMap.Default, 0, CommandFlags.None, command, args); + message.WriteTo(new MessageWriter(null, CommandMap.Default, writer)); + return Encoding.UTF8.GetString(writer.Written).Replace("\r\n", "|"); + } + + [Theory] + // The largest ask for this shape is 23 (WriteHeader wants framed command bytes + 3 + MaxInt32TextLen; + // WriteCountPrefix wants 3 + MaxInt64TextLen), so: 8 declines everything, 16 declines the 23-byte asks + // only, and 64 declines NOTHING - it is here as a control, not as pressure. 520 likewise for this + // payload; it only bites on the quick-span path, which needs a value to be large. + [InlineData(8)] + [InlineData(16)] + [InlineData(64)] + [InlineData(520)] + public void ShortSpansStillProduceCorrectFrames(int max) + { + var expected = Render(64 * 1024, "SET", "mykey", "myvalue"); + Assert.Equal("*3|$3|SET|$5|mykey|$7|myvalue|", expected); + Assert.Equal(expected, Render(max, "SET", "mykey", "myvalue")); + } + + [Theory] + [InlineData(8, 500)] + [InlineData(8, 512)] + [InlineData(8, 513)] + // NOTE 520 does NOT stress the string path: a 512-byte string asks for exactly 512, which a 520-cap + // writer honours, so these pass on main too. Kept as regression cover, not as evidence. + [InlineData(520, 512)] + [InlineData(520, 5000)] + public void ShortSpansStillProduceCorrectFramesForLargeValues(int max, int payload) + { + var value = new string('x', payload); + var expected = Render(64 * 1024, "SET", "mykey", value); + Assert.Equal(expected, Render(max, "SET", "mykey", value)); + } + + [Theory] + // The reported crash shape: WriteUnifiedSpan with a sizeable BINARY value, which asks for + // 5 + MaxInt32TextLen + length and previously wrote it unchecked. byte[] takes a different route from + // string, and the value-shapes test only reaches it with four bytes. + // + // The hint carries int32-text slack it rarely uses, so the cap has to be below the ACTUAL frame length + // (1 + digits + 2 + length + 2) to overrun, not merely below the hint. Cases marked (!) fail unfixed; + // the rest are boundary cover that passes either way. + [InlineData(8, 400)] // (!) asks 416, gets 8, writes 408 + [InlineData(400, 400)] // (!) asks 416, gets 400, writes 408 - overruns by 8 + [InlineData(519, 512)] // (!) $512\r\n...\r\n is exactly 520; one byte short + [InlineData(520, 512)] // exactly fits, so the slack in the hint absorbs the shortfall + [InlineData(520, 513)] // over MaxQuickSpanSize, so the looping big-value branch runs + [InlineData(64, 4096)] // likewise, well over + public void ShortSpansStillProduceCorrectFramesForLargeBinaryValues(int max, int payload) + { + var value = new byte[payload]; + for (var i = 0; i < value.Length; i++) value[i] = (byte)(i % 251); + + var expected = Render(64 * 1024, "SET", "mykey", value); + Assert.Equal(expected, Render(max, "SET", "mykey", value)); + } + + [Theory] + [InlineData(8)] + [InlineData(16)] + [InlineData(40)] + public void ShortSpansStillProduceCorrectFramesForEveryValueShape(int max) + { + // one of each writer path: int64, uint64-range, double, byte[], empty, and a long string + object[] args = + [ + "mykey", + 1234567890, + long.MinValue, + ulong.MaxValue, + 1.5d, + new byte[] { 1, 2, 3, 250 }, + "", + new string('x', 600), + ]; + + Assert.Equal(Render(64 * 1024, "SET", args), Render(max, "SET", args)); + } + + [Fact] + public void TheBaselineIsItselfCorrect() + { + // guards against both sides being equally wrong + Assert.Equal("*4|$3|SET|$5|mykey|$3|1.5|$2|-1|", Render(64 * 1024, "SET", "mykey", 1.5d, -1)); + } + + // ---- paths the message shapes above do not reach ------------------------------------------------ + // Called directly: these are internal, and routing to them through a message would pin the test to + // whichever command happens to use them today. + + private static string RenderDirect(int max, Action> write) + { + var writer = new StingyWriter(max); + write(writer); + return Encoding.UTF8.GetString(writer.Written).Replace("\r\n", "|"); + } + + [Theory] + [InlineData(8)] + [InlineData(46)] // one short of the 47 this path needs, so the fallback is forced + [InlineData(47)] + public void Sha1AsHexSurvivesShortSpans(int max) + { + // the tightest fit in the writer: 47 requested, exactly 47 written, no slack at all + var hash = new byte[20]; + for (var i = 0; i < hash.Length; i++) hash[i] = (byte)(i * 11); + + static Action> Write(byte[] hash) + => w => new MessageWriter(null, CommandMap.Default, w).WriteSha1AsHex(hash); + + var expected = RenderDirect(64 * 1024, Write(hash)); + Assert.Equal(42, expected.Replace("|", "\r\n").Length - 5); // $40 CRLF + 40 hex + CRLF + Assert.Equal(expected, RenderDirect(max, Write(hash))); + } + + [Theory] + [InlineData(8)] + [InlineData(16)] + public void KeyspacePrefixesSurviveShortSpans(int max) + { + // WithKeyPrefix / channel prefixes: a real user-facing path, and prefixed writes are two-part + var prefix = Encoding.UTF8.GetBytes("tenant7:"); + + static Action> WriteString(byte[] prefix) + => w => MessageWriter.WriteUnifiedPrefixedString(w, prefix, "user:1"); + + Assert.Equal("$14|tenant7:user:1|", RenderDirect(64 * 1024, WriteString(prefix))); + Assert.Equal("$14|tenant7:user:1|", RenderDirect(max, WriteString(prefix))); + } + + [Theory] + [InlineData(8)] + [InlineData(16)] + public void MultiBulkHeaderWithPrefixSurvivesShortSpans(int max) + { + static Action> Write() + => w => MessageWriter.WriteMultiBulkHeader(w, 4, RespPrefix.Map); + + Assert.Equal("%2|", RenderDirect(64 * 1024, Write())); + Assert.Equal("%2|", RenderDirect(max, Write())); + } + + [Theory] + [InlineData(8)] + [InlineData(16)] + public void IntegerSurvivesShortSpans(int max) + { + // server-side only (toys/StackExchange.Redis.Server), but it is the same shape + static Action> Write() + => w => MessageWriter.WriteInteger(w, long.MinValue); + + Assert.Equal(":-9223372036854775808|", RenderDirect(64 * 1024, Write())); + Assert.Equal(":-9223372036854775808|", RenderDirect(max, Write())); + } + + [Theory] + [InlineData(8)] + [InlineData(16)] + [InlineData(22)] // one short of the int64 width a long count can need + public void CountPrefixIsSizedForALong(int max) + { + // DEFENSIVE, not a fix for a reachable bug. WriteMultiBulkHeader takes a long and hints + // 3 + MaxInt32TextLen (14) on main, which is short of the 23 a long can need - but nothing can + // reach it: argument counts come from array lengths, so they are int-bounded in practice. The + // shared WriteCountPrefix here sizes for the parameter type rather than for today's callers, + // and this pins that down so a future long-valued caller cannot reintroduce a short hint. + static Action> Write() + => w => MessageWriter.WriteMultiBulkHeader(w, long.MaxValue); + + Assert.Equal("*9223372036854775807|", RenderDirect(64 * 1024, Write())); + Assert.Equal("*9223372036854775807|", RenderDirect(max, Write())); + } + + // ---- the repo's own writer, with no synthetic writer involved ------------------------------------ + + /// + /// BlockBuffer under-delivers deterministically, so this needs no contrived writer at all. + /// + /// + /// BlockBuffer.GetBuffer clamps the hint to [16, 128] and then hands back whatever is left in the + /// block - its own comment says so: "this isn't an actual max, just a max of what we guarantee; we give + /// the caller whatever is left in the buffer". So any request above 128 is under-served whenever the + /// block has between 128 and 527 bytes remaining, which covers the quick-span path (up to 528) and the + /// string encode (up to 512). No concurrency, no recycled segments, no CycleBuffer - just capacity. + /// + /// writes through MessageWriter.BlockBuffer, so this is the shipped + /// path end to end. + /// + /// + [Theory] + [InlineData(400)] + [InlineData(450)] + [InlineData(500)] + public void BlockBufferUnderDeliversWithoutAnyContrivedWriter(int size) + { + var harness = new TestHarness(); + var value = new string('x', size); + + // several arguments in a row, so the block fills and a later one lands in the 128..527 window + object[] args = [value, value, value, value, value, value]; + + var frame = harness.Write("ECHO", args); + var text = Encoding.UTF8.GetString(frame).Replace("\r\n", "|"); + + Assert.StartsWith("*7|$4|ECHO|", text); + Assert.Equal(6, CountOccurrences(text, "$" + size + "|" + value + "|")); + } + + private static int CountOccurrences(string haystack, string needle) + { + var count = 0; + for (var i = haystack.IndexOf(needle, StringComparison.Ordinal); i >= 0; + i = haystack.IndexOf(needle, i + needle.Length, StringComparison.Ordinal)) + { + count++; + } + + return count; + } +}