Skip to content

fix(bigtable): prevent overflow if there are lots of pending vrpcs#13227

Open
mutianf wants to merge 1 commit into
googleapis:mainfrom
mutianf:overflow
Open

fix(bigtable): prevent overflow if there are lots of pending vrpcs#13227
mutianf wants to merge 1 commit into
googleapis:mainfrom
mutianf:overflow

Conversation

@mutianf
Copy link
Copy Markdown
Contributor

@mutianf mutianf commented May 19, 2026

When there are lots of pending vrpcs this call stack could happen:

SessionPoolImpl.tryDrainPendingRpcs()
  └── PendingVRpc.drainTo(...)
        └── VRpcImpl.start(...)
              └── session.startRpc(...) 
                    └── listener.onClose(...) 
                          └── SessionPoolImpl.onVRpcComplete(...) 
                                └── SessionPoolImpl.tryDrainPendingRpcs() [RECURSIVE LOOP]

This can cause JVM to crash.

Add a flag to guard the reentry.

@mutianf mutianf requested review from a team as code owners May 19, 2026 12:47
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a isDraining flag to SessionPoolImpl to prevent concurrent execution of the tryDrainPendingRpcs method. The review feedback correctly identifies a potential risk of deadlocks or lock contention because external code (rpc.drainTo) is executed while holding the monitor lock. It is recommended to refactor the logic to use explicit locks and perform the draining operations outside of the synchronized section to improve performance and safety.

Comment on lines +531 to 551
if (isDraining) {
return;
}
isDraining = true;
try {
while (!pendingRpcs.isEmpty()) {
if (pendingRpcs.peek().isCancelled) {
pendingRpcs.pop();
continue;
}
Optional<SessionHandle> handle = picker.pickSession();
if (!handle.isPresent()) {
break;
}
PendingVRpc<?, ?> rpc = pendingRpcs.removeFirst();
rpc.drainTo(handle.get());
}
PendingVRpc<?, ?> rpc = pendingRpcs.removeFirst();
rpc.drainTo(handle.get());
} finally {
isDraining = false;
}
}
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.

medium

The current implementation of tryDrainPendingRpcs performs synchronous calls to rpc.drainTo(handle.get()) while holding the lock on this. Calling external code while holding a monitor lock is discouraged as it can lead to deadlocks or extended lock contention. Furthermore, in performance-sensitive code like a session pool, prefer using explicit locks over the 'synchronized' keyword to protect shared state. Consider refactoring this to collect the PendingVRpc and SessionHandle pairs under an explicit lock, and then execute the drainTo calls outside of the locked section.

References
  1. In performance-sensitive code, prefer using explicit locks over the 'synchronized' keyword to protect shared state while ensuring thread safety and visibility.

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.

1 participant