From b15212aa8f4dec5bee5e69298f46c0dd62a199b5 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 11 May 2012 16:50:18 -0700 Subject: [PATCH] Perform plugin cleanup in the background When a plugin is stopped, schedule a Plugin Cleaner task to run 1 minute later to try and clean out the garbage and release the JAR from $site_path/tmp. If the cleaner doesn't succeed, schedule again for 1 minute later until it does. Each failed clean attempt backs off by another minute until it tries once every 15 minutes (4 times per hour). Plugins that don't clean are probably still linked into the live object graph somewhere and cannot be garbage collected. This indicates a memory leak, and the server administrator may need to restart the server to free up memory. Leaking should only happen with badly coded plugins that attempt to manually manage their listener registrations, or start threads they don't terminate. Change-Id: I0ca8c2c70b6fd9bcfbb7444ea3995210c9d80c2f --- .../gerrit/server/plugins/CleanupHandle.java | 2 + .../server/plugins/PluginCleanerTask.java | 100 ++++++++++++++++++ .../gerrit/server/plugins/PluginLoader.java | 57 +++++----- .../gerrit/server/plugins/PluginModule.java | 2 + .../server/plugins/PluginScannerThread.java | 2 +- .../sshd/commands/PluginReloadCommand.java | 2 +- 6 files changed, 130 insertions(+), 35 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginCleanerTask.java diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/CleanupHandle.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/CleanupHandle.java index e18d840d26..7018a3b7f5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/CleanupHandle.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/CleanupHandle.java @@ -40,6 +40,8 @@ class CleanupHandle extends WeakReference { } if (!tmpFile.delete() && tmpFile.exists()) { PluginLoader.log.warn("Cannot delete " + tmpFile.getAbsolutePath()); + } else { + PluginLoader.log.info("Cleaned plugin " + tmpFile.getName()); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginCleanerTask.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginCleanerTask.java new file mode 100644 index 0000000000..7081d706d7 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginCleanerTask.java @@ -0,0 +1,100 @@ +// Copyright (C) 2012 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.plugins; + +import com.google.gerrit.server.git.WorkQueue; +import com.google.inject.Inject; +import com.google.inject.Singleton; + +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; + +@Singleton +class PluginCleanerTask implements Runnable { + private final WorkQueue workQueue; + private final PluginLoader loader; + private volatile int pending; + private Future self; + private int attempts; + private long start; + + @Inject + PluginCleanerTask(WorkQueue workQueue, PluginLoader loader) { + this.workQueue = workQueue; + this.loader = loader; + } + + @Override + public void run() { + try { + for (int t = 0; t < 2 * (attempts + 1); t++) { + System.gc(); + Thread.sleep(50); + } + } catch (InterruptedException e) { + } + + int left = loader.processPendingCleanups(); + synchronized (this) { + pending = left; + self = null; + + if (0 < left) { + long waiting = System.currentTimeMillis() - start; + PluginLoader.log.warn(String.format( + "%d plugins still waiting to be reclaimed after %d minutes", + pending, + TimeUnit.MILLISECONDS.toMinutes(waiting))); + attempts = Math.min(attempts + 1, 15); + ensureScheduled(); + } else { + attempts = 0; + } + } + } + + @Override + public String toString() { + int p = pending; + if (0 < p) { + return String.format("Plugin Cleaner (waiting for %d plugins)", p); + } + return "Plugin Cleaner"; + } + + synchronized void clean(int expect) { + if (self == null && pending == 0) { + start = System.currentTimeMillis(); + } + pending = expect; + ensureScheduled(); + } + + private void ensureScheduled() { + if (self == null && 0 < pending) { + if (attempts == 1) { + self = workQueue.getDefaultQueue().schedule( + this, + 30, + TimeUnit.SECONDS); + } else { + self = workQueue.getDefaultQueue().schedule( + this, + attempts + 1, + TimeUnit.MINUTES); + } + } + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginLoader.java index 828a88d33c..324e3de433 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginLoader.java @@ -26,6 +26,7 @@ import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; import com.google.inject.Inject; import com.google.inject.Module; +import com.google.inject.Provider; import com.google.inject.Singleton; import org.eclipse.jgit.lib.Config; @@ -68,12 +69,14 @@ public class PluginLoader implements LifecycleListener { private final Map broken; private final ReferenceQueue cleanupQueue; private final ConcurrentMap cleanupHandles; + private final Provider cleaner; private final PluginScannerThread scanner; @Inject public PluginLoader(SitePaths sitePaths, PluginGuiceEnvironment pe, ServerInformationImpl sii, + Provider pct, @GerritServerConfig Config cfg) { pluginsDir = sitePaths.plugins_dir; dataDir = sitePaths.data_dir; @@ -84,6 +87,7 @@ public class PluginLoader implements LifecycleListener { broken = Maps.newHashMap(); cleanupQueue = new ReferenceQueue(); cleanupHandles = Maps.newConcurrentMap(); + cleaner = pct; long checkFrequency = ConfigUtil.getTimeUnit(cfg, "plugins", null, "checkFrequency", @@ -110,7 +114,6 @@ public class PluginLoader implements LifecycleListener { File old = new File(pluginsDir, ".last_" + name + ".zip"); File tmp = asTemp(in, ".next_" + name, ".zip", pluginsDir); - boolean clean = false; synchronized (this) { Plugin active = running.get(name); if (active != null) { @@ -125,18 +128,13 @@ public class PluginLoader implements LifecycleListener { runPlugin(name, jar, active); if (active == null) { log.info(String.format("Installed plugin %s", name)); - } else { - clean = true; } } catch (PluginInstallException e) { jar.delete(); throw e; } - } - if (clean) { - System.gc(); - processPendingCleanups(); + cleanInBackground(); } } @@ -166,7 +164,6 @@ public class PluginLoader implements LifecycleListener { } public void disablePlugins(Set names) { - boolean clean = false; synchronized (this) { for (String name : names) { Plugin active = running.get(name); @@ -180,12 +177,8 @@ public class PluginLoader implements LifecycleListener { active.stop(); running.remove(name); - clean = true; } - } - if (clean) { - System.gc(); - processPendingCleanups(); + cleanInBackground(); } } @@ -193,7 +186,7 @@ public class PluginLoader implements LifecycleListener { public synchronized void start() { log.info("Loading plugins from " + pluginsDir.getAbsolutePath()); srvInfoImpl.state = ServerInformation.State.STARTUP; - rescan(false); + rescan(); srvInfoImpl.state = ServerInformation.State.RUNNING; if (scanner != null) { scanner.start(); @@ -207,26 +200,18 @@ public class PluginLoader implements LifecycleListener { } srvInfoImpl.state = ServerInformation.State.SHUTDOWN; synchronized (this) { - boolean clean = !running.isEmpty(); for (Plugin p : running.values()) { p.stop(); } running.clear(); broken.clear(); - if (clean) { + if (cleanupHandles.size() > running.size()) { System.gc(); processPendingCleanups(); } } } - public void rescan(boolean forceCleanup) { - if (rescanImp() || forceCleanup) { - System.gc(); - processPendingCleanups(); - } - } - public void reload(List names) throws InvalidPluginException, PluginInstallException { synchronized (this) { @@ -256,14 +241,14 @@ public class PluginLoader implements LifecycleListener { throw e; } } - System.gc(); - processPendingCleanups(); + + cleanInBackground(); } } - private synchronized boolean rescanImp() { + public synchronized void rescan() { List jars = scanJarsInPluginsDirectory(); - boolean clean = stopRemovedPlugins(jars); + stopRemovedPlugins(jars); for (File jar : jars) { String name = nameOf(jar); @@ -285,14 +270,13 @@ public class PluginLoader implements LifecycleListener { runPlugin(name, jar, active); if (active == null) { log.info(String.format("Loaded plugin %s", name)); - } else { - clean = true; } } catch (PluginInstallException e) { log.warn(String.format("Cannot load plugin %s", name), e.getCause()); } } - return clean; + + cleanInBackground(); } private void runPlugin(String name, File jar, Plugin oldPlugin) @@ -322,7 +306,7 @@ public class PluginLoader implements LifecycleListener { } } - private boolean stopRemovedPlugins(List jars) { + private void stopRemovedPlugins(List jars) { Set unload = Sets.newHashSet(running.keySet()); for (File jar : jars) { unload.remove(nameOf(jar)); @@ -331,15 +315,22 @@ public class PluginLoader implements LifecycleListener { log.info(String.format("Unloading plugin %s", name)); running.remove(name).stop(); } - return !unload.isEmpty(); } - private synchronized void processPendingCleanups() { + synchronized int processPendingCleanups() { CleanupHandle h; while ((h = (CleanupHandle) cleanupQueue.poll()) != null) { h.cleanup(); cleanupHandles.remove(h); } + return Math.max(0, cleanupHandles.size() - running.size()); + } + + private void cleanInBackground() { + int cnt = Math.max(0, cleanupHandles.size() - running.size()); + if (0 < cnt) { + cleaner.get().clean(cnt); + } } private static String nameOf(File jar) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginModule.java index 5f5b8e3633..ab7dc3c839 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginModule.java @@ -23,8 +23,10 @@ public class PluginModule extends LifecycleModule { bind(ServerInformationImpl.class); bind(ServerInformation.class).to(ServerInformationImpl.class); + bind(PluginCleanerTask.class); bind(PluginGuiceEnvironment.class); bind(PluginLoader.class); + bind(CopyConfigModule.class); listener().to(PluginLoader.class); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginScannerThread.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginScannerThread.java index b2e3fedeb3..a484c5d44b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginScannerThread.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginScannerThread.java @@ -38,7 +38,7 @@ class PluginScannerThread extends Thread { } } catch (InterruptedException e) { } - loader.rescan(false); + loader.rescan(); } } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PluginReloadCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PluginReloadCommand.java index e01c6d8dd0..d60465cca2 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PluginReloadCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PluginReloadCommand.java @@ -37,7 +37,7 @@ final class PluginReloadCommand extends SshCommand { @Override protected void run() throws UnloggedFailure { if (names == null || names.isEmpty()) { - loader.rescan(true); + loader.rescan(); } else { try { loader.reload(names);