Bound exclusive lock acquisition in SharedLock to fail fast on a dead… - #3882
Bound exclusive lock acquisition in SharedLock to fail fast on a dead…#3882mdsarfarazulh wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df39962fe2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
df39962 to
a288865
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2888654b5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| throw new RedisException("Timed out after " + TimeUnit.NANOSECONDS.toMillis(exclusiveLockTimeoutNanos) | ||
| + "ms acquiring the exclusive SharedLock; the lock holder likely died inside the guarded region. " | ||
| + "The endpoint must be rebuilt to recover."); |
There was a problem hiding this comment.
When this timeout is reached from DefaultEndpoint.closeAsync() via cancelBufferedCommands(), the endpoint has already changed STATUS from open to closed before calling doExclusive; throwing synchronously here aborts the close path before the channel is closed or closeFuture is completed, so subsequent closeAsync() callers get the same permanently incomplete future instead of the endpoint being rebuilt or failing the close future. Handle this timeout in the close path, or complete the close future exceptionally, before leaving the endpoint in the closed state.
Useful? React with 👍 / 👎.
a288865 to
830de44
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 830de44. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 830de44607
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| long timeoutNanos = deadline - System.nanoTime(); | ||
| if (timeoutNanos > 0) { | ||
| try { | ||
| acquired = lock.tryLock(timeoutNanos, TimeUnit.NANOSECONDS); |
There was a problem hiding this comment.
When the caller enters closeAsync() with its interrupt flag already set while another thread is legitimately in a doExclusive section such as notifyChannelActive(), the new preflight tryLock() only handles the uncontended case; once it returns false, this interruptible timed lock throws immediately instead of waiting for normal contention to clear. That turns a non-leaked lock into a RedisException/exceptional close solely because the caller was already interrupted, whereas the old lock.lock() path ignored interrupts, so the bounded acquisition should preserve the flag but continue waiting until acquisition or timeout.
Useful? React with 👍 / 👎.
| if (cancelError != null) { | ||
| closeFuture.completeExceptionally(cancelError); |
There was a problem hiding this comment.
When closeAsync() is called while disconnected (channel == null) and there are commands in disconnectedBuffer, the new SharedLock timeout path makes cancelBufferedCommands() throw before drainCommands() runs; this branch only completes the close future exceptionally. Because no channel close/inactive callback follows in the null-channel case, those queued command futures remain in the buffer and never complete after close returns, so the timeout fallback should also drain/fail the buffered commands or otherwise complete them.
Useful? React with 👍 / 👎.
| ArgumentCaptor<io.netty.util.concurrent.GenericFutureListener> captor = ArgumentCaptor | ||
| .forClass(io.netty.util.concurrent.GenericFutureListener.class); |
There was a problem hiding this comment.
This new test inlines io.netty.util.concurrent.GenericFutureListener in both the captor type and .forClass(...). The repo's Java style requires imports over fully-qualified names except for genuine same-name clashes, and there is no conflicting GenericFutureListener in this file, so this should be imported and referenced by simple name.
AGENTS.md reference: AGENTS.md:L150-L152
Useful? React with 👍 / 👎.
|
@a-TODO-rov please have a look at this PR. |
| throw new RedisException("Timed out after " + TimeUnit.NANOSECONDS.toMillis(exclusiveLockTimeoutNanos) | ||
| + "ms acquiring the exclusive SharedLock; the exclusive lock was likely abandoned by a previous holder. " | ||
| + "The endpoint must be rebuilt to recover."); | ||
| } | ||
|
|
||
| throw new RedisException("Timed out after " + TimeUnit.NANOSECONDS.toMillis(exclusiveLockTimeoutNanos) | ||
| + "ms waiting for " + currentWriters + " shared writer(s) to drain while acquiring the " | ||
| + "exclusive SharedLock; a shared writer was likely leaked. The endpoint must be rebuilt to recover."); |
There was a problem hiding this comment.
What happens to the connection after exception is thrown ?
There was a problem hiding this comment.
When a RedisException is thrown due to the SharedLock timeout, the connection and endpoint are permanently closed and cleaned up:
- State Transition:
DefaultEndpoint.STATUSandRedisChannelHandler.CLOSEDtransition toST_CLOSEDbefore the lock is acquired, ensuringisClosed() == true,isOpen() == false, and any
subsequent command writes fail immediately ("Connection is closed"). - Watchdog Disarmed:
connectionWatchdog.prepareClose()is invoked before lock acquisition, preventing background reconnect loops. - Channel & Socket Teardown: Even when the lock fails fast,
DefaultEndpoint.closeAsync()proceeds to invokechannel.close(), releasing Netty pipeline handlers and the underlying TCP socket. Any
channel close failure is attached as a suppressed exception. - Buffered Commands Drained: Even on lock timeout, buffered/queued commands are drained and cancelled directly (lock-free) so awaiting callers do not hang.
- Pool / Listener Eviction:
RedisChannelHandlercatches the close future completion and firescloseEvents.fireEventClosed(this). Connection pools (BoundedAsyncPool,GenericObjectPool, cluster
node pools) receive the event/exception, evict the damaged connection, and create a fresh instance on subsequent requests. - Why Rebuilding is Advised: A
SharedLocktimeout indicates a thread terminated inside an exclusive section or leaked writer state, breaking internal concurrency invariants. Rebuilding the
endpoint/connection provides a fresh lock with clean state.
… holder or leaked writer redis#3804 redis#3880
830de44 to
4582c5e
Compare
2 participants
… holder or leaked writer #3804 #3880
Make sure that:
mvn formatter:formattarget. Don’t submit any formatting related changes.Note
Medium Risk
Changes core connection lifecycle locking and close behavior on all endpoints; incorrect timeout or error handling could affect close/reconnect under contention, but behavior is heavily tested and only alters previously infinite-wait failure modes.
Overview
Fixes #3804 and #3880 by replacing unbounded exclusive locking in
SharedLockwith a 30s default timeout (configurable via constructor for tests). Exclusive acquisition uses timedtryLockinstead of blocking forever when the internal guard lock was leaked by a dead thread, and writer draining uses a deadline-bounded spin instead of infinite CPU burn when shared writers or abandoned exclusive state (writers == -1) can never clear.On timeout,
doExclusivethrowsRedisExceptionwith messages that distinguish leaked guard locks, leaked shared writers, and abandoned exclusive mode, indicating the endpoint must be rebuilt.DefaultEndpoint.closeAsyncandClusterNodeEndpoint.closeAsyncnow catch failures from exclusive command drain/cancel during close, still proceed with channel teardown where applicable, and complete the close future exceptionally (with suppressed causes when both drain and channel close fail) rather than hanging Netty event-loop threads.Unit tests cover lock timeout scenarios, interrupted-thread close, and cluster node close behavior.
Reviewed by Cursor Bugbot for commit 4582c5e. Bugbot is set up for automated code reviews on this repo. Configure here.