feat(command): add zero-copy GetToBuffer and SetFromBuffer#3834
Merged
Conversation
Adds two new methods on the StringCmdable interface that let callers read
and write Redis string values directly into/from pre-allocated byte
buffers, eliminating the per-call payload allocation that the regular
Get/Set methods incur.
Public API:
GetToBuffer(ctx, key, buf) *ZeroCopyStringCmd
SetFromBuffer(ctx, key, buf) *StatusCmd
ZeroCopyStringCmd { Val() int; Bytes() []byte; Result() (int, error) }
Available on *Client, *ClusterClient, *Ring, *Conn and Pipeliner via
🛡️ Jit Security Scan Results✅ No security findings were detected in this PR
Security scan by Jit
|
ReadLine() returns a slice into bufio.Reader's internal buffer (the
ReadSlice fast path). The follow-up io.ReadFull / Discard calls on
r.rd can trigger bufio.fill(), which copies new source bytes over the
slot line[0] used to point at. The post-read
if line[0] == RespVerbatim { ... }
check on line 520 of reader.go could therefore observe a refilled byte
rather than the original response type. When that refilled byte
happened to be '=' (0x3D, RespVerbatim) — for example the head of a
bulk-string payload whose tail forced the refill — a regular `$<n>...`
Fix: capture line[0] into a local `isVerbatim` boolean before any
further read on r.rd, and use that local for both the pre-read length
check and the post-read prefix-stripping branch.
Regression test (internal/proto/reader_test.go): a payload sized to
exactly DefaultBufferSize, all '=' bytes. After ReadLine consumes the
header, bufio holds (bufsize - headerLen) payload bytes; the final
headerLen bytes io.ReadFull asks for force bufio.fill(), which places
payload-tail + trailing \r\n at position 0 of bufio.buf — i.e.
'=' at the slot line[0] points at. Against the buggy code the test
fails with n=32764 wanted 32768; with the fix all 10 ReadStringInto
tests pass.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6ebdc96. Configure here.
Three related cleanups to ReadStringInto and the cmd it backs: 1. Buffer-too-small now drains the unread payload + trailing CRLF before returning the error, so the underlying connection stays aligned for the next reply. Without this drain, a too-small caller buffer left the bulk payload sitting in the bufio stream — the connection would then be returned to the pool in a corrupted state, and the next user of that connection would parse the leftover bytes as a malformed response. The existing test only checked the first call's error and missed this; it now feeds two consecutive bulk-string replies and asserts the second parses cleanly. Verified failure-mode by reverting just the drain: the follow-up read fails with `can't parse reply="hello" reading string into buffer`. 2. Drop RespVerbatim handling from ReadStringInto. The only public caller is GetToBuffer, which issues GET, and GET never returns a verbatim string — RESP3 verbatim replies come from server-info commands like CLIENT INFO / DEBUG OBJECT / LOLWUT. Keeping the branch re-introduces a hazard class where the response-type byte read from a stale line[0] after a bufio refill can be misinterpreted as the verbatim format tag (the bug fixed in "fix(proto): avoid stale line[0] after bufio refill in ReadStringInto"). Removing the branch eliminates the class structurally and drops an O(n) memmove on the verbatim path (`copy(buf, buf[4:n])`). The unit test that previously asserted verbatim parsing is reshaped to assert verbatim now surfaces as an error, and the BulkStringStartingWithEqRefill regression test is kept (and its comment updated) as a guard against re-introducing verbatim support without re-introducing the bug. 3. Add a godoc paragraph to ZeroCopyStringCmd.Clone explaining the deep-copy semantics (clone gets a freshly allocated buffer, not a slice of the caller's memory) and noting that Clone is unreachable through normal client flows: it is only called from cluster fan-out routing in osscluster_router.go for multi-shard commands like DBSIZE / KEYS / FLUSHDB, and ZeroCopyStringCmd is only produced by GetToBuffer which issues GET — a single-key command never fanned out. Combined with NoRetry()=true blocking the retry path, Clone exists only to satisfy the Cmder interface and to keep the cmd usable if a future caller does fan out.
Three small follow-ups to ZeroCopyStringCmd identified during a full
review of the zero-copy buffer code path:
1. Clone() now returns a marked clone whose readReply drains the
network reply and surfaces an explicit error rather than producing
silently-wrong results. Cloning a ZeroCopyStringCmd has no
well-defined semantics: the cmd writes into caller-owned memory
(the buf passed to GetToBuffer), and a clone can neither share that
buf (sibling clones would race, last-writer wins) nor allocate its
own buf (the result would be invisible to whoever asked for the
original cmd's reply). The previous implementation chose the second
path and lost the result; this commit chooses to fail loudly.
The Cmder interface requires Clone, so we still return a clone.
The clone is flagged via a new private field, and readReply checks
that flag, calls rd.DiscardNext() to keep the connection aligned,
and returns a clear error ("redis: ZeroCopyStringCmd cannot be
cloned (cmd writes into caller-owned memory)").
Pre-setting cmd.err in Clone would NOT work because pipelineReadCmds
overwrites it with whatever readReply returns — the drain-and-error
path is what actually survives the framework.
In practice this path is unreachable through normal client flows:
Clone is only called from cluster fan-out routing in
osscluster_router.go for multi-shard commands (DBSIZE / KEYS /
FLUSHDB), and ZeroCopyStringCmd is only produced by GetToBuffer
which issues GET — a single-key command routed to one shard, never
fanned out. Combined with NoRetry()=true blocking the retry path,
the clone path is exercised only by explicit caller code.
A new Ginkgo spec asserts both that processing a clone returns the
expected error and that a follow-up GetToBuffer on the same client
succeeds (the drain keeps the connection usable).
2. readReply now resets cmd.n = 0 on entry so that an error after a
previous successful run can't leak the old data through Bytes().
Cheap defensive belt-and-braces — the caller is supposed to check
Err() first, but this removes a stale-state failure mode entirely.
3. The ZeroCopyStringCmd type godoc previously claimed "no data is
consumed beyond the header line" on the buffer-too-small path.
That stopped being true after the previous commit added the drain
to ReadStringInto. Updated to match reality.
Verified end-to-end against Redis 8.8 in Docker:
orig GetToBuffer: OK
cloned cmd processing: errored as expected
follow-up GetToBuffer: OK — connection stays aligned
…path SetFromBuffer and Set(ctx, key, buf, 0) dispatch to the same `case []byte` branch in proto.Writer and produce byte-identical output on the wire. The "zero-copy on send" property comes from bufio.Writer.Write bypassing its internal buffer for payloads larger than that buffer — a property of bufio, not of SetFromBuffer. Set([]byte) gets the same behaviour for free. Without this clarification, callers reading the godoc could reasonably infer that SetFromBuffer is a performance optimisation over Set, choose it for that reason, and be disappointed when the bench numbers come back indistinguishable from Set([]byte). The added paragraph spells out the actual rationale (API symmetry with GetToBuffer for code that uses the buffer-based pattern on both sides) and tells callers who don't care about that symmetry that Set(ctx, key, buf, 0) is equally efficient. No behaviour change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Adds two new methods on the StringCmdable interface that let callers read and write Redis string values directly into/from pre-allocated byte buffers, eliminating the per-call payload allocation that the regular Get/Set methods incur.
Public API:
Available on
*Client,*ClusterClient,*Ring,*ConnandPipeliner.Also added examples.
Note
Medium Risk
Touches core RESP read/write and connection alignment on errors; behavior is well-tested but callers must size buffers correctly and accept no automatic retry for GetToBuffer (including in pipelines).
Overview
Adds
GetToBufferandSetFromBufferonStringCmdableso GET/SET can use caller-owned[]byteinstead of allocating a full payload on each call.ZeroCopyStringCmdreads bulk replies via newproto.Reader.ReadStringInto, with buffer-too-small errors that drain the connection,NoRetry()(partial reads would corrupt the buffer), and a clone path that drains and errors rather than writing into an undefined destination.SetFromBufferis documented as wire-equivalent toSetwith[]byte; the win on reads is the main allocation story. Integration tests cover round-trips up to 10 MiB, undersized buffers, and cloned commands; proto tests guard drain-on-error and avoid verbatim RESP handling that could mis-parse after bufio refill.Also ships a zerocopy-buffer example (pipelines, shared-buffer hazards), CI-capped benchmarks vs
Get/Setand a raw-socket floor, and extendsmake benchwith an 11m timeout for the heavier matrix.Reviewed by Cursor Bugbot for commit c76a7e3. Bugbot is set up for automated code reviews on this repo. Configure here.