Skip to content

Commit b6409de

Browse files
committed
GH-2715: Fix channel leak in CachingConnectionFactory
Fixes: #2715 When connection is closed from the broker, there are some channels leak into a cache after reconnection **Auto-cherry-pick to `3.0.x`**
1 parent 105dfac commit b6409de

File tree

1 file changed

+19
-36
lines changed

1 file changed

+19
-36
lines changed

spring-rabbit/src/main/java/org/springframework/amqp/rabbit/connection/CachingConnectionFactory.java

Lines changed: 19 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,6 @@ public enum ConfirmType {
194194

195195
private final Condition connectionAvailableCondition = this.connectionLock.newCondition();
196196

197-
198197
private final ActiveObjectCounter<Channel> inFlightAsyncCloses = new ActiveObjectCounter<>();
199198

200199
private final AtomicBoolean running = new AtomicBoolean();
@@ -1156,7 +1155,7 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
11561155
return null;
11571156
}
11581157
else {
1159-
physicalClose(channelProxy);
1158+
physicalClose();
11601159
return null;
11611160
}
11621161
}
@@ -1237,29 +1236,16 @@ else if (txEnds.contains(methodName)) {
12371236
}
12381237
}
12391238

1240-
private void releasePermitIfNecessary(ChannelProxy proxy) {
1239+
private void releasePermitIfNecessary() {
12411240
if (CachingConnectionFactory.this.channelCheckoutTimeout > 0) {
1242-
/*
1243-
* Only release a permit if this is a normal close; if the channel is
1244-
* in the list, it means we're closing a cached channel (for which a permit
1245-
* has already been released).
1246-
*/
1247-
1248-
this.theConnection.channelListLock.lock();
1249-
try {
1250-
if (this.channelList.contains(proxy)) {
1251-
return;
1252-
}
1253-
}
1254-
finally {
1255-
this.theConnection.channelListLock.unlock();
1256-
}
12571241
Semaphore permits = CachingConnectionFactory.this.checkoutPermits.get(this.theConnection);
12581242
if (permits != null) {
1259-
permits.release();
1260-
if (logger.isDebugEnabled()) {
1261-
logger.debug("Released permit for '" + this.theConnection + "', remaining: "
1262-
+ permits.availablePermits());
1243+
if (permits.availablePermits() < CachingConnectionFactory.this.channelCacheSize) {
1244+
permits.release();
1245+
if (logger.isDebugEnabled()) {
1246+
logger.debug("Released permit for '" + this.theConnection + "', remaining: "
1247+
+ permits.availablePermits());
1248+
}
12631249
}
12641250
}
12651251
else {
@@ -1285,11 +1271,8 @@ private void logicalClose(ChannelProxy proxy) throws IOException, TimeoutExcepti
12851271
if (this.target instanceof PublisherCallbackChannel) {
12861272
this.target.close(); // emit nacks if necessary
12871273
}
1288-
if (this.channelList.contains(proxy)) {
1289-
this.channelList.remove(proxy);
1290-
}
1291-
else {
1292-
releasePermitIfNecessary(proxy);
1274+
if (!this.channelList.remove(proxy)) {
1275+
releasePermitIfNecessary();
12931276
}
12941277
this.target = null;
12951278
return;
@@ -1328,7 +1311,7 @@ private void returnToCache(ChannelProxy proxy) {
13281311
// The channel didn't handle confirms, so close it altogether to avoid
13291312
// memory leaks for pending confirms
13301313
try {
1331-
physicalClose(this.theConnection.channelsAwaitingAcks.remove(this.target));
1314+
physicalClose();
13321315
}
13331316
catch (@SuppressWarnings(UNUSED) Exception e) {
13341317
}
@@ -1352,7 +1335,7 @@ private void doReturnToCache(@Nullable ChannelProxy proxy) {
13521335
else {
13531336
if (proxy.isOpen()) {
13541337
try {
1355-
physicalClose(proxy);
1338+
physicalClose();
13561339
}
13571340
catch (@SuppressWarnings(UNUSED) Exception e) {
13581341
}
@@ -1372,7 +1355,7 @@ private void cacheOrClose(ChannelProxy proxy) {
13721355
logger.trace("Cache limit reached: " + this.target);
13731356
}
13741357
try {
1375-
physicalClose(proxy);
1358+
physicalClose();
13761359
}
13771360
catch (@SuppressWarnings(UNUSED) Exception e) {
13781361
}
@@ -1381,8 +1364,8 @@ else if (!alreadyCached) {
13811364
if (logger.isTraceEnabled()) {
13821365
logger.trace("Returning cached Channel: " + this.target);
13831366
}
1384-
releasePermitIfNecessary(proxy);
13851367
this.channelList.addLast(proxy);
1368+
releasePermitIfNecessary();
13861369
setHighWaterMark();
13871370
}
13881371
}
@@ -1399,7 +1382,7 @@ private void setHighWaterMark() {
13991382
}
14001383
}
14011384

1402-
private void physicalClose(ChannelProxy proxy) throws IOException, TimeoutException {
1385+
private void physicalClose() throws IOException, TimeoutException {
14031386
if (logger.isDebugEnabled()) {
14041387
logger.debug("Closing cached Channel: " + this.target);
14051388
}
@@ -1413,7 +1396,7 @@ private void physicalClose(ChannelProxy proxy) throws IOException, TimeoutExcept
14131396
(ConfirmType.CORRELATED.equals(CachingConnectionFactory.this.confirmType) ||
14141397
CachingConnectionFactory.this.publisherReturns)) {
14151398
async = true;
1416-
asyncClose(proxy);
1399+
asyncClose();
14171400
}
14181401
else {
14191402
this.target.close();
@@ -1430,12 +1413,12 @@ private void physicalClose(ChannelProxy proxy) throws IOException, TimeoutExcept
14301413
finally {
14311414
this.target = null;
14321415
if (!async) {
1433-
releasePermitIfNecessary(proxy);
1416+
releasePermitIfNecessary();
14341417
}
14351418
}
14361419
}
14371420

1438-
private void asyncClose(ChannelProxy proxy) {
1421+
private void asyncClose() {
14391422
ExecutorService executorService = getChannelsExecutor();
14401423
final Channel channel = CachedChannelInvocationHandler.this.target;
14411424
CachingConnectionFactory.this.inFlightAsyncCloses.add(channel);
@@ -1467,7 +1450,7 @@ private void asyncClose(ChannelProxy proxy) {
14671450
}
14681451
finally {
14691452
CachingConnectionFactory.this.inFlightAsyncCloses.release(channel);
1470-
releasePermitIfNecessary(proxy);
1453+
releasePermitIfNecessary();
14711454
}
14721455
}
14731456
});

0 commit comments

Comments
 (0)