Skip to content

Retain Redis argument storage until a transaction completes - #3665

Open
wasphin wants to merge 1 commit into
apache:masterfrom
wasphin:fix-redis-transaction-argument-storage
Open

wasphin wants to merge 1 commit into
apache:masterfrom
wasphin:fix-redis-transaction-argument-storage

Conversation

@wasphin

@wasphin wasphin commented Oct 25, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Problem Summary:

A Redis transaction can retain argument views across separate reads. The
connection currently clears its argument arena at the end of a read even
when a transaction handler is still active, so later reads can overwrite
those arguments before the transaction executes.

What is changed and the side effects?

Changed:

  • Retain the argument arena while a transaction handler is active.
  • Document the argument-string lifetime during a transaction.
  • Add a real connection regression with separate requests and repeated
    transactions that retain argument views.

Side effects:

  • Performance effects: Argument storage is retained until the transaction
    completes; its memory use depends on the accumulated transaction data.
  • Breaking backward compatibility: No API signature changes.

Check List:

@chenBright
chenBright requested a balanced review from Copilot October 26, 2026 03:06

This comment was marked as low quality.

@wwbmmm wwbmmm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[replied by brpc-oncall robot] The fix correctly keeps the per-connection argument arena alive while a transaction handler is active. Two points should be settled before merge: the memory cost is paid unconditionally for every in-flight transaction (including handlers that copy their arguments), and the new lifetime documentation should state its scope and exact end point.

LOG_IF(WARNING, socket->Write(&sendbuf, &wopt) != 0)
<< "Fail to send redis reply";
if(ctx->parser.ParsedArgsSize() == 0) {
if (ctx->parser.ParsedArgsSize() == 0 && !ctx->transaction_handler) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[replied by brpc-oncall robot] Retention here is unconditional while a transaction handler exists: not only the queued command arguments but also every RedisReply output(&ctx->arena) built during the transaction now stays allocated until the transaction ends. Handlers that already copy the arguments (which is what the in-tree MultiTransactionHandler and RedisServiceImpl examples do) get no benefit and see their per-connection memory grow with all accumulated transaction data, and a client controls how long a transaction stays open, so this is now an unbounded, client-driven growth per connection. Please at least extend the comment above this check to state why the arena must be retained and who pays for it, and consider whether a cap on retained transaction data (or an opt-in for handlers that need the views) is needed to keep the memory behavior bounded.

Comment thread src/brpc/redis.h
// just like what redis-server does.
// `args' is the array of request command. For example, "set somekey somevalue"
// corresponds to args[0]=="set", args[1]=="somekey" and args[2]=="somevalue".
// The argument strings remain valid until the current transaction completes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[replied by brpc-oncall robot] This comment sits in the generic Run() documentation, but the guarantee only holds for commands dispatched to the handler created by NewTransactionHandler() (plus the command whose Run() returned REDIS_CMD_CONTINUE). For a normal handler the arguments are only valid during Run(). Please scope the sentence explicitly, and make the end point precise: the storage is released once the read that completes the transaction finishes parsing, so the views must not be kept after the Run() call that returns REDIS_CMD_HANDLED (e.g. say "until Run() of the transaction handler that completes the transaction returns").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants