Skip to content

Commit 2745450

Browse files
m-k8srozza
authored andcommitted
JAVA-6250 Do not retain the connection-open handler from the channel 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
1 parent 0a57ca6 commit 2745450

2 files changed

Lines changed: 159 additions & 1 deletion

File tree

driver-core/src/main/com/mongodb/internal/connection/netty/NettyStream.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -530,7 +530,11 @@ public void operationComplete(final ChannelFuture future) {
530530
channelFuture.channel().close();
531531
} else {
532532
channel = channelFuture.channel();
533-
channel.closeFuture().addListener((ChannelFutureListener) future1 -> handleReadResponse(null, new IOException("The connection to the server was closed")));
533+
// capture only the enclosing stream in the close listener: capturing OpenChannelFutureListener.this
534+
// would pin the open handler (and everything it references) for the lifetime of the pooled channel
535+
NettyStream stream = NettyStream.this;
536+
channel.closeFuture().addListener((ChannelFutureListener) future1 ->
537+
stream.handleReadResponse(null, new IOException("The connection to the server was closed")));
534538
}
535539
handler.completed(null);
536540
} else {
Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
/*
2+
* Copyright 2008-present MongoDB, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.mongodb.internal.connection.netty;
18+
19+
import com.mongodb.ServerAddress;
20+
import com.mongodb.connection.AsyncCompletionHandler;
21+
import com.mongodb.connection.SocketSettings;
22+
import com.mongodb.connection.SslSettings;
23+
import io.netty.buffer.PooledByteBufAllocator;
24+
import io.netty.channel.nio.NioEventLoopGroup;
25+
import io.netty.channel.socket.nio.NioSocketChannel;
26+
import org.bson.ByteBuf;
27+
import org.junit.jupiter.api.AfterEach;
28+
import org.junit.jupiter.api.BeforeEach;
29+
import org.junit.jupiter.api.DisplayName;
30+
import org.junit.jupiter.api.Test;
31+
32+
import java.io.IOException;
33+
import java.lang.ref.WeakReference;
34+
import java.net.InetAddress;
35+
import java.net.ServerSocket;
36+
import java.net.Socket;
37+
import java.util.Collections;
38+
import java.util.concurrent.CountDownLatch;
39+
import java.util.concurrent.TimeUnit;
40+
import java.util.concurrent.atomic.AtomicReference;
41+
42+
import static com.mongodb.ClusterFixture.OPERATION_CONTEXT;
43+
import static org.junit.jupiter.api.Assertions.assertEquals;
44+
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
45+
import static org.junit.jupiter.api.Assertions.assertNotNull;
46+
import static org.junit.jupiter.api.Assertions.assertNull;
47+
import static org.junit.jupiter.api.Assertions.assertTrue;
48+
49+
/**
50+
* Covers the listener that {@code OpenChannelFutureListener} registers on {@code channel.closeFuture()},
51+
* from both angles:
52+
* <ul>
53+
* <li>behavior: a read waiting on the channel must be failed when the channel closes;</li>
54+
* <li>footprint: the listener must not keep the connection-open {@link AsyncCompletionHandler}
55+
* (and everything it transitively references, e.g. the callback chain of the operation that was
56+
* waiting for the connection) strongly reachable while the channel remains open.</li>
57+
* </ul>
58+
*/
59+
public class NettyStreamCloseFutureListenerTest {
60+
61+
private ServerSocket serverSocket;
62+
private NioEventLoopGroup eventLoopGroup;
63+
private NettyStream stream;
64+
65+
@BeforeEach
66+
public void setUp() throws Exception {
67+
serverSocket = new ServerSocket(0, 1, InetAddress.getLoopbackAddress());
68+
eventLoopGroup = new NioEventLoopGroup();
69+
stream = (NettyStream) new NettyStreamFactory(
70+
host -> Collections.singletonList(InetAddress.getLoopbackAddress()),
71+
SocketSettings.builder().connectTimeout(10, TimeUnit.SECONDS).build(),
72+
SslSettings.builder().build(),
73+
eventLoopGroup, NioSocketChannel.class, PooledByteBufAllocator.DEFAULT, null)
74+
.create(new ServerAddress("127.0.0.1", serverSocket.getLocalPort()));
75+
}
76+
77+
@AfterEach
78+
public void tearDown() throws Exception {
79+
stream.close();
80+
eventLoopGroup.shutdownGracefully();
81+
serverSocket.close();
82+
}
83+
84+
@Test
85+
@DisplayName("open handler should not remain strongly reachable from the open channel after the open completes")
86+
public void shouldReleaseOpenHandlerAfterOpenCompletesWhileChannelRemainsOpen() throws Exception {
87+
CountDownLatch opened = new CountDownLatch(1);
88+
AsyncCompletionHandler<Void> handler = new AsyncCompletionHandler<Void>() {
89+
@Override
90+
public void completed(final Void result) {
91+
opened.countDown();
92+
}
93+
94+
@Override
95+
public void failed(final Throwable t) {
96+
opened.countDown();
97+
}
98+
};
99+
WeakReference<AsyncCompletionHandler<Void>> canary = new WeakReference<AsyncCompletionHandler<Void>>(handler);
100+
101+
stream.openAsync(OPERATION_CONTEXT, handler);
102+
assertTrue(opened.await(10, TimeUnit.SECONDS), "open did not complete");
103+
104+
handler = null;
105+
for (int i = 0; i < 10 && canary.get() != null; i++) {
106+
System.gc();
107+
Thread.sleep(100);
108+
}
109+
assertNull(canary.get(),
110+
"the connection-open AsyncCompletionHandler must not stay strongly reachable from the open "
111+
+ "channel (closeFuture listener) after the open has completed");
112+
}
113+
114+
@Test
115+
@DisplayName("pending read should be failed when the channel is closed")
116+
public void shouldFailPendingReadWhenChannelIsClosed() throws Exception {
117+
AtomicReference<Socket> acceptedSocket = new AtomicReference<>();
118+
Thread acceptor = new Thread(() -> {
119+
try {
120+
acceptedSocket.set(serverSocket.accept());
121+
} catch (IOException ignored) {
122+
// the assertions below fail if nothing was accepted
123+
}
124+
});
125+
acceptor.start();
126+
127+
stream.open(OPERATION_CONTEXT);
128+
acceptor.join(TimeUnit.SECONDS.toMillis(10));
129+
assertNotNull(acceptedSocket.get(), "the server never accepted the connection");
130+
131+
CountDownLatch readCompleted = new CountDownLatch(1);
132+
AtomicReference<Throwable> readFailure = new AtomicReference<>();
133+
stream.readAsync(4, OPERATION_CONTEXT, new AsyncCompletionHandler<ByteBuf>() {
134+
@Override
135+
public void completed(final ByteBuf result) {
136+
readCompleted.countDown();
137+
}
138+
139+
@Override
140+
public void failed(final Throwable t) {
141+
readFailure.set(t);
142+
readCompleted.countDown();
143+
}
144+
});
145+
146+
acceptedSocket.get().close();
147+
148+
assertTrue(readCompleted.await(10, TimeUnit.SECONDS), "the pending read never completed");
149+
Throwable failure = readFailure.get();
150+
assertNotNull(failure, "the pending read completed successfully instead of failing");
151+
assertInstanceOf(IOException.class, failure);
152+
assertEquals("The connection to the server was closed", failure.getMessage());
153+
}
154+
}

0 commit comments

Comments
 (0)