Shutdown watchdog thread on close

Previously leaked thread instances during test execution

2 files changed, 29 insertions, 19 deletions

diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ContainerWatchdog.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ContainerWatchdog.java
index f6c13529c0d..12a7d413215 100644
--- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ContainerWatchdog.java
+++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ContainerWatchdog.java
@@ -41,9 +41,10 @@ class ContainerWatchdog implements ContainerWatchdogMetrics, AutoCloseable {
 
     private final Object monitor = new Object();
     private final List<DeactivatedContainer> deactivatedContainers = new LinkedList<>();
-    private final ScheduledExecutorService scheduler;
     private final Clock clock;
+    private final boolean enableScheduler;
 
+    private ScheduledExecutorService scheduler;
     private ActiveContainer currentContainer;
     private Instant currentContainerActivationTime;
     private int numStaleContainers;
@@ -53,25 +54,31 @@ class ContainerWatchdog implements ContainerWatchdogMetrics, AutoCloseable {
 
 
     ContainerWatchdog() {
-        this(new ScheduledThreadPoolExecutor(
-                     1,
-                     runnable -> {
-                         Thread thread = new Thread(runnable, "container-watchdog");
-                         thread.setDaemon(true);
-                         return thread;
-                     }),
-             Clock.systemUTC());
-    }
-
-    ContainerWatchdog(ScheduledExecutorService scheduler, Clock clock) {
-        this.scheduler = scheduler;
+        this(Clock.systemUTC(), true);
+    }
+
+    /* For unit testing only */
+    ContainerWatchdog(Clock clock, boolean enableScheduler) {
         this.clock = clock;
         this.lastLogTime = clock.instant();
+        this.enableScheduler = enableScheduler;
     }
 
     void start() {
-        this.containerMontoringTask = scheduler.scheduleAtFixedRate(this::monitorDeactivatedContainers,
-                CONTAINER_CHECK_PERIOD.getSeconds(), CONTAINER_CHECK_PERIOD.getSeconds(), TimeUnit.SECONDS);
+        if (enableScheduler) {
+            if (scheduler == null) this.scheduler = new ScheduledThreadPoolExecutor(
+                    1,
+                    runnable -> {
+                        Thread thread = new Thread(runnable, "container-watchdog");
+                        thread.setDaemon(true);
+                        return thread;
+                    });
+            if (containerMontoringTask != null) containerMontoringTask.cancel(false);
+            if (threadMonitoringTask != null) threadMonitoringTask.cancel(false);
+            containerMontoringTask = scheduler.scheduleAtFixedRate(
+                    this::monitorDeactivatedContainers,
+                    CONTAINER_CHECK_PERIOD.getSeconds(), CONTAINER_CHECK_PERIOD.getSeconds(), TimeUnit.SECONDS);
+        }
     }
 
     @Override
@@ -88,9 +95,14 @@ class ContainerWatchdog implements ContainerWatchdogMetrics, AutoCloseable {
         if (containerMontoringTask != null) containerMontoringTask.cancel(false);
         if (threadMonitoringTask != null) threadMonitoringTask.cancel(false);
         synchronized (monitor) {
+            if (scheduler != null) scheduler.shutdownNow();
             deactivatedContainers.clear();
             currentContainer = null;
             currentContainerActivationTime = null;
+            if (scheduler != null && !scheduler.awaitTermination(10, TimeUnit.SECONDS)) {
+                log.warning("Failed to shutdown container watchdog within 10 seconds");
+            }
+            scheduler = null;
         }
     }
 
@@ -102,7 +114,7 @@ class ContainerWatchdog implements ContainerWatchdogMetrics, AutoCloseable {
             currentContainer = nextContainer;
             currentContainerActivationTime = clock.instant();
             if (threadMonitoringTask != null) threadMonitoringTask.cancel(false);
-            threadMonitoringTask = scheduler.schedule(this::monitorThreads, 1, TimeUnit.MINUTES);
+            if (enableScheduler) threadMonitoringTask = scheduler.schedule(this::monitorThreads, 1, TimeUnit.MINUTES);
         }
     }
 
diff --git a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerWatchdogTest.java b/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerWatchdogTest.java
index 6d178567b75..37dee2d67b3 100644
--- a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerWatchdogTest.java
+++ b/jdisc_core/src/test/java/com/yahoo/jdisc/core/ContainerWatchdogTest.java
@@ -9,10 +9,8 @@ import org.junit.jupiter.api.Test;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.Map;
-import java.util.concurrent.ScheduledExecutorService;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.mockito.Mockito.mock;
 
 /**
  * @author bjorncs
@@ -24,7 +22,7 @@ public class ContainerWatchdogTest {
         TestDriver driver = TestDriver.newSimpleApplicationInstanceWithoutOsgi();
         ManualClock clock = new ManualClock(Instant.EPOCH);
         DummyMetric metric = new DummyMetric();
-        ContainerWatchdog watchdog = new ContainerWatchdog(mock(ScheduledExecutorService.class), clock);
+        ContainerWatchdog watchdog = new ContainerWatchdog(clock, false);
 
         ActiveContainer containerWithoutRetainedResources = new ActiveContainer(driver.newContainerBuilder());