You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by ta...@apache.org on 2017/07/17 17:00:15 UTC
qpid-jms git commit: QPIDJMS-300 Fix leak of session executor thread
on close race
Repository: qpid-jms
Updated Branches:
refs/heads/master 87a4b1765 -> e447924a4
QPIDJMS-300 Fix leak of session executor thread on close race
On close of local and remote ends at the same time the session can leak
an instance of the completion executor as it tries to clean up any async
close completions before fully shutting down.
Project: http://git-wip-us.apache.org/repos/asf/qpid-jms/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-jms/commit/e447924a
Tree: http://git-wip-us.apache.org/repos/asf/qpid-jms/tree/e447924a
Diff: http://git-wip-us.apache.org/repos/asf/qpid-jms/diff/e447924a
Branch: refs/heads/master
Commit: e447924a4c5333ccfed9b26a2046dea2a702c869
Parents: 87a4b17
Author: Timothy Bish <ta...@gmail.com>
Authored: Mon Jul 17 13:00:02 2017 -0400
Committer: Timothy Bish <ta...@gmail.com>
Committed: Mon Jul 17 13:00:02 2017 -0400
----------------------------------------------------------------------
.../java/org/apache/qpid/jms/JmsConnection.java | 2 +
.../org/apache/qpid/jms/JmsMessageConsumer.java | 6 +-
.../org/apache/qpid/jms/JmsMessageProducer.java | 6 +-
.../java/org/apache/qpid/jms/JmsSession.java | 92 +++++++++++++-------
.../org/apache/qpid/jms/util/NoOpExecutor.java | 36 ++++++++
.../integration/ProducerIntegrationTest.java | 59 ++++++++++++-
.../transports/netty/NettyWsTransportTest.java | 1 -
7 files changed, 165 insertions(+), 37 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/e447924a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsConnection.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsConnection.java b/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsConnection.java
index 2bd1947..5fade17 100644
--- a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsConnection.java
+++ b/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsConnection.java
@@ -125,6 +125,8 @@ public class JmsConnection implements AutoCloseable, Connection, TopicConnection
executor = new ThreadPoolExecutor(1, 1, 5, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>(),
new QpidJMSThreadFactory("QpidJMS Connection Executor: " + connectionInfo.getId(), connectionInfo.isUseDaemonThread()));
+ executor.setRejectedExecutionHandler(new ThreadPoolExecutor.DiscardOldestPolicy());
+
// We need to start the core thread in order for it to prevent JVM shutdown as our
// single non-daemon thread.
executor.execute(new Runnable() {
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/e447924a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsMessageConsumer.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsMessageConsumer.java b/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsMessageConsumer.java
index 77349e8..58609b3 100644
--- a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsMessageConsumer.java
+++ b/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsMessageConsumer.java
@@ -31,6 +31,7 @@ import javax.jms.MessageFormatException;
import javax.jms.MessageListener;
import javax.jms.Session;
+import org.apache.qpid.jms.exceptions.JmsConnectionFailedException;
import org.apache.qpid.jms.exceptions.JmsExceptionSupport;
import org.apache.qpid.jms.message.JmsInboundMessageDispatch;
import org.apache.qpid.jms.message.JmsMessage;
@@ -151,7 +152,10 @@ public class JmsMessageConsumer implements AutoCloseable, MessageConsumer, JmsMe
*/
protected void doClose() throws JMSException {
shutdown();
- this.connection.destroyResource(consumerInfo);
+ try {
+ this.connection.destroyResource(consumerInfo);
+ } catch (JmsConnectionFailedException jmsex) {
+ }
}
/**
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/e447924a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsMessageProducer.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsMessageProducer.java b/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsMessageProducer.java
index df01058..1164d15 100644
--- a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsMessageProducer.java
+++ b/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsMessageProducer.java
@@ -29,6 +29,7 @@ import javax.jms.JMSException;
import javax.jms.Message;
import javax.jms.MessageProducer;
+import org.apache.qpid.jms.exceptions.JmsConnectionFailedException;
import org.apache.qpid.jms.message.JmsMessageIDBuilder;
import org.apache.qpid.jms.meta.JmsProducerId;
import org.apache.qpid.jms.meta.JmsProducerInfo;
@@ -92,7 +93,10 @@ public class JmsMessageProducer implements AutoCloseable, MessageProducer {
protected void doClose() throws JMSException {
session.checkIsCompletionThread();
shutdown();
- connection.destroyResource(producerInfo);
+ try {
+ connection.destroyResource(producerInfo);
+ } catch (JmsConnectionFailedException jmsEx) {
+ }
}
/**
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/e447924a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java b/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java
index 2f13bf5..a69eae3 100644
--- a/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java
+++ b/qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsSession.java
@@ -27,7 +27,8 @@ import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedDeque;
import java.util.concurrent.Executor;
import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicLong;
@@ -64,6 +65,7 @@ import javax.jms.TopicPublisher;
import javax.jms.TopicSession;
import javax.jms.TopicSubscriber;
+import org.apache.qpid.jms.exceptions.JmsConnectionFailedException;
import org.apache.qpid.jms.exceptions.JmsExceptionSupport;
import org.apache.qpid.jms.message.JmsInboundMessageDispatch;
import org.apache.qpid.jms.message.JmsMessage;
@@ -86,6 +88,7 @@ import org.apache.qpid.jms.provider.ProviderConstants.ACK_TYPE;
import org.apache.qpid.jms.provider.ProviderFuture;
import org.apache.qpid.jms.selector.SelectorParser;
import org.apache.qpid.jms.selector.filter.FilterException;
+import org.apache.qpid.jms.util.NoOpExecutor;
import org.apache.qpid.jms.util.QpidJMSThreadFactory;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -260,7 +263,10 @@ public class JmsSession implements AutoCloseable, Session, QueueSession, TopicSe
protected void doClose() throws JMSException {
boolean interrupted = Thread.interrupted();
shutdown();
- connection.destroyResource(sessionInfo);
+ try {
+ connection.destroyResource(sessionInfo);
+ } catch (JmsConnectionFailedException jmsex) {
+ }
connection.removeSession(sessionInfo);
if (interrupted) {
Thread.currentThread().interrupt();
@@ -299,19 +305,17 @@ public class JmsSession implements AutoCloseable, Session, QueueSession, TopicSe
// Ensure that no asynchronous completion sends remain blocked after close.
synchronized (sessionInfo) {
- if (completionExcecutor != null) {
- if (cause == null) {
- cause = new JMSException("Session closed remotely before message transfer result was notified");
- }
+ ensureCompletionExecutorExists();
+ if (cause == null) {
+ cause = new JMSException("Session closed remotely before message transfer result was notified");
+ }
- completionExcecutor.execute(new FailOrCompleteAsyncCompletionsTask(JmsExceptionSupport.create(cause)));
- completionExcecutor.shutdown();
- try {
- completionExcecutor.awaitTermination(connection.getCloseTimeout(), TimeUnit.MILLISECONDS);
- } catch (InterruptedException e) {
- LOG.trace("Session close awaiting send completions was interrupted");
- }
- completionExcecutor = null;
+ completionExcecutor.execute(new FailOrCompleteAsyncCompletionsTask(JmsExceptionSupport.create(cause)));
+ completionExcecutor.shutdown();
+ try {
+ completionExcecutor.awaitTermination(connection.getCloseTimeout(), TimeUnit.MILLISECONDS);
+ } catch (InterruptedException e) {
+ LOG.trace("Session close awaiting send completions was interrupted");
}
}
}
@@ -1027,17 +1031,23 @@ public class JmsSession implements AutoCloseable, Session, QueueSession, TopicSe
if (exec == null) {
synchronized (sessionInfo) {
if (deliveryExecutor == null) {
- deliveryExecutor = createExecutor("delivery dispatcher");
- }
+ if (!closed.get()) {
+ exec = createExecutor("delivery dispatcher");
+ exec.execute(new Runnable() {
- exec = deliveryExecutor;
- exec.execute(new Runnable() {
+ @Override
+ public void run() {
+ JmsSession.this.deliveryThread = Thread.currentThread();
+ }
+ });
- @Override
- public void run() {
- JmsSession.this.deliveryThread = Thread.currentThread();
+ deliveryExecutor = exec;
+ } else {
+ return NoOpExecutor.INSTANCE;
}
- });
+ } else {
+ exec = deliveryExecutor;
+ }
}
}
@@ -1045,21 +1055,35 @@ public class JmsSession implements AutoCloseable, Session, QueueSession, TopicSe
}
Executor getCompletionExecutor() {
+ return getCompletionExecutor(false);
+ }
+
+ private void ensureCompletionExecutorExists() {
+ getCompletionExecutor(true);
+ }
+
+ private Executor getCompletionExecutor(boolean ignoreClosed) {
ExecutorService exec = completionExcecutor;
if (exec == null) {
synchronized (sessionInfo) {
if (completionExcecutor == null) {
- completionExcecutor = createExecutor("completion dispatcher");
- }
+ if (!closed.get() || ignoreClosed) {
+ exec = createExecutor("completion dispatcher");
+ exec.execute(new Runnable() {
- exec = completionExcecutor;
- exec.execute(new Runnable() {
+ @Override
+ public void run() {
+ JmsSession.this.completionThread = Thread.currentThread();
+ }
+ });
- @Override
- public void run() {
- JmsSession.this.completionThread = Thread.currentThread();
+ completionExcecutor = exec;
+ } else {
+ return NoOpExecutor.INSTANCE;
}
- });
+ } else {
+ exec = completionExcecutor;
+ }
}
}
@@ -1067,7 +1091,12 @@ public class JmsSession implements AutoCloseable, Session, QueueSession, TopicSe
}
private ExecutorService createExecutor(final String threadNameSuffix) {
- return Executors.newSingleThreadExecutor(new QpidJMSThreadFactory("JmsSession ["+ sessionInfo.getId() + "] " + threadNameSuffix, true));
+ ThreadPoolExecutor executor = new ThreadPoolExecutor(1, 1, 5, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>(),
+ new QpidJMSThreadFactory("JmsSession ["+ sessionInfo.getId() + "] " + threadNameSuffix, true));
+
+ executor.setRejectedExecutionHandler(new ThreadPoolExecutor.DiscardOldestPolicy());
+
+ return executor;
}
protected JmsSessionInfo getSessionInfo() {
@@ -1201,7 +1230,6 @@ public class JmsSession implements AutoCloseable, Session, QueueSession, TopicSe
}
protected void onConnectionInterrupted() {
-
transactionContext.onConnectionInterrupted();
// TODO - Synthesize a better exception
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/e447924a/qpid-jms-client/src/main/java/org/apache/qpid/jms/util/NoOpExecutor.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/main/java/org/apache/qpid/jms/util/NoOpExecutor.java b/qpid-jms-client/src/main/java/org/apache/qpid/jms/util/NoOpExecutor.java
new file mode 100644
index 0000000..a179309
--- /dev/null
+++ b/qpid-jms-client/src/main/java/org/apache/qpid/jms/util/NoOpExecutor.java
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.qpid.jms.util;
+
+import java.util.concurrent.Executor;
+
+/**
+ * Simple executor implementation that ignores any requests to
+ * execute a task. This can be used in the case where an executor
+ * should be returned or provided but the state of the application
+ * or object is such that it will not process any new work and the
+ * result of ignoring the request does not impact the application.
+ */
+public class NoOpExecutor implements Executor {
+
+ public static final Executor INSTANCE = new NoOpExecutor();
+
+ @Override
+ public void execute(Runnable command) {
+
+ }
+}
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/e447924a/qpid-jms-client/src/test/java/org/apache/qpid/jms/integration/ProducerIntegrationTest.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/test/java/org/apache/qpid/jms/integration/ProducerIntegrationTest.java b/qpid-jms-client/src/test/java/org/apache/qpid/jms/integration/ProducerIntegrationTest.java
index 2175fff..24f5b8a 100644
--- a/qpid-jms-client/src/test/java/org/apache/qpid/jms/integration/ProducerIntegrationTest.java
+++ b/qpid-jms-client/src/test/java/org/apache/qpid/jms/integration/ProducerIntegrationTest.java
@@ -68,7 +68,6 @@ import org.apache.qpid.jms.JmsDefaultConnectionListener;
import org.apache.qpid.jms.JmsOperationTimedOutException;
import org.apache.qpid.jms.JmsSendTimedOutException;
import org.apache.qpid.jms.message.foreign.ForeignJmsMessage;
-import org.apache.qpid.jms.provider.amqp.AmqpSupport;
import org.apache.qpid.jms.provider.amqp.message.AmqpMessageIdHelper;
import org.apache.qpid.jms.provider.amqp.message.AmqpMessageSupport;
import org.apache.qpid.jms.test.QpidJmsTestCase;
@@ -89,6 +88,8 @@ import org.apache.qpid.jms.test.testpeer.matchers.sections.MessageHeaderSectionM
import org.apache.qpid.jms.test.testpeer.matchers.sections.MessagePropertiesSectionMatcher;
import org.apache.qpid.jms.test.testpeer.matchers.sections.TransferPayloadCompositeMatcher;
import org.apache.qpid.jms.test.testpeer.matchers.types.EncodedAmqpValueMatcher;
+import org.apache.qpid.jms.util.QpidJMSTestRunner;
+import org.apache.qpid.jms.util.Repeat;
import org.apache.qpid.proton.amqp.Binary;
import org.apache.qpid.proton.amqp.Symbol;
import org.apache.qpid.proton.amqp.UnsignedByte;
@@ -96,11 +97,12 @@ import org.apache.qpid.proton.amqp.UnsignedInteger;
import org.hamcrest.Matcher;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
-import org.hamcrest.core.CombinableMatcher;
import org.junit.Test;
+import org.junit.runner.RunWith;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+@RunWith(QpidJMSTestRunner.class)
public class ProducerIntegrationTest extends QpidJmsTestCase {
private static final Logger LOG = LoggerFactory.getLogger(ProducerIntegrationTest.class);
@@ -1123,6 +1125,7 @@ public class ProducerIntegrationTest extends QpidJmsTestCase {
}
}
+ @Repeat(repetitions = 1)
@Test(timeout = 20000)
public void testRemotelyCloseConnectionDuringSyncSend() throws Exception {
final String BREAD_CRUMB = "ErrorMessageBreadCrumb";
@@ -1166,6 +1169,58 @@ public class ProducerIntegrationTest extends QpidJmsTestCase {
}
testPeer.waitForAllHandlersToComplete(3000);
+
+ connection.close();
+ }
+ }
+
+ @Repeat(repetitions = 1)
+ @Test(timeout = 20000)
+ public void testRemotelyCloseConnectionAndDropDuringSyncSend() throws Exception {
+ final String BREAD_CRUMB = "ErrorMessageBreadCrumb";
+
+ try (TestAmqpPeer testPeer = new TestAmqpPeer();) {
+ Connection connection = testFixture.establishConnecton(testPeer);
+
+ testPeer.expectBegin();
+ Session session = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
+
+ // Expect producer creation, give it credit.
+ testPeer.expectSenderAttach();
+
+ String text = "myMessage";
+ MessageHeaderSectionMatcher headersMatcher = new MessageHeaderSectionMatcher(true).withDurable(equalTo(true));
+ MessageAnnotationsSectionMatcher msgAnnotationsMatcher = new MessageAnnotationsSectionMatcher(true);
+ MessagePropertiesSectionMatcher propsMatcher = new MessagePropertiesSectionMatcher(true);
+ TransferPayloadCompositeMatcher messageMatcher = new TransferPayloadCompositeMatcher();
+ messageMatcher.setHeadersMatcher(headersMatcher);
+ messageMatcher.setMessageAnnotationsMatcher(msgAnnotationsMatcher);
+ messageMatcher.setPropertiesMatcher(propsMatcher);
+ messageMatcher.setMessageContentMatcher(new EncodedAmqpValueMatcher(text));
+
+ // Expect a message to be sent, but don't send a disposition in
+ // response, simply remotely close the connection instead.
+ testPeer.expectTransfer(messageMatcher, nullValue(), false, false, null, false);
+ testPeer.remotelyCloseConnection(true, AmqpError.RESOURCE_LIMIT_EXCEEDED, BREAD_CRUMB);
+ testPeer.dropAfterLastHandler();
+
+ Queue queue = session.createQueue("myQueue");
+ final MessageProducer producer = session.createProducer(queue);
+
+ Message message = session.createTextMessage(text);
+
+ try {
+ producer.send(message);
+ fail("Expected exception to be thrown");
+ } catch (JMSException jmse) {
+ // Expected
+ assertNotNull("Expected exception to have a message", jmse.getMessage());
+ assertTrue("Expected breadcrumb to be present in message", jmse.getMessage().contains(BREAD_CRUMB));
+ }
+
+ testPeer.waitForAllHandlersToComplete(3000);
+
+ connection.close();
}
}
http://git-wip-us.apache.org/repos/asf/qpid-jms/blob/e447924a/qpid-jms-client/src/test/java/org/apache/qpid/jms/transports/netty/NettyWsTransportTest.java
----------------------------------------------------------------------
diff --git a/qpid-jms-client/src/test/java/org/apache/qpid/jms/transports/netty/NettyWsTransportTest.java b/qpid-jms-client/src/test/java/org/apache/qpid/jms/transports/netty/NettyWsTransportTest.java
index bf04eb4..f7b6b69 100644
--- a/qpid-jms-client/src/test/java/org/apache/qpid/jms/transports/netty/NettyWsTransportTest.java
+++ b/qpid-jms-client/src/test/java/org/apache/qpid/jms/transports/netty/NettyWsTransportTest.java
@@ -30,7 +30,6 @@ import org.apache.qpid.jms.test.Wait;
import org.apache.qpid.jms.transports.Transport;
import org.apache.qpid.jms.transports.TransportListener;
import org.apache.qpid.jms.transports.TransportOptions;
-import org.apache.qpid.jms.transports.netty.NettyTcpTransportTest.NettyTransportListener;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org