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 <btell...@linagora.com> Authored: Tue Jul 4 16:32:17 2017 +0700 Committer: benwa <btell...@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-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org
