Reintroduce enforced destruction of ActiveContainer (#2712)

Reintroduce enforced destruction of ActiveContainer - without using finalizer

3 files changed, 149 insertions, 17 deletions

diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java
index fbc45f000d2..27ce25affef 100644
--- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java
+++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java
@@ -17,6 +17,7 @@ import com.yahoo.jdisc.service.ServerProvider;
 
 import java.net.URI;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * @author <a href="mailto:simon@yahoo-inc.com">Simon Thoresen</a>
@@ -31,6 +32,7 @@ public class ActiveContainer extends AbstractResource implements CurrentContaine
     private final Map<String, BindingSet<RequestHandler>> clientBindings;
     private final BindingSetSelector bindingSetSelector;
     private final TimeoutManagerImpl timeoutMgr;
+    final Destructor destructor;
 
     public ActiveContainer(ContainerBuilder builder) {
         serverProviders = builder.serverProviders().activate();
@@ -55,13 +57,16 @@ public class ActiveContainer extends AbstractResource implements CurrentContaine
         });
         guiceInjector = builder.guiceModules().activate();
         termination = new ContainerTermination(builder.appContext());
+        destructor = new Destructor(resourceReferences, timeoutMgr, termination);
     }
 
     @Override
     protected void destroy() {
-        resourceReferences.release();
-        timeoutMgr.shutdown();
-        termination.run();
+        boolean alreadyDestructed = destructor.destruct();
+        if (alreadyDestructed) {
+            throw new IllegalStateException(
+                    "Already destructed! This should not occur unless destroy have been called directly!");
+        }
     }
 
     /**
@@ -116,4 +121,30 @@ public class ActiveContainer extends AbstractResource implements CurrentContaine
         }
         return new ContainerSnapshot(this, serverBindings, clientBindings);
     }
+
+    // NOTE: An instance of this class must never contain a reference to the outer class (ActiveContainer).
+    static class Destructor {
+        private final ResourcePool resourceReferences;
+        private final TimeoutManagerImpl timeoutMgr;
+        private final ContainerTermination termination;
+        private final AtomicBoolean done = new AtomicBoolean();
+
+        private Destructor(ResourcePool resourceReferences,
+                           TimeoutManagerImpl timeoutMgr,
+                           ContainerTermination termination) {
+            this.resourceReferences = resourceReferences;
+            this.timeoutMgr = timeoutMgr;
+            this.termination = termination;
+        }
+
+        boolean destruct() {
+            boolean alreadyDestructed = this.done.getAndSet(true);
+            if (!alreadyDestructed) {
+                resourceReferences.release();
+                timeoutMgr.shutdown();
+                termination.run();
+            }
+            return alreadyDestructed;
+        }
+    }
 }
diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdog.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdog.java
index 0004d1d818b..90c410d5e22 100644
--- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdog.java
+++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdog.java
@@ -5,10 +5,14 @@ import com.google.inject.Inject;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.jdisc.statistics.ActiveContainerMetrics;
 
+import java.lang.ref.PhantomReference;
+import java.lang.ref.ReferenceQueue;
 import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.WeakHashMap;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledThreadPoolExecutor;
@@ -29,11 +33,16 @@ class ActiveContainerDeactivationWatchdog implements ActiveContainerMetrics, Aut
     static final Duration WATCHDOG_FREQUENCY = Duration.ofMinutes(20);
     static final Duration ACTIVE_CONTAINER_GRACE_PERIOD = Duration.ofHours(1);
     static final Duration GC_TRIGGER_FREQUENCY = ACTIVE_CONTAINER_GRACE_PERIOD.minusMinutes(5);
+    static final Duration ENFORCE_DESTRUCTION_GCED_CONTAINERS_FREQUENCY = Duration.ofMinutes(5);
 
     private static final Logger log = Logger.getLogger(ActiveContainerDeactivationWatchdog.class.getName());
 
     private final Object monitor = new Object();
     private final WeakHashMap<ActiveContainer, LifecycleStats> deactivatedContainers = new WeakHashMap<>();
+    private final ReferenceQueue<ActiveContainer> garbageCollectedContainers = new ReferenceQueue<>();
+    @SuppressWarnings("MismatchedQueryAndUpdateOfCollection")
+    // Instances of the phantom references must be kept alive until they are polled from the reference queue
+    private final Set<ActiveContainerPhantomReference> destructorReferences = new HashSet<>();
     private final ScheduledExecutorService scheduler;
     private final Clock clock;
 
@@ -44,7 +53,7 @@ class ActiveContainerDeactivationWatchdog implements ActiveContainerMetrics, Aut
     ActiveContainerDeactivationWatchdog() {
         this(
                 Clock.systemUTC(),
-                new ScheduledThreadPoolExecutor(2, runnable -> {
+                new ScheduledThreadPoolExecutor(3, runnable -> {
                     Thread thread = new Thread(runnable, "active-container-deactivation-watchdog");
                     thread.setDaemon(true);
                     return thread;
@@ -54,26 +63,30 @@ class ActiveContainerDeactivationWatchdog implements ActiveContainerMetrics, Aut
     ActiveContainerDeactivationWatchdog(Clock clock, ScheduledExecutorService scheduler) {
         this.clock = clock;
         this.scheduler = scheduler;
-        this.scheduler.scheduleAtFixedRate(
-                this::warnOnStaleContainers,
-                WATCHDOG_FREQUENCY.getSeconds(),
-                WATCHDOG_FREQUENCY.getSeconds(),
-                TimeUnit.SECONDS);
-        this.scheduler.scheduleAtFixedRate(
-                System::gc,
-                GC_TRIGGER_FREQUENCY.getSeconds(),
-                GC_TRIGGER_FREQUENCY.getSeconds(),
-                TimeUnit.SECONDS);
+        this.scheduler.scheduleAtFixedRate(this::warnOnStaleContainers,
+                                           WATCHDOG_FREQUENCY.getSeconds(),
+                                           WATCHDOG_FREQUENCY.getSeconds(),
+                                           TimeUnit.SECONDS);
+        this.scheduler.scheduleAtFixedRate(System::gc,
+                                           GC_TRIGGER_FREQUENCY.getSeconds(),
+                                           GC_TRIGGER_FREQUENCY.getSeconds(),
+                                           TimeUnit.SECONDS);
+        this.scheduler.scheduleAtFixedRate(this::enforceDestructionOfGarbageCollectedContainers,
+                                           ENFORCE_DESTRUCTION_GCED_CONTAINERS_FREQUENCY.getSeconds(),
+                                           ENFORCE_DESTRUCTION_GCED_CONTAINERS_FREQUENCY.getSeconds(),
+                                           TimeUnit.SECONDS);
     }
 
     void onContainerActivation(ActiveContainer nextContainer) {
         synchronized (monitor) {
             Instant now = clock.instant();
-            if (currentContainer != null) {
-                deactivatedContainers.put(currentContainer, new LifecycleStats(currentContainerActivationTime, now));
-            }
+            ActiveContainer previousContainer = currentContainer;
             currentContainer = nextContainer;
             currentContainerActivationTime = now;
+            if (previousContainer != null) {
+                deactivatedContainers.put(previousContainer, new LifecycleStats(currentContainerActivationTime, now));
+                destructorReferences.add(new ActiveContainerPhantomReference(previousContainer, garbageCollectedContainers));
+            }
         }
     }
 
@@ -92,6 +105,7 @@ class ActiveContainerDeactivationWatchdog implements ActiveContainerMetrics, Aut
         synchronized (monitor) {
             scheduler.shutdown();
             deactivatedContainers.clear();
+            destructorReferences.clear();
             currentContainer = null;
             currentContainerActivationTime = null;
         }
@@ -107,6 +121,20 @@ class ActiveContainerDeactivationWatchdog implements ActiveContainerMetrics, Aut
         }
     }
 
+    private void enforceDestructionOfGarbageCollectedContainers() {
+        ActiveContainerPhantomReference reference;
+        while ((reference = (ActiveContainerPhantomReference) garbageCollectedContainers.poll()) != null) {
+            try {
+                reference.enforceDestruction();
+            } catch (Throwable t) {
+                log.log(Level.SEVERE, "Failed to do post-GC destruction of " + reference.containerName, t);
+            } finally {
+                destructorReferences.remove(reference);
+                reference.clear();
+            }
+        }
+    }
+
     private List<DeactivatedContainer> getDeactivatedContainersSnapshot() {
         Instant now = clock.instant();
         synchronized (monitor) {
@@ -157,4 +185,23 @@ class ActiveContainerDeactivationWatchdog implements ActiveContainerMetrics, Aut
         }
     }
 
+    private static class ActiveContainerPhantomReference extends PhantomReference<ActiveContainer> {
+        public final String containerName;
+        private final ActiveContainer.Destructor destructor;
+
+        public ActiveContainerPhantomReference(ActiveContainer activeContainer,
+                                               ReferenceQueue<? super ActiveContainer> q) {
+            super(activeContainer, q);
+            this.containerName = activeContainer.toString();
+            this.destructor = activeContainer.destructor;
+        }
+
+        public void enforceDestruction() {
+            boolean alreadyCompleted = destructor.destruct();
+            if (!alreadyCompleted) {
+                log.severe(containerName + " was not correctly cleaned up " +
+                        "because of a resource leak or invalid use of reference counting.");
+            }
+        }
+    }
 }
diff --git a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdogTest.java b/jdisc_core/src/test/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdogTest.java
index 1a776f59e29..09ca2f93ff1 100644
--- a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdogTest.java
+++ b/jdisc_core/src/test/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdogTest.java
@@ -7,12 +7,20 @@ import com.yahoo.jdisc.test.TestDriver;
 import com.yahoo.test.ManualClock;
 import org.junit.Test;
 
+import java.lang.ref.WeakReference;
+import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.Map;
 import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
 /**
  * @author bjorncs
@@ -59,6 +67,31 @@ public class ActiveContainerDeactivationWatchdogTest {
 
     }
 
+    @Test
+    public void deactivated_container_destructed_if_its_reference_counter_is_nonzero() {
+        ExecutorMock executor = new ExecutorMock();
+        ActiveContainerDeactivationWatchdog watchdog =
+                new ActiveContainerDeactivationWatchdog(Clock.systemUTC(), executor);
+        ActiveContainer container =
+                new ActiveContainer(TestDriver.newSimpleApplicationInstanceWithoutOsgi().newContainerBuilder());
+        AtomicBoolean destructed = new AtomicBoolean(false);
+        container.shutdown().notifyTermination(() -> destructed.set(true));
+
+        container.refer(); // increase reference counter to simluate a leaking resource
+        watchdog.onContainerActivation(container);
+        container.release(); // release resource
+        watchdog.onContainerActivation(null); // deactive container
+
+        WeakReference<ActiveContainer> containerWeakReference = new WeakReference<>(container);
+        container = null; // make container instance collectable by GC
+        System.gc();
+
+        assertNull("Container is not GCed - probably because the watchdog has a concrete reference to it",
+                   containerWeakReference.get());
+        executor.containerDestructorCommand.run();
+        assertTrue("Destructor is not called on deactivated container", destructed.get());
+    }
+
     private static class MockMetric implements Metric {
         public int totalCount;
         public int withRetainedReferencesCount;
@@ -88,4 +121,25 @@ public class ActiveContainerDeactivationWatchdogTest {
         }
     }
 
+    private static class ExecutorMock extends ScheduledThreadPoolExecutor {
+
+        public Runnable containerDestructorCommand;
+        private int registrationCounter = 0;
+
+        public ExecutorMock() {
+            super(1);
+        }
+
+        @Override
+        public ScheduledFuture<?> scheduleAtFixedRate(Runnable command, long initialDelay, long period, TimeUnit unit) {
+            if (registrationCounter == 2) {
+                containerDestructorCommand = command;
+            } else if (registrationCounter > 2){
+                throw new IllegalStateException("Unexpected registration");
+            }
+            ++registrationCounter;
+            return null;
+        }
+    }
+
 }