fix(bigtable): prevent overflow if there are lots of pending vrpcs#13227
fix(bigtable): prevent overflow if there are lots of pending vrpcs#13227mutianf wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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.
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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
- In performance-sensitive code, prefer using explicit locks over the 'synchronized' keyword to protect shared state while ensuring thread safety and visibility.
When there are lots of pending vrpcs this call stack could happen:
This can cause JVM to crash.
Add a flag to guard the reentry.