Skip to content

[fix][client] Defer op cmd release to the write event loop on send timeout - #26456

Open
nodece wants to merge 1 commit into
apache:masterfrom
nodece:fix-send-timeout-inflight-write-race
Open

[fix][client] Defer op cmd release to the write event loop on send timeout#26456
nodece wants to merge 1 commit into
apache:masterfrom
nodece:fix-send-timeout-inflight-write-race

Conversation

@nodece

@nodece nodece commented Sep 3, 2026

Copy link
Copy Markdown
Member

Motivation

The send-timeout path (failPendingMessages with cnx() == null) released op.cmd and recycled the op inline on the timer thread while the message frame may still have been queued for writing on a connection event loop: the write callback sits behind a reconnect/disconnect storm, or the buffers are already in the channel outbound buffer. Releasing the buffers from the wrong thread could return them to the pool while the in-flight write is still reading them, letting new batches overwrite the frame content and corrupting the wire bytes (broken checksum / lost frame sync on the broker). The existing cnx != null branch already deferred this cleanup to the event loop; the null (reconnect-window) branch did not.

Modifications

  • OpSendMsg tracks the event loop each cmd was last handed to for writing (writeEventLoop), and cmd is now volatile so stale callbacks can never observe a released cmd.
  • releaseOpCmdAndRecycle defers the cmd release and op recycle to that event loop (serialized after the in-flight write), with an inline fallback when the loop is shutting down. releaseOpCmd clears op.cmd before releasing so a stale callback always fails its guard.
  • WriteInEventLoopCallback now skips stale writes (op re-sent on another connection or already disposed) and only drops the reference it took, instead of writing a released buffer or mutating a recycled op.
  • failPendingMessages fails inline when the connection event loop rejects the deferred task (shutting down during reconnect churn) instead of leaving the pending messages queued.
  • ackReceived / recoverChecksumError / recoverNotAllowedError now go through the same write-loop-aware release.

Verifications

  • ProducerImplTest 9/9 passing (includes 4 regression tests for the timeout + in-flight-write race).
  • checkstyleMain / checkstyleTest passing.

…meout

Motivation

The send-timeout path (failPendingMessages with cnx() == null) released
op.cmd and recycled the op inline on the timer thread while the message
frame may still have been queued for writing on a connection event loop:
the write callback sits behind a reconnect/disconnect storm, or the
buffers are already in the channel outbound buffer. Releasing the buffers
from the wrong thread could return them to the pool while the in-flight
write is still reading them, letting new batches overwrite the frame
content and corrupting the wire bytes (broken checksum / lost frame sync
on the broker). The existing cnx != null branch already deferred this
cleanup to the event loop; the null (reconnect-window) branch did not.

Modifications

- OpSendMsg tracks the event loop each cmd was last handed to for writing
  (writeEventLoop), and cmd is now volatile so stale callbacks can never
  observe a released cmd.
- releaseOpCmdAndRecycle defers the cmd release and op recycle to that
  event loop (serialized after the in-flight write), with an inline
  fallback when the loop is shutting down. releaseOpCmd clears op.cmd
  before releasing so a stale callback always fails its guard.
- WriteInEventLoopCallback now skips stale writes (op re-sent on another
  connection or already disposed) and only drops the reference it took,
  instead of writing a released buffer or mutating a recycled op.
- failPendingMessages fails inline when the connection event loop rejects
  the deferred task (shutting down during reconnect churn) instead of
  leaving the pending messages queued.
- ackReceived / recoverChecksumError / recoverNotAllowedError now go
  through the same write-loop-aware release.
@nodece

nodece commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

Sharing the end-to-end validation I ran against the exact head of this PR (6b94c813bc3), in case it's useful for the review.

Setup verification: built from a clean worktree at the PR head (bit-exact match with the PR ref, zero uncommitted changes, clean rebuild with reproducible jar hashes); bytecode-checked that the jar contains the writeEventLoop change and the untouched two-write encoder; confirmed at runtime via -verbose:class that both classes load from those jars.

Harness: standalone broker + proxy as separate docker containers, producer through the proxy with batchingMaxMessages=1000 / batchingMaxBytes=128KB / batchingMaxPublishDelay=1ms, sendTimeout(3, SECONDS), no compression, 4 threads x 8KB payloads, client JVM -XX:MaxDirectMemorySize=48m; two hard proxy restarts (docker restart) per 50s round during active traffic; broker log checked for Failed to verify checksum / TooLongFrameException / unknown tag type.

Results (freshly recreated containers for each phase):

  • unmodified master (merge base): corruption in round 1 — Failed to verify checksum followed by TooLongFrameException, where the reported frame length is the message payload bytes (i.e. the parser is reading mid-payload), and the connection gets closed
  • this PR alone: 22/22 rounds, zero broker-side corruption — including one round with ~49k client send errors during a violent restart window: the timeout path fired heavily, and every failure was recoverable via retry/reconnect, with no corrupt bytes on the wire

No throughput difference vs. master under a normal direct-memory configuration.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant