[activemq] branch activemq-5.15.x updated: Stop failed timer task from breaking timers.

[jb...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

jbonofre pushed a commit to branch activemq-5.15.x
in repository https://gitbox.apache.org/repos/asf/activemq.git


The following commit(s) were added to refs/heads/activemq-5.15.x by this push:
     new 401101f  Stop failed timer task from breaking timers.
401101f is described below

commit 401101f9404dfe079b1ff0fbdd1a15510ef47bc6
Author: Matt Daley <m6...@gmail.com>
AuthorDate: Fri Nov 22 23:14:19 2019 +0000

    Stop failed timer task from breaking timers.
    
    (cherry picked from commit b379879dbfa357930f431bd955c42c78d241834d)
---
 .../apache/activemq/thread/SchedulerTimerTask.java | 25 +++++++++++++++++++++-
 .../org/apache/activemq/thread/SchedulerTest.java  | 17 +++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/activemq-client/src/main/java/org/apache/activemq/thread/SchedulerTimerTask.java b/activemq-client/src/main/java/org/apache/activemq/thread/SchedulerTimerTask.java
index 95cbdb1..55bb323 100644
--- a/activemq-client/src/main/java/org/apache/activemq/thread/SchedulerTimerTask.java
+++ b/activemq-client/src/main/java/org/apache/activemq/thread/SchedulerTimerTask.java
@@ -16,6 +16,9 @@
  */
 package org.apache.activemq.thread;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.util.TimerTask;
 
 /**
@@ -23,6 +26,8 @@ import java.util.TimerTask;
  *
  */
 public class SchedulerTimerTask extends TimerTask {
+    private static final Logger LOGGER = LoggerFactory.getLogger(SchedulerTimerTask.class);
+
     private final Runnable task;
 
     public SchedulerTimerTask(Runnable task) {
@@ -30,6 +35,24 @@ public class SchedulerTimerTask extends TimerTask {
     }
 
     public void run() {
-        this.task.run();                         
+        try {
+            this.task.run();
+        } catch (Error error) {
+            // Very bad error. Can't swallow this but can log it.
+            LOGGER.error("Scheduled task error", error);
+            throw error;
+        } catch (Exception exception) {
+            // It is a known issue of java.util.Timer that if a TimerTask.run() method throws an exception, the
+            // Timer's thread exits as if Timer.cancel() had been called. This makes the Timer completely unusable from
+            // that point on - whenever the Timer triggers there is an 'IllegalStateException: Timer already cancelled'
+            // and the task does not get executed.
+            //
+            // In practice, this makes the ActiveMQ client unable to refresh connections to brokers. Generally, tasks
+            // are well coded to not throw exceptions but if they do, problems ensue...
+            //
+            // The answer, here, is to log the exception and then carry on without throwing further. This gives the
+            // added benefit that, having seen the exception thrown, one can then go and fix the offending task!
+            LOGGER.error("Scheduled task exception", exception);
+        }
     }
 }
\ No newline at end of file
diff --git a/activemq-client/src/test/java/org/apache/activemq/thread/SchedulerTest.java b/activemq-client/src/test/java/org/apache/activemq/thread/SchedulerTest.java
index d63c831..e0743b3 100644
--- a/activemq-client/src/test/java/org/apache/activemq/thread/SchedulerTest.java
+++ b/activemq-client/src/test/java/org/apache/activemq/thread/SchedulerTest.java
@@ -69,16 +69,33 @@ public class SchedulerTest {
         assertFalse(latch.await(1000, TimeUnit.MILLISECONDS));
     }
 
+    @Test
+    public void testExecutePeriodicallyTaskExceptionDoesNotBreakTimer() throws Exception {
+        final CountDownLatch latch = new CountDownLatch(10);
+        scheduler.executePeriodically(new CountDownRunnable(latch, 5L), 10);
+        assertTrue(latch.await(5000, TimeUnit.MILLISECONDS));
+    }
+
     private static class CountDownRunnable implements Runnable {
         final CountDownLatch latch;
+        final Long throwAtCount;
 
         CountDownRunnable(final CountDownLatch latch) {
+            this(latch, null);
+        }
+
+        CountDownRunnable(final CountDownLatch latch, Long throwAtCount) {
             this.latch = latch;
+            this.throwAtCount = throwAtCount;
         }
 
         @Override
         public void run() {
             latch.countDown();
+
+            if (throwAtCount != null && latch.getCount() == throwAtCount) {
+                throw new RuntimeException("You never want this to happen in a real task!!");
+            }
         }
     }