You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by ad...@apache.org on 2017/07/05 13:59:38 UTC
[31/34] james-project git commit: JAMES-2085 Stop swallowing
exceptions in JMS related code
JAMES-2085 Stop swallowing exceptions in JMS related code
Project: http://git-wip-us.apache.org/repos/asf/james-project/repo
Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/c5758e6c
Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/c5758e6c
Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/c5758e6c
Branch: refs/heads/master
Commit: c5758e6c5972c72fb7bb623f657d30dd4828c8f0
Parents: e755858
Author: benwa <bt...@linagora.com>
Authored: Tue Jul 4 16:32:17 2017 +0700
Committer: benwa <bt...@linagora.com>
Committed: Wed Jul 5 17:14:04 2017 +0700
----------------------------------------------------------------------
.../james/queue/activemq/ActiveMQMailQueue.java | 46 +---
.../activemq/ActiveMQMailQueueBlobTest.java | 12 -
.../apache/james/queue/jms/JMSMailQueue.java | 227 ++++++-------------
.../james/queue/jms/JMSMailQueueItem.java | 22 +-
.../jms/MimeMessageObjectMessageSource.java | 15 +-
.../queue/jms/AbstractJMSMailQueueTest.java | 17 +-
.../james/queue/jms/JMSMailQueueTest.java | 10 -
7 files changed, 97 insertions(+), 252 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/james-project/blob/c5758e6c/server/queue/queue-activemq/src/main/java/org/apache/james/queue/activemq/ActiveMQMailQueue.java
----------------------------------------------------------------------
diff --git a/server/queue/queue-activemq/src/main/java/org/apache/james/queue/activemq/ActiveMQMailQueue.java b/server/queue/queue-activemq/src/main/java/org/apache/james/queue/activemq/ActiveMQMailQueue.java
index e39fb81..57c69c9 100644
--- a/server/queue/queue-activemq/src/main/java/org/apache/james/queue/activemq/ActiveMQMailQueue.java
+++ b/server/queue/queue-activemq/src/main/java/org/apache/james/queue/activemq/ActiveMQMailQueue.java
@@ -207,13 +207,7 @@ public class ActiveMQMailQueue extends JMSMailQueue implements ActiveMQSupport {
}
throw e;
} finally {
-
- try {
- if (producer != null)
- producer.close();
- } catch (JMSException e) {
- // ignore here
- }
+ closeProducer(producer);
}
}
@@ -304,35 +298,16 @@ public class ActiveMQMailQueue extends JMSMailQueue implements ActiveMQSupport {
size = reply.getLong("size");
return size;
} catch (NumberFormatException e) {
- // if we hit this we can't calculate the size so just catch
- // it
+ return super.getSize();
}
}
-
+ return super.getSize();
} catch (Exception e) {
throw new MailQueueException("Unable to remove mails", e);
} finally {
-
- if (consumer != null) {
-
- try {
- consumer.close();
- } catch (JMSException e1) {
- e1.printStackTrace();
- // ignore on rollback
- }
- }
-
- if (producer != null) {
-
- try {
- producer.close();
- } catch (JMSException e1) {
- // ignore on rollback
- }
- }
-
+ closeConsumer(consumer);
+ closeProducer(producer);
if (replyTo != null) {
try {
@@ -342,18 +317,11 @@ public class ActiveMQMailQueue extends JMSMailQueue implements ActiveMQSupport {
// every TemporaryQueue which will never get unregistered
replyTo.delete();
} catch (JMSException e) {
+ logger.error("Error while deleting temporary queue", e);
}
}
- try {
- if (session != null)
- session.close();
- } catch (JMSException e1) {
- // ignore here
- }
+ closeSession(session);
}
-
- // if we came to this point we should just fallback to super method
- return super.getSize();
}
}
http://git-wip-us.apache.org/repos/asf/james-project/blob/c5758e6c/server/queue/queue-activemq/src/test/java/org/apache/james/queue/activemq/ActiveMQMailQueueBlobTest.java
----------------------------------------------------------------------
diff --git a/server/queue/queue-activemq/src/test/java/org/apache/james/queue/activemq/ActiveMQMailQueueBlobTest.java b/server/queue/queue-activemq/src/test/java/org/apache/james/queue/activemq/ActiveMQMailQueueBlobTest.java
index 92d97c2..b360913 100644
--- a/server/queue/queue-activemq/src/test/java/org/apache/james/queue/activemq/ActiveMQMailQueueBlobTest.java
+++ b/server/queue/queue-activemq/src/test/java/org/apache/james/queue/activemq/ActiveMQMailQueueBlobTest.java
@@ -35,8 +35,6 @@ public class ActiveMQMailQueueBlobTest extends ActiveMQMailQueueTest {
public final static String BASE_DIR = "file://target/james-test";
private MyFileSystem fs;
- private JMSMailQueue queue;
-
@Override
protected ActiveMQConnectionFactory createConnectionFactory() {
ActiveMQConnectionFactory factory = super.createConnectionFactory();
@@ -62,16 +60,6 @@ public class ActiveMQMailQueueBlobTest extends ActiveMQMailQueueTest {
}
@Override
- public JMSMailQueue getQueue() {
- return queue;
- }
-
- @Override
- public void setQueue(JMSMailQueue queue) {
- this.queue = queue;
- }
-
- @Override
protected boolean useBlobMessages() {
return true;
}
http://git-wip-us.apache.org/repos/asf/james-project/blob/c5758e6c/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSMailQueue.java
----------------------------------------------------------------------
diff --git a/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSMailQueue.java b/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSMailQueue.java
index db65d45..39a6dd5 100644
--- a/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSMailQueue.java
+++ b/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSMailQueue.java
@@ -60,6 +60,7 @@ import org.apache.james.queue.api.ManageableMailQueue;
import org.apache.mailet.Mail;
import org.apache.mailet.MailAddress;
import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import com.google.common.base.Throwables;
@@ -76,6 +77,58 @@ import com.google.common.base.Throwables;
*/
public class JMSMailQueue implements ManageableMailQueue, JMSSupport, MailPrioritySupport, Disposable {
+ protected static void closeSession(Session session) {
+ if (session != null) {
+ try {
+ session.close();
+ } catch (JMSException e) {
+ LOGGER.error("Error while closing session", e);
+ }
+ }
+ }
+
+ protected static void closeProducer(MessageProducer producer) {
+ if (producer != null) {
+ try {
+ producer.close();
+ } catch (JMSException e) {
+ LOGGER.error("Error while closing producer", e);
+ }
+ }
+ }
+
+ protected static void closeConsumer(MessageConsumer consumer) {
+ if (consumer != null) {
+ try {
+ consumer.close();
+ } catch (JMSException e) {
+ LOGGER.error("Error while closing consumer", e);
+ }
+ }
+ }
+
+ protected static void rollback(Session session) {
+ if (session != null) {
+ try {
+ session.rollback();
+ } catch (JMSException e) {
+ LOGGER.error("Error while rolling session back", e);
+ }
+ }
+ }
+
+ private static void closeBrowser(QueueBrowser browser) {
+ if (browser != null) {
+ try {
+ browser.close();
+ } catch (JMSException e) {
+ LOGGER.error("Error while closing browser", e);
+ }
+ }
+ }
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(JMSMailQueue.class);
+
protected final String queueName;
protected final Connection connection;
protected final MailQueueItemDecoratorFactory mailQueueItemDecoratorFactory;
@@ -132,47 +185,14 @@ public class JMSMailQueue implements ManageableMailQueue, JMSSupport, MailPriori
return createMailQueueItem(connection, session, consumer, message);
} else {
session.commit();
-
- if (consumer != null) {
-
- try {
- consumer.close();
- } catch (JMSException e1) {
- // ignore on rollback
- }
- }
- try {
- if (session != null)
- session.close();
- } catch (JMSException e1) {
- // ignore here
- }
+ closeConsumer(consumer);
+ closeSession(session);
}
} catch (Exception e) {
- if (session != null) {
- try {
- session.rollback();
- } catch (JMSException e1) {
- // ignore on rollback
- }
- }
-
- if (consumer != null) {
-
- try {
- consumer.close();
- } catch (JMSException e1) {
- // ignore on rollback
- }
- }
- try {
- if (session != null)
- session.close();
- } catch (JMSException e1) {
- // ignore here
- }
-
+ rollback(session);
+ closeConsumer(consumer);
+ closeSession(session);
throw new MailQueueException("Unable to dequeue next message", e);
} finally {
timeMetric.stopAndPublish();
@@ -209,23 +229,11 @@ public class JMSMailQueue implements ManageableMailQueue, JMSSupport, MailPriori
enqueuedMailsMetric.increment();
mailQueueSize.increment();
} catch (Exception e) {
- if (session != null) {
- try {
- session.rollback();
- } catch (JMSException e1) {
- // ignore on rollback
- }
- }
+ rollback(session);
throw new MailQueueException("Unable to enqueue mail " + mail, e);
-
} finally {
timeMetric.stopAndPublish();
- try {
- if (session != null)
- session.close();
- } catch (JMSException e) {
- // ignore here
- }
+ closeSession(session);
}
}
@@ -267,15 +275,8 @@ public class JMSMailQueue implements ManageableMailQueue, JMSSupport, MailPriori
producer.send(message, Message.DEFAULT_DELIVERY_MODE, msgPrio, Message.DEFAULT_TIME_TO_LIVE);
} finally {
-
- try {
- if (producer != null)
- producer.close();
- } catch (JMSException e) {
- // ignore here
- }
+ closeProducer(producer);
}
-
}
/**
@@ -505,20 +506,8 @@ public class JMSMailQueue implements ManageableMailQueue, JMSSupport, MailPriori
logger.error("Unable to get size of queue " + queueName, e);
throw new MailQueueException("Unable to get size of queue " + queueName, e);
} finally {
- try {
- if (browser != null)
- browser.close();
- } catch (JMSException e1) {
- // ignore here
- }
-
- try {
- if (session != null)
- session.close();
- } catch (JMSException e1) {
- // ignore here
- }
-
+ closeBrowser(browser);
+ closeSession(session);
}
}
@@ -557,37 +546,12 @@ public class JMSMailQueue implements ManageableMailQueue, JMSSupport, MailPriori
return count;
} catch (Exception e) {
logger.error("Unable to flush mail", e);
- try {
- session.rollback();
- } catch (JMSException e1) {
- // ignore on rollback
- }
+ rollback(session);
throw new MailQueueException("Unable to get size of queue " + queueName, e);
} finally {
- if (consumer != null) {
-
- try {
- consumer.close();
- } catch (JMSException e1) {
- // ignore on rollback
- }
- }
- if (producer != null) {
-
- try {
- producer.close();
- } catch (JMSException e1) {
- // ignore on rollback
- }
- }
-
- try {
- if (session != null)
- session.close();
- } catch (JMSException e1) {
- // ignore here
- }
-
+ closeConsumer(consumer);
+ closeProducer(producer);
+ closeSession(session);
}
}
@@ -636,30 +600,12 @@ public class JMSMailQueue implements ManageableMailQueue, JMSSupport, MailPriori
session.commit();
return messages;
} catch (Exception e) {
- try {
- session.rollback();
- } catch (JMSException e1) {
- // ignore on rollback
- }
+ rollback(session);
throw new MailQueueException("Unable to remove mails", e);
} finally {
- if (consumer != null) {
-
- try {
- consumer.close();
- } catch (JMSException e1) {
- // ignore on rollback
- }
- }
-
- try {
- if (session != null)
- session.close();
- } catch (JMSException e1) {
- // ignore here
- }
-
+ closeConsumer(consumer);
+ closeSession(session);
}
}
@@ -760,39 +706,15 @@ public class JMSMailQueue implements ManageableMailQueue, JMSSupport, MailPriori
@Override
public void close() {
-
- try {
- if (myBrowser != null)
- myBrowser.close();
- } catch (JMSException e1) {
- // ignore here
- }
-
- try {
- if (mySession != null)
- mySession.close();
- } catch (JMSException e1) {
- // ignore here
- }
-
+ closeBrowser(myBrowser);
+ closeSession(mySession);
}
};
} catch (Exception e) {
- try {
- if (browser != null)
- browser.close();
- } catch (JMSException e1) {
- // ignore here
- }
-
- try {
- if (session != null)
- session.close();
- } catch (JMSException e1) {
- // ignore here
- }
+ closeBrowser(browser);
+ closeSession(session);
logger.error("Unable to browse queue " + queueName, e);
throw new MailQueueException("Unable to browse queue " + queueName, e);
@@ -804,6 +726,7 @@ public class JMSMailQueue implements ManageableMailQueue, JMSSupport, MailPriori
try {
connection.close();
} catch (JMSException e) {
+ logger.error("Error while closing session", e);
}
}
http://git-wip-us.apache.org/repos/asf/james-project/blob/c5758e6c/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSMailQueueItem.java
----------------------------------------------------------------------
diff --git a/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSMailQueueItem.java b/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSMailQueueItem.java
index dd13591..5f195bd 100644
--- a/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSMailQueueItem.java
+++ b/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/JMSMailQueueItem.java
@@ -51,29 +51,13 @@ public class JMSMailQueueItem implements MailQueueItem {
if (success) {
session.commit();
} else {
- try {
- session.rollback();
- } catch (JMSException e1) {
- // ignore on rollback
- }
+ JMSMailQueue.rollback(session);
}
} catch (JMSException ex) {
throw new MailQueueException("Unable to commit dequeue operation for mail " + mail.getName(), ex);
} finally {
- if (consumer != null) {
-
- try {
- consumer.close();
- } catch (JMSException e1) {
- // ignore on rollback
- }
- }
- try {
- if (session != null)
- session.close();
- } catch (JMSException e) {
- // ignore here
- }
+ JMSMailQueue.closeConsumer(consumer);
+ JMSMailQueue.closeSession(session);
}
}
http://git-wip-us.apache.org/repos/asf/james-project/blob/c5758e6c/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/MimeMessageObjectMessageSource.java
----------------------------------------------------------------------
diff --git a/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/MimeMessageObjectMessageSource.java b/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/MimeMessageObjectMessageSource.java
index 3ddd765..0b223dd 100644
--- a/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/MimeMessageObjectMessageSource.java
+++ b/server/queue/queue-jms/src/main/java/org/apache/james/queue/jms/MimeMessageObjectMessageSource.java
@@ -25,9 +25,12 @@ import javax.jms.JMSException;
import javax.jms.ObjectMessage;
import javax.mail.util.SharedByteArrayInputStream;
+import org.apache.commons.io.IOUtils;
import org.apache.james.core.MimeMessageSource;
import org.apache.james.lifecycle.api.Disposable;
import org.apache.james.lifecycle.api.LifecycleUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* {@link MimeMessageSource} implementation which reads the data from the
@@ -36,6 +39,8 @@ import org.apache.james.lifecycle.api.LifecycleUtil;
*/
public class MimeMessageObjectMessageSource extends MimeMessageSource implements Disposable {
+ private static final Logger LOGGER = LoggerFactory.getLogger(MimeMessageObjectMessageSource.class);
+
private final ObjectMessage message;
private final SharedByteArrayInputStream in;
private final String id;
@@ -65,22 +70,18 @@ public class MimeMessageObjectMessageSource extends MimeMessageSource implements
@Override
public void dispose() {
- try {
- in.close();
- } catch (IOException e1) {
- // ignore on dispose
- }
+ IOUtils.closeQuietly(in);
LifecycleUtil.dispose(in);
try {
message.clearBody();
} catch (JMSException e) {
- // ignore on dispose
+ LOGGER.error("Error clearing JMS message body", e);
}
try {
message.clearProperties();
} catch (JMSException e) {
- // ignore on dispose
+ LOGGER.error("Error clearing JMS message properties", e);
}
content = null;
}
http://git-wip-us.apache.org/repos/asf/james-project/blob/c5758e6c/server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/AbstractJMSMailQueueTest.java
----------------------------------------------------------------------
diff --git a/server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/AbstractJMSMailQueueTest.java b/server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/AbstractJMSMailQueueTest.java
index 3947a58..52b1aef 100644
--- a/server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/AbstractJMSMailQueueTest.java
+++ b/server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/AbstractJMSMailQueueTest.java
@@ -30,6 +30,7 @@ import java.util.Enumeration;
import java.util.Iterator;
import java.util.Properties;
import java.util.UUID;
+import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import javax.jms.ConnectionFactory;
@@ -48,6 +49,7 @@ import org.apache.james.queue.api.ManageableMailQueue.MailQueueIterator;
import org.apache.james.queue.api.RawMailQueueItemDecoratorFactory;
import org.apache.mailet.Mail;
import org.apache.mailet.MailAddress;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.slf4j.Logger;
@@ -61,9 +63,7 @@ public abstract class AbstractJMSMailQueueTest {
protected final static String QUEUE_NAME = "test";
- public abstract JMSMailQueue getQueue();
-
- public abstract void setQueue(JMSMailQueue queue);
+ private JMSMailQueue queue;
protected ActiveMQConnectionFactory createConnectionFactory() {
return new ActiveMQConnectionFactory("vm://localhost?create=false");
@@ -77,12 +77,11 @@ public abstract class AbstractJMSMailQueueTest {
@Before
public void setUp() throws Exception {
ConnectionFactory connectionFactory = createConnectionFactory();
- setQueue(createQueue(connectionFactory, new RawMailQueueItemDecoratorFactory(), QUEUE_NAME));
+ queue = createQueue(connectionFactory, new RawMailQueueItemDecoratorFactory(), QUEUE_NAME);
}
@Test
public void testFIFO() throws MessagingException, InterruptedException, IOException, MailAddressException {
- final JMSMailQueue queue = getQueue();
// should be empty
assertEquals(0, queue.getSize());
@@ -123,7 +122,6 @@ public abstract class AbstractJMSMailQueueTest {
@Test
public void testDelayedDeQueue() throws MessagingException, InterruptedException, IOException, MailAddressException {
- final JMSMailQueue queue = getQueue();
// should be empty
assertEquals(0, queue.getSize());
@@ -159,7 +157,6 @@ public abstract class AbstractJMSMailQueueTest {
@Test
public void testFlush() throws MessagingException, InterruptedException, IOException, MailAddressException {
- final JMSMailQueue queue = getQueue();
// should be empty
assertEquals(0, queue.getSize());
@@ -203,8 +200,6 @@ public abstract class AbstractJMSMailQueueTest {
@Test
public void testRemoveWithRecipient() throws MessagingException, InterruptedException, MailAddressException {
- final JMSMailQueue queue = getQueue();
-
assertEquals(0, queue.getSize());
Mail mail = createMail();
@@ -231,7 +226,6 @@ public abstract class AbstractJMSMailQueueTest {
@Test
public void testRemoveWithSender() throws MessagingException, InterruptedException, MailAddressException {
- final JMSMailQueue queue = getQueue();
assertEquals(0, queue.getSize());
MailImpl mail = createMail();
@@ -258,7 +252,6 @@ public abstract class AbstractJMSMailQueueTest {
@Test
public void testRemoveWithName() throws MessagingException, InterruptedException, MailAddressException {
- final JMSMailQueue queue = getQueue();
assertEquals(0, queue.getSize());
MailImpl mail = createMail();
@@ -335,7 +328,6 @@ public abstract class AbstractJMSMailQueueTest {
@Test
public void testPrioritySupport() throws InterruptedException, MessagingException, IOException, MailAddressException {
- final JMSMailQueue queue = getQueue();
// should be empty
assertEquals(0, queue.getSize());
@@ -371,7 +363,6 @@ public abstract class AbstractJMSMailQueueTest {
@Test
public void testBrowse() throws MessagingException, InterruptedException, IOException, MailAddressException {
- final JMSMailQueue queue = getQueue();
// should be empty
assertEquals(0, queue.getSize());
http://git-wip-us.apache.org/repos/asf/james-project/blob/c5758e6c/server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/JMSMailQueueTest.java
----------------------------------------------------------------------
diff --git a/server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/JMSMailQueueTest.java b/server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/JMSMailQueueTest.java
index 6e7cfc5..d14f65f 100644
--- a/server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/JMSMailQueueTest.java
+++ b/server/queue/queue-jms/src/test/java/org/apache/james/queue/jms/JMSMailQueueTest.java
@@ -10,7 +10,6 @@ import java.util.Arrays;
public class JMSMailQueueTest extends AbstractJMSMailQueueTest {
- private JMSMailQueue queue;
private static BrokerService broker;
@BeforeClass
@@ -43,13 +42,4 @@ public class JMSMailQueueTest extends AbstractJMSMailQueueTest {
return aBroker;
}
- @Override
- public JMSMailQueue getQueue() {
- return queue;
- }
-
- @Override
- public void setQueue(JMSMailQueue queue) {
- this.queue = queue;
- }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org