Stop the buffer pool pruner thread when the pool is empty - #2036
Draft
rozza wants to merge 2 commits into
Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes PowerOfTwoBufferPool’s pruning mechanism so the pruner thread is only created when there is actual work (buffers in the pool) and can terminate once the pool is drained, addressing classloader retention issues in app servers (JAVA-6279 / GitHub #2029).
Changes:
- Replace fixed-rate pruning with “schedule-next-only-if-needed” pruning so the pruner stops when the pool is empty.
- Configure the pruner executor to allow core thread timeout and to remove canceled tasks, enabling thread termination after idle periods.
- Add/adjust unit tests to assert: no thread for empty pool, thread terminates when drained, and pruning can resume after termination.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| driver-core/src/main/com/mongodb/internal/connection/PowerOfTwoBufferPool.java | Reworks pruning scheduling to avoid a forever-running pruner thread and allow executor threads to time out when idle. |
| driver-core/src/test/unit/com/mongodb/internal/connection/PowerOfTwoBufferPoolTest.java | Adds tests validating pruner thread lifecycle (no start on empty, terminate when drained, restart on later activity) and makes pruning assertions non-sleep-based. |
Suppressed comments (1)
driver-core/src/test/unit/com/mongodb/internal/connection/PowerOfTwoBufferPoolTest.java:159
- This assertion calls pool.getBuffer(256) but does not release the returned ByteBuf, which can leave the buffer checked out for the rest of the test run.
assertSame("the pool must keep the buffer because it does not prune", wrapped, pool.getBuffer(256).asNIO());
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+143
to
+144
| assertTrue("a buffer released after termination should still be pruned", | ||
| await(() -> pool.getBuffer(256).asNIO() != wrapped)); |
|
|
||
| private void schedulePrune() { | ||
| try { | ||
| pruner.schedule(this::pruneAndRescheduleIfNeeded, maxIdleTimeNanos / 2, TimeUnit.NANOSECONDS); |
PROBLEM The driver starts a thread that removes idle buffers from PowerOfTwoBufferPool.DEFAULT. That thread never stops. A thread that runs forever keeps the class loader of all driver classes in memory. The static data of those classes also stays in memory. Then an application server cannot unload an application, and the memory of that application stays in use. Users report this behavior in GitHub issue 2029 and in JAVA-5643. CAUSE DEFAULT is a static field, and it calls enablePruning() during class initialization. That method schedules a periodic task. The executor starts the worker thread for the first task, but the pool is empty at that time. Therefore the thread has no work, and it continues to wake up forever. The thread keeps the class loader in memory because the JVM captures the class loader when it constructs the thread. The data that the thread holds is not the cause. For this reason, a change to the references of the thread cannot release the class loader. The thread must stop. SOLUTION enablePruning() now sets a flag. It does not schedule a task. The release() method schedules one prune when it puts a buffer into the pool. Each prune schedules the next prune, but only if the pool still holds a buffer. The pruner does not schedule the next prune when the pool becomes empty. The pruner also uses these settings on its executor: - allowCoreThreadTimeOut(true), so that the worker thread can stop - a keep-alive time of maxIdleTime / 2 - setRemoveOnCancelPolicy(true) The work queue becomes empty after the last prune. Then the keep-alive time expires, and the worker thread stops. A later call to release() schedules a new prune, and the executor starts a new thread. Two threads must not schedule a prune at the same time. A prune must also not stop while a different thread adds a buffer to the pool. The AtomicBoolean pruningScheduled prevents both conditions. The prune clears the flag, and then it examines the pool one more time before it stops. DRAWBACKS The pool releases the class loader about 90 seconds after the last buffer release. The default value of maxIdleTime is one minute. A buffer is old enough to remove only after two prunes, and the keep-alive time adds 30 seconds. The class loader stays in memory during that period. A tool that examines threads at the moment of an undeployment can still find a live thread. A pool that becomes idle and then busy starts a new thread. This adds a small cost. A test measures this cost. A busy pool keeps one thread, because new work arrives before the keep-alive time expires. prune() keeps its current behavior after an error. It writes a log message and throws the error again, and the pruner does not start again. This behavior is the same as before this change. PRIOR ART Netty has the same problem and uses the same solution. GlobalEventExecutor is a single-thread singleton. It starts its thread when work arrives, and it stops the thread when the task queue stays empty for a quiet period. The deprecated ThreadDeathWatcher class uses the same pattern. The steps that this change uses to clear and then examine the flag follow GlobalEventExecutor.TaskRunner. Netty also sets the context class loader of a new thread to null. See netty#7290 and JDK-7008595. That change corrects a different problem, which is a driver thread that keeps an application class loader in memory. This commit does not include that change. TESTS New tests in PowerOfTwoBufferPoolTest show three results. An empty pool starts no thread. The thread stops after the pruner empties the pool. The pruner starts again after the thread stops. A separate test harness measures class loader retention. That harness loads the driver into a child class loader, opens a MongoClient, closes it, and then waits for the class loader to become unreachable. Before this change, the class loader stayed in memory. After this change, the JVM collects it. The harness is a local development tool, and it is not part of this commit. JAVA-6279
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PROBLEM
The driver starts a thread that removes idle buffers from PowerOfTwoBufferPool.DEFAULT. That thread never stops. A thread that runs forever keeps the class loader of all driver classes in memory. The static data of those classes also stays in memory. Then an application server cannot unload an application, and the memory of that application stays in use. Users report this behavior in GitHub issue 2029 and in JAVA-5643.
CAUSE
DEFAULT is a static field, and it calls enablePruning() during class initialization. That method schedules a periodic task. The executor starts the worker thread for the first task, but the pool is empty at that time. Therefore the thread has no work, and it continues to wake up forever.
The thread keeps the class loader in memory because the JVM captures the class loader when it constructs the thread. The data that the thread holds is not the cause. For this reason, a change to the references of the thread cannot release the class loader. The thread must stop.
SOLUTION
enablePruning() now sets a flag. It does not schedule a task. The release() method schedules one prune when it puts a buffer into the pool. Each prune schedules the next prune, but only if the pool still holds a buffer. The pruner does not schedule the next prune when the pool becomes empty.
The pruner also uses these settings on its executor:
The work queue becomes empty after the last prune. Then the keep-alive time expires, and the worker thread stops. A later call to release() schedules a new prune, and the executor starts a new thread.
Two threads must not schedule a prune at the same time. A prune must also not stop while a different thread adds a buffer to the pool. The AtomicBoolean pruningScheduled prevents both conditions. The prune clears the flag, and then it examines the pool one more time before it stops.
DRAWBACKS
The pool releases the class loader about 90 seconds after the last buffer release. The default value of maxIdleTime is one minute. A buffer is old enough to remove only after two prunes, and the keep-alive time adds 30 seconds. The class loader stays in memory during that period. A tool that examines threads at the moment of an undeployment can still find a live thread.
A pool that becomes idle and then busy starts a new thread. This adds a small cost. A test measures this cost. A busy pool keeps one thread, because new work arrives before the keep-alive time expires.
prune() keeps its current behavior after an error. It writes a log message and throws the error again, and the pruner does not start again. This behavior is the same as before this change.
PRIOR ART
Netty has the same problem and uses the same solution. GlobalEventExecutor is a single-thread singleton. It starts its thread when work arrives, and it stops the thread when the task queue stays empty for a quiet period. The deprecated ThreadDeathWatcher class uses the same pattern. The steps that this change uses to clear and then examine the flag follow GlobalEventExecutor.TaskRunner.
Netty also sets the context class loader of a new thread to null. See netty#7290 and JDK-7008595. That change corrects a different problem, which is a driver thread that keeps an application class loader in memory. This commit does not include that change.
TESTS
New tests in PowerOfTwoBufferPoolTest show three results. An empty pool starts no thread. The thread stops after the pruner empties the pool. The pruner starts again after the thread stops.
A separate test harness measures class loader retention. That harness loads the driver into a child class loader, opens a MongoClient, closes it, and then waits for the class loader to become unreachable. Before this change, the class loader stayed in memory. After this change, the JVM collects it. The harness is a local development tool, and it is not part of this commit.
JAVA-6279