diff options
Diffstat (limited to 'jdisc_core/src')
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; + } + } + } |