Tags: StackExchange/StackExchange.Redis
Tags
Do not assume GetSpan honoured the size hint (#3220) * Do not assume GetSpan honoured the size hint IBufferWriter<T>.GetSpan(sizeHint) guarantees one element, not sizeHint. The "should be at least this size" wording in the docs is left over from when the parameter was called minSize; it is advisory, and consuming code is expected to test what it got and fall back. The BCL does exactly that - BuffersExtensions loops and copies in slices rather than demanding one contiguous block. See CommunityToolkit/dotnet#1208. MessageWriter treated it as a guarantee throughout: take the span, write the full amount into it, advance by that amount. The looser contract exists for transports with page limits, which 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. It is also why this reproduces intermittently and under concurrency rather than deterministically. Reported as an ArgumentOutOfRangeException from WriteUnifiedSpan, but that is the benign manifestation and not the only site. Every fixed-size write had the same shape, and one of them is worse than an exception: var span = writer.GetSpan(expectedLength); fixed (byte* bPtr = &MemoryMarshal.GetReference(span)) Encoding.UTF8.GetBytes(cPtr, value.Length, bPtr, expectedLength); writer.Advance(expectedLength); expectedLength is passed to the encoder as the destination capacity rather than being derived from the span, so against a short span that is a buffer OVERRUN - it writes past the end of memory it does not own, silently. It now falls through to the encoder loop below it, which sizes every write from span.Length and already copes with whatever it is given. Every other site checks the length inline and hands the rare case to a separate NoInlining method that composes in a stack buffer and writes through BuffersExtensions.Write. That split is load-bearing, not style: a stackalloc in the caller's cold branch changes codegen for the whole method, and writing the fallbacks inline cost 20% on the simplest command shape. It is the same "inline-optimized for it-fits, pathological case doesn't inline" structure the BCL uses. Measured, formatting only, SET key value: 64.66ns before against 66.64ns after, so +3.1% - small error bars, so real rather than noise. That is the honest price of checking. Larger values come out ahead (203.7ns to 184.9ns), because sharing one length-prefix helper removed duplicated work. Sites fixed: both WriteHeader overloads, both WriteMultiBulkHeader overloads, WriteUnifiedPrefixedString, WriteUnifiedPrefixedBlob, WriteUnifiedInt64, WriteUnifiedUInt64, WriteUnifiedDouble, WriteInteger, WriteUnifiedSequenceIterator, the SHA1 bulk-string path, WriteUnifiedSpan and WriteCrlf. The encoder loop was already correct; the commented-out WriteUnifiedSequence is left alone. Tests use a writer that honours at most N bytes per span and places a canary past the span it hands out, so an overrun is detected rather than landing harmlessly in slack - a plain growing buffer passes while the real writer corrupts the next thing in the pool. 13 tests across the value shapes; 6 fail without the fix, with two distinct crash modes. Full suite against the docker topology: 6172 passed, 150 skipped, 1 failed - FailoverTests.SubscriptionsSurvivePrimarySwitchAsync, which fails identically on main. * Move the hint fallbacks out of line, and address review findings Restores the NoInlining split that was lost to a git checkout during benchmarking, plus the review findings: - WriteUnifiedInt64 passed MaxPrefixScratch (30) while requesting 27; now MaxValueScratch, which is what its doc comment says it is for. - PrefixScratch slices both spans to length, so it bounds-checks in Release rather than relying on a Debug.Assert that is gone there. - WriteCountPrefix is sized for an int64: the multi-bulk header, the blob prefix and the sequence iterator all format a long and previously asked for int32 width, so the direct path could be handed exactly 14 bytes and write 23. - WriteCrlf keeps its hint of 2 rather than falling to BuffersExtensions.Write, which asks with no hint at all and would split a CRLF across segments. - The test harness checks its canary on hand-out and on reading the result, not only on Advance, so a span written past and then abandoned cannot slip through. - Coverage for WriteSha1AsHex (the tightest fit in the file), keyspace prefixes, the prefixed multi-bulk header, WriteInteger, and a long-width count. * Benchmark the fallback path too, and correct the performance claims Adds a stingy writer to the benchmark, so the fallback branch is actually exercised: the Reusable writer always returns the remainder of a 16KB buffer and so only ever measures what the CHECK costs, which the previous table could not be read as the cost of the fix generally. Corrects the numbers, which were overstated in two directions: - The -9.2% on LargeValue was partly measurement drift. main's own figure moved between 187ns and 204ns across sessions, so the claim that sharing a length-prefix helper made large values faster is not supported and is dropped. - MixedValueShapes swung between -0.4% and +4.5% across runs. BenchmarkDotNet's error bars measure within-run precision, not between-run reproducibility, and at this scale the latter is several percent. Only the SET key value figure is reproducible: 64.48ns against 66.42ns, so about +3%, measured twice independently with matching filter sets. Note the filter set matters - including the fallback benchmark in the same run moved the figure to 70.60ns, so before-and-after runs must measure the same set. * Cover the reported binary shape; correct two overstated test labels - add ShortSpansStillProduceCorrectFramesForLargeBinaryValues: byte[] takes a different route from string through WriteUnifiedSpan, and the value-shapes test only reached it with four bytes. 400/512/513 at caps of 8/400/520. - note that [InlineData(520, 512)] on the string theory does not stress that path - a 512-byte string asks for exactly 512, which a 520-cap writer honours, so it passes unfixed. Kept as regression cover, not as evidence. - relabel CountPrefixIsSizedForALong as defensive: main's long overload really does hint int32 width, but argument counts come from array lengths, so no caller can reach it. - document that TryGetSpan returning false is not side-effect free, and the two ways StingyWriter is stricter than a real writer. * Make the binary straddle cases actually straddle 519/512 is the case that bites: the hint carries int32-text slack it rarely uses, so a cap has to fall below the real frame length (1 + digits + 2 + len + 2 = 520 here), not merely below the hint, to overrun. Marked which cases fail unfixed and which are boundary cover. * Keep the in-place length prefix for large values; make the stingy writer stingy Three findings from review: 1. WriteUnifiedSpanSlow was the NORMAL path for values over MaxQuickSpanSize, not a fallback, and called WriteCountPrefixSlow unconditionally - so every 4KB byte[] composed its $4096\r\n into a stack buffer and handed it to a hintless Write, costing a copy and risking the prefix splitting across segments: the same fragmentation just fixed in WriteCrlf. The string path never had this. Now calls the checked WriteCountPrefix, and is renamed WriteUnifiedSpanPiecewise since '...Slow' described only one of its two entry reasons. The two header fallbacks likewise: the ask that was declined was larger than 23, so the smaller one may still land in place. 2. The Stingy benchmark writer capped at 64, but the largest ask in SET user:1 marc is 23 - so it declined nothing and measured the direct path twice. 16 forces the fallback but still does not fault on main; 8 does both. Same correction to ShortSpansStillProduceCorrectFrames, whose comment claimed 64 applied pressure. 3. MaxPrefixScratch was dead - every stackalloc uses MaxValueScratch or Sha1BulkStringLength. Deleted, wording folded into MaxValueScratch. WriteCountPrefixSlow is now reached only from WriteCountPrefix. * Bring two doc comments up to date with the previous commit - WriteUnifiedSpanPiecewise keeps NoInlining, but no longer for the reason documented on WriteCountPrefixSlow: there is no stackalloc left in it to perturb the caller's codegen. It is now simply about keeping the large-value body out of the hot small-value WriteUnifiedSpan frame. Said so. - WriteHeaderSlow/WriteHeaderUnframedSlow inherited a summary saying they 'compose in a stack buffer and let the looping write place it'. Since the last commit they call the checked WriteCountPrefix, which only composes if it is itself declined. The 'fallback when the writer declined the hint' half was still accurate; only the mechanism half was stale.
Fix tag-triggered releases: accept unprefixed tags, keep 3.2.0 reacha… …ble (#3217) The 3.2.0 release failed its "Verify tag matches computed version" guard with "Tag '3.2.0' does not match the computed version '3.2.0-g8460293104'". Cause: publicReleaseRefSpec still carried the v2-era "^refs/tags/v\d+\.\d+". The whole v3 line tags without the "v" (3.0.0 ... 3.1.31, 3.2.0), so on a release event - where GITHUB_REF is refs/tags/3.2.0 - nbgv did not consider the build a public release and appended the "-g<commit>" suffix. This never bit us before because release.yml is new (#3213); every earlier v3 package was pushed from a main build, where the refspec did match. The stray 3.1.1-g7441909d06 on nuget.org is the same failure mode escaping under the old pipeline. Relax the refspec to "^refs/tags/v?\d+\.\d+" so both spellings are public. Because this commit adds to the commit height, drop versionHeightOffset to -3 so the release commit still computes as 3.2.0 rather than skipping to 3.2.1. versionHeightOffsetAppliesTo is already "3.2", so the height is not reset. Verified with nbgv on this commit: refs/tags/3.2.0, refs/tags/v3.2.0 and refs/heads/main all compute NuGetPackageVersion 3.2.0.
Docs site: name the maintainer in the footer, not the repository owner Cayman's footer renders "<repository> is maintained by <site.github.owner_name>", and that name comes from whoever owns the repository, so seredis.dev read "StackExchange.Redis is maintained by StackExchange". That is a statement about ownership rather than maintenance, and it isn't true of this project. jekyll-github-metadata merges anything set under `github:` in _config.yml over the values it reads from the API (SiteGitHubMunger#github_namespace is `drop.merge(@original_config)`), so overriding owner_name/owner_url is enough - no layout override, and no copy of the theme's markup to keep in step. Nothing else on the site reads either key; Cayman uses them only in that footer span. The alternative was dropping the line entirely via `is_project_page: false`, but Cayman guards its "View on GitHub" button with the same flag, which we do want.
Add possibility to use different passwords for Sentinel and Redis host ( #1698) (#3140) * Add possibility to use different passwords for sentinel and redis (#1698) * Use correct Sentinel properties in ConfigurationOptions.ToString() * Removed unused usings * - added sentinel user/pw fields to unit test that checks ConfigurationOptions - fixed typos, so fields, not properties, are used on the correct places * Fix test * Use a private _isSentinel instead of serverType * Moved Sentinel credentials usage to HandshakeAsync * another property <-> field mixup fixed Co-authored-by: Marc Gravell <marc.gravell@gmail.com> * another property <-> field mixup fixed Co-authored-by: Marc Gravell <marc.gravell@gmail.com> * Moved Sentinel credentials usage back to SentinelPrimaryConnect(Async) in ConnectionMultiplexer.Sentinel * Restore CodeAnalysis using for [Experimental]; move new API entries to Unshipped --------- Co-authored-by: Marc Gravell <marc.gravell@gmail.com>
Config strings: '!@name' is a Linux abstract Unix socket (socat/syste… …md '@' convention) (#3165) '!name' has always meant a Unix domain socket, but '!@foo' silently produced a PATHNAME socket literally named '@foo' -- the wrong socket, found by nothing. The parse now maps '@' after '!' to the kernel's leading-NUL spelling, Linux-gated (no other platform has the namespace; elsewhere '@' stays a literal filename, matching redis-cli). ToString round-trips for free: UnixDomainSocketEndPoint renders abstract names back as '@name'. Tests: FormatTests gains UDS parse/round-trip coverage (there was NONE, even for pathnames) -- pathname and abstract cells, the latter Linux-gated. Verified live end-to-end besides: a ConnectionMultiplexer built from ConfigurationOptions.Parse("!@se-abs-test,abortConnect=true") against a Garnet listening on the same abstract name, SET/GET round-trip clean, no filesystem footprint.
Throw a clear error when an Execute command contains whitespace (#3122) * Throw a clear error when an Execute command contains whitespace Execute("ACL SETUSER x") passes a whole command line as the single command token, which gets sent as one unknown command and comes back as an opaque server error. Since a redis command token never contains internal whitespace, this is always a caller mistake, so the ExecuteMessage constructor now fails fast with an ArgumentException-style RedisCommandException that names the offending command and shows the correct token-per-argument form. Resolves #2689. Signed-off-by: Arpit Jain <arpitjain099@gmail.com> * test for simple space only limit to simple space; if people are being *that* creative, that's on them * Remove test case for 'echo\thello' command Removed test case for command with tab character. --------- Signed-off-by: Arpit Jain <arpitjain099@gmail.com> Co-authored-by: Marc Gravell <marc.gravell@gmail.com>
PreviousNext