JAVA-6250: Do not retain the connection-open handler from the channel close listener.#2018
Merged
Conversation
…close listener. The listener registered on the channel closeFuture when a pooled connection opens captured the enclosing OpenChannelFutureListener instance. This kept the open handler, and the whole operation graph reachable from it (callbacks and their buffers), strongly referenced for as long as the pooled channel stayed alive. Capture the enclosing NettyStream in a local variable instead, so the operation graph becomes collectable as soon as the connection opening completes. JAVA-6250
The close listener now captures only the NettyStream, so the one-shot connection-open handler can no longer be pinned for the channel lifetime. Replaces the local-variable capture idiom with a structural guarantee. JAVA-6250
Explicit NettyStream dependency instead of the implicit enclosing instance, removing the this$0 capture footgun that caused JAVA-6250. No behavioral change. JAVA-6250
Member
Author
rozza
requested review from
Copilot and
nhachicha
and removed request for
vbabanin
July 20, 2026 10:16
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a memory-retention issue in driver-core’s Netty-based stream implementation where a channel.closeFuture() listener could keep the connection-open handler (and its transitive callback graph) strongly reachable for the lifetime of a pooled channel. The fix ensures the close listener only retains the NettyStream, allowing the open handler graph to be garbage-collected once open completes.
Changes:
- Refactors
NettyStreamchannel-open completion handling to avoid retaining the open handler via a close-future listener (introduces a dedicatedCloseChannelFutureListener). - Makes
OpenChannelFutureListenerastaticnested class and explicitly passesNettyStreamto prevent implicit outer captures. - Adds a unit test covering both the GC/reachability aspect and the “pending read fails on channel close” behavior.
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/netty/NettyStream.java | Avoids retaining the open handler by ensuring the channel close-future listener captures only NettyStream. |
| driver-core/src/test/unit/com/mongodb/internal/connection/netty/NettyStreamCloseFutureListenerTest.java | Adds regression tests for handler reachability after open and for failing pending reads on channel close. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Extract the GC reachability probe into a named assertRefUnreachable helper that documents the WeakReference-as-reachability-probe idiom, extract the accept plumbing into openAndAcceptConnection, and add explanatory comments tying the footprint assertion back to JAVA-6250. Test-only, no behaviour change. JAVA-6250
Comment on lines
+155
to
+162
| @SuppressWarnings("BusyWait") | ||
| private static void assertRefUnreachable(final WeakReference<?> ref, final String message) throws InterruptedException { | ||
| long deadlineNanos = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); | ||
| while (ref.get() != null && System.nanoTime() < deadlineNanos) { | ||
| System.gc(); | ||
| Thread.sleep(100); | ||
| } | ||
| assertNull(ref.get(), message); |
nhachicha
approved these changes
Jul 21, 2026
| @BeforeEach | ||
| public void setUp() throws Exception { | ||
| serverSocket = new ServerSocket(0, 1, InetAddress.getLoopbackAddress()); | ||
| eventLoopGroup = new NioEventLoopGroup(); |
Collaborator
There was a problem hiding this comment.
NioEventLoopGroup is deprecated, you can use new MultiThreadIoEventLoopGroup(NioIoHandler.newFactory())
| private static void assertRefUnreachable(final WeakReference<?> ref, final String message) throws InterruptedException { | ||
| long deadlineNanos = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); | ||
| while (ref.get() != null && System.nanoTime() < deadlineNanos) { | ||
| System.gc(); |
Collaborator
There was a problem hiding this comment.
I think this is only honoured in HotSpsot, it's not guarantee to trigger the GC...
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.
The listener registered on the channel closeFuture when a pooled
connection opens captured the enclosing OpenChannelFutureListener
instance. This kept the open handler, and the whole operation graph
reachable from it (callbacks and their buffers), strongly referenced
for as long as the pooled channel stayed alive. Capture the enclosing
NettyStream in a local variable instead, so the operation graph
becomes collectable as soon as the connection opening completes.
JAVA-6250