Repository: Unleash/unleash-client-java
Pull request #103 (2020-03-12 21:33:16)

fix: add `shutdown()` to `Unleash`
This PR introduces a new `shutdown()` method to 2 interfaces, `Unleash` and `UnleashScheduledExecutor`

In order to properly shutdown a server that's running an Unleash client, all scheduled threads should be shutdown gracefully.

With the current implementation there is no clean way to shutdown the `UnleashScheduledExecutor`. If we're using the default implementation of `UnleashScheduledExecutorImpl`, all fields that would allow us to do so are either private, or are swallowed; The `ScheduledFuture` object returned from `setInterval` is not set to any field in the 2 constructors that call it, `UnleashMetricServiceImpl` and `FeatureToggleRepository`. Nor is the `ScheduledThreadPoolExecutor` exposed (it has a `shutdown()` method).

Our only option right now is to implement our own `UnleashScheduledExecutor`, pass it in as the config, and have a `close()` or `shutdown()` method on it. (we can't even use `UnleashScheduledExecutorImpl` because of the private fields.

The implementation gets the executor from the `config` instance and call `shutdown()` on it. That way, no matter which implementation of `UnleashScheduledExecutor` one would use, they can shut it down via the main interface.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/29313749/badge)](https://coveralls.io/builds/29313749)

Coverage decreased (-0.4%) to 92.604% when pulling **3cb63010645a43708900053bc49c320a16a1fc32 on Enrico2:master** into **ca50b3b9121327a05d7b2db8c980c82dacb94a62 on Unleash:master**.


Patch:
diff --git a/src/main/java/no/finn/unleash/DefaultUnleash.java b/src/main/java/no/finn/unleash/DefaultUnleash.java
index c63ebcf0..452d0577 100644
--- a/src/main/java/no/finn/unleash/DefaultUnleash.java
+++ b/src/main/java/no/finn/unleash/DefaultUnleash.java
@@ -162,4 +162,9 @@ public final class DefaultUnleash implements Unleash {
     private Strategy getStrategy(String strategy) {
         return strategyMap.containsKey(strategy) ? strategyMap.get(strategy) : UNKNOWN_STRATEGY;
     }
+
+    @Override
+    public void shutdown() {
+        config.getScheduledExecutor().shutdown();
+    }
 }
diff --git a/src/main/java/no/finn/unleash/Unleash.java b/src/main/java/no/finn/unleash/Unleash.java
index 3bd04b80..83335327 100644
--- a/src/main/java/no/finn/unleash/Unleash.java
+++ b/src/main/java/no/finn/unleash/Unleash.java
@@ -33,4 +33,6 @@ public interface Unleash {
     Variant getVariant(final String toggleName, final Variant defaultValue);
 
     List<String> getFeatureToggleNames();
+
+    default void shutdown() {}
 }
diff --git a/src/main/java/no/finn/unleash/util/UnleashScheduledExecutor.java b/src/main/java/no/finn/unleash/util/UnleashScheduledExecutor.java
index 35a00938..f1be0cef 100644
--- a/src/main/java/no/finn/unleash/util/UnleashScheduledExecutor.java
+++ b/src/main/java/no/finn/unleash/util/UnleashScheduledExecutor.java
@@ -1,11 +1,15 @@
 package no.finn.unleash.util;
 
-import java.util.concurrent.*;
+import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.ScheduledFuture;
 
 public interface UnleashScheduledExecutor {
-       ScheduledFuture setInterval(
-               Runnable command, long initialDelaySec, long periodSec) throws RejectedExecutionException;
+    ScheduledFuture setInterval(
+            Runnable command, long initialDelaySec, long periodSec) throws RejectedExecutionException;
 
-       Future<Void> scheduleOnce(Runnable runnable);
+    Future<Void> scheduleOnce(Runnable runnable);
 
+    default public void shutdown() {
+    }
 }
diff --git a/src/main/java/no/finn/unleash/util/UnleashScheduledExecutorImpl.java b/src/main/java/no/finn/unleash/util/UnleashScheduledExecutorImpl.java
index 7d3169c6..2874305f 100644
--- a/src/main/java/no/finn/unleash/util/UnleashScheduledExecutorImpl.java
+++ b/src/main/java/no/finn/unleash/util/UnleashScheduledExecutorImpl.java
@@ -53,4 +53,8 @@ public class UnleashScheduledExecutorImpl implements UnleashScheduledExecutor {
         return (Future<Void>) executorService.submit(runnable);
     }
 
+    @Override
+    public void shutdown() {
+        this.scheduledThreadPoolExecutor.shutdown();
+    }
 }


Test patch:
diff --git a/src/test/java/no/finn/unleash/util/UnleashScheduledExecutorImplTest.java b/src/test/java/no/finn/unleash/util/UnleashScheduledExecutorImplTest.java
index 87e5f5ba..e11bd4c9 100644
--- a/src/test/java/no/finn/unleash/util/UnleashScheduledExecutorImplTest.java
+++ b/src/test/java/no/finn/unleash/util/UnleashScheduledExecutorImplTest.java
@@ -1,5 +1,6 @@
 package no.finn.unleash.util;
 
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -9,6 +10,11 @@ public class UnleashScheduledExecutorImplTest {
     private UnleashScheduledExecutorImpl unleashScheduledExecutor = new UnleashScheduledExecutorImpl();
     private int periodicalTaskCounter;
 
+    @BeforeEach
+    public void setup() {
+        this.periodicalTaskCounter = 0;
+    }
+
     @Test
     public void scheduleOnce_doNotInterfereWithPeriodicalTasks() {
         unleashScheduledExecutor.setInterval(this::periodicalTask, 0, 1);
@@ -29,4 +35,11 @@ public class UnleashScheduledExecutorImplTest {
         this.periodicalTaskCounter++;
     }
 
-}
\ No newline at end of file
+    @Test
+    public void shutdown_stopsRunningScheduledTasks() {
+        unleashScheduledExecutor.setInterval(this::periodicalTask, 5, 1);
+        unleashScheduledExecutor.shutdown();
+        sleep5seconds();
+        assertThat(periodicalTaskCounter).isEqualTo(0);
+    }
+}