james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From adup...@apache.org
Subject [31/34] james-project git commit: JAMES-2085 Stop swallowing exceptions in JMS related code
Date Wed, 05 Jul 2017 13:59:38 GMT
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 <btellier@linagora.com>
Authored: Tue Jul 4 16:32:17 2017 +0700
Committer: benwa <btellier@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


Mime
View raw message