Skip to content

feat(command): add zero-copy GetToBuffer and SetFromBuffer#3834

Merged
ndyakov merged 9 commits into
masterfrom
ndyakov/zerocopy-buffer-commands
Jun 11, 2026
Merged

feat(command): add zero-copy GetToBuffer and SetFromBuffer#3834
ndyakov merged 9 commits into
masterfrom
ndyakov/zerocopy-buffer-commands

Conversation

@ndyakov

@ndyakov ndyakov commented May 28, 2026

Copy link
Copy Markdown
Member

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.

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 GetToBuffer and SetFromBuffer on StringCmdable so GET/SET can use caller-owned []byte instead of allocating a full payload on each call. ZeroCopyStringCmd reads bulk replies via new proto.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.

SetFromBuffer is documented as wire-equivalent to Set with []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/Set and a raw-socket floor, and extends make bench with 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.

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-ci

jit-ci Bot commented May 28, 2026

Copy link
Copy Markdown

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

Comment thread internal/proto/reader.go Outdated
ndyakov added 4 commits May 30, 2026 10:14
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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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.

Comment thread string_commands.go
ndyakov added 4 commits May 30, 2026 13:10
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.

@ofekshenawa ofekshenawa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ndyakov
ndyakov merged commit 74d9bb0 into master Jun 11, 2026
40 of 41 checks passed
@ndyakov
ndyakov deleted the ndyakov/zerocopy-buffer-commands branch June 11, 2026 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants