Repository navigation
Conversation
…ining SessionPoolImpl.close() previously left the watchdog running until awaitTerminated() was called. Callers that close a pool without calling awaitTerminated() (such as TableBase.close() when an idle table is evicted from compat.Util.createSessionMap) leaked the self-rescheduling watchdog on the client's shared HashedWheelTimer, retaining the closed SessionPoolImpl in memory. Close the watchdog as soon as all sessions drain after close() (immediately if the pool is empty, or in onSessionClose when the last session terminates), and guard onSessionGoAway against creating replacement sessions after the pool is closed.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that the watchdog timer is closed when the session pool is closed and all sessions have drained, preventing potential timer leaks when callers only invoke close() without calling awaitTerminated(). A new test is added to verify this behavior. The review feedback suggests using System.nanoTime() instead of System.currentTimeMillis() in the test's wait loop to ensure monotonic time measurements and avoid sensitivity to system clock adjustments.
| long deadlineMs = System.currentTimeMillis() + 10_000; | ||
| while (!sessionPool.watchdog.isClosed() && System.currentTimeMillis() < deadlineMs) { | ||
| Thread.sleep(10); | ||
| } |
There was a problem hiding this comment.
Using System.currentTimeMillis() to measure deadlines or elapsed time is sensitive to system clock adjustments (such as NTP updates). It is safer and more robust to use System.nanoTime() for monotonic time measurements in test wait loops.
| long deadlineMs = System.currentTimeMillis() + 10_000; | |
| while (!sessionPool.watchdog.isClosed() && System.currentTimeMillis() < deadlineMs) { | |
| Thread.sleep(10); | |
| } | |
| long deadlineNano = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); | |
| while (!sessionPool.watchdog.isClosed() && System.nanoTime() < deadlineNano) { | |
| Thread.sleep(10); | |
| } |
SessionPoolImpl.close()previously left the pool'sWatchdogrunning untilawaitTerminated()was called. Callers that close a pool without callingawaitTerminated()(such asTableBase.close()when an idle table is evicted fromcompat.Util.createSessionMap) leaked the self-rescheduling watchdog on the client's sharedHashedWheelTimer, retaining the closedSessionPoolImplin memory.Close the watchdog as soon as all sessions drain after
close()(immediately if the pool is empty, or inonSessionClose()when the last session terminates), and guardonSessionGoAway()against creating replacement sessions after the pool is closed.