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
This commit is contained in:
		| @@ -40,6 +40,8 @@ class CleanupHandle extends WeakReference<ClassLoader> { | |||||||
|     } |     } | ||||||
|     if (!tmpFile.delete() && tmpFile.exists()) { |     if (!tmpFile.delete() && tmpFile.exists()) { | ||||||
|       PluginLoader.log.warn("Cannot delete " + tmpFile.getAbsolutePath()); |       PluginLoader.log.warn("Cannot delete " + tmpFile.getAbsolutePath()); | ||||||
|  |     } else { | ||||||
|  |       PluginLoader.log.info("Cleaned plugin " + tmpFile.getName()); | ||||||
|     } |     } | ||||||
|   } |   } | ||||||
| } | } | ||||||
|   | |||||||
| @@ -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); | ||||||
|  |       } | ||||||
|  |     } | ||||||
|  |   } | ||||||
|  | } | ||||||
| @@ -26,6 +26,7 @@ import com.google.gerrit.server.config.GerritServerConfig; | |||||||
| import com.google.gerrit.server.config.SitePaths; | import com.google.gerrit.server.config.SitePaths; | ||||||
| import com.google.inject.Inject; | import com.google.inject.Inject; | ||||||
| import com.google.inject.Module; | import com.google.inject.Module; | ||||||
|  | import com.google.inject.Provider; | ||||||
| import com.google.inject.Singleton; | import com.google.inject.Singleton; | ||||||
|  |  | ||||||
| import org.eclipse.jgit.lib.Config; | import org.eclipse.jgit.lib.Config; | ||||||
| @@ -68,12 +69,14 @@ public class PluginLoader implements LifecycleListener { | |||||||
|   private final Map<String, FileSnapshot> broken; |   private final Map<String, FileSnapshot> broken; | ||||||
|   private final ReferenceQueue<ClassLoader> cleanupQueue; |   private final ReferenceQueue<ClassLoader> cleanupQueue; | ||||||
|   private final ConcurrentMap<CleanupHandle, Boolean> cleanupHandles; |   private final ConcurrentMap<CleanupHandle, Boolean> cleanupHandles; | ||||||
|  |   private final Provider<PluginCleanerTask> cleaner; | ||||||
|   private final PluginScannerThread scanner; |   private final PluginScannerThread scanner; | ||||||
|  |  | ||||||
|   @Inject |   @Inject | ||||||
|   public PluginLoader(SitePaths sitePaths, |   public PluginLoader(SitePaths sitePaths, | ||||||
|       PluginGuiceEnvironment pe, |       PluginGuiceEnvironment pe, | ||||||
|       ServerInformationImpl sii, |       ServerInformationImpl sii, | ||||||
|  |       Provider<PluginCleanerTask> pct, | ||||||
|       @GerritServerConfig Config cfg) { |       @GerritServerConfig Config cfg) { | ||||||
|     pluginsDir = sitePaths.plugins_dir; |     pluginsDir = sitePaths.plugins_dir; | ||||||
|     dataDir = sitePaths.data_dir; |     dataDir = sitePaths.data_dir; | ||||||
| @@ -84,6 +87,7 @@ public class PluginLoader implements LifecycleListener { | |||||||
|     broken = Maps.newHashMap(); |     broken = Maps.newHashMap(); | ||||||
|     cleanupQueue = new ReferenceQueue<ClassLoader>(); |     cleanupQueue = new ReferenceQueue<ClassLoader>(); | ||||||
|     cleanupHandles = Maps.newConcurrentMap(); |     cleanupHandles = Maps.newConcurrentMap(); | ||||||
|  |     cleaner = pct; | ||||||
|  |  | ||||||
|     long checkFrequency = ConfigUtil.getTimeUnit(cfg, |     long checkFrequency = ConfigUtil.getTimeUnit(cfg, | ||||||
|         "plugins", null, "checkFrequency", |         "plugins", null, "checkFrequency", | ||||||
| @@ -110,7 +114,6 @@ public class PluginLoader implements LifecycleListener { | |||||||
|  |  | ||||||
|     File old = new File(pluginsDir, ".last_" + name + ".zip"); |     File old = new File(pluginsDir, ".last_" + name + ".zip"); | ||||||
|     File tmp = asTemp(in, ".next_" + name, ".zip", pluginsDir); |     File tmp = asTemp(in, ".next_" + name, ".zip", pluginsDir); | ||||||
|     boolean clean = false; |  | ||||||
|     synchronized (this) { |     synchronized (this) { | ||||||
|       Plugin active = running.get(name); |       Plugin active = running.get(name); | ||||||
|       if (active != null) { |       if (active != null) { | ||||||
| @@ -125,18 +128,13 @@ public class PluginLoader implements LifecycleListener { | |||||||
|         runPlugin(name, jar, active); |         runPlugin(name, jar, active); | ||||||
|         if (active == null) { |         if (active == null) { | ||||||
|           log.info(String.format("Installed plugin %s", name)); |           log.info(String.format("Installed plugin %s", name)); | ||||||
|         } else { |  | ||||||
|           clean = true; |  | ||||||
|         } |         } | ||||||
|       } catch (PluginInstallException e) { |       } catch (PluginInstallException e) { | ||||||
|         jar.delete(); |         jar.delete(); | ||||||
|         throw e; |         throw e; | ||||||
|       } |       } | ||||||
|     } |  | ||||||
|  |  | ||||||
|     if (clean) { |       cleanInBackground(); | ||||||
|       System.gc(); |  | ||||||
|       processPendingCleanups(); |  | ||||||
|     } |     } | ||||||
|   } |   } | ||||||
|  |  | ||||||
| @@ -166,7 +164,6 @@ public class PluginLoader implements LifecycleListener { | |||||||
|   } |   } | ||||||
|  |  | ||||||
|   public void disablePlugins(Set<String> names) { |   public void disablePlugins(Set<String> names) { | ||||||
|     boolean clean = false; |  | ||||||
|     synchronized (this) { |     synchronized (this) { | ||||||
|       for (String name : names) { |       for (String name : names) { | ||||||
|         Plugin active = running.get(name); |         Plugin active = running.get(name); | ||||||
| @@ -180,12 +177,8 @@ public class PluginLoader implements LifecycleListener { | |||||||
|  |  | ||||||
|         active.stop(); |         active.stop(); | ||||||
|         running.remove(name); |         running.remove(name); | ||||||
|         clean = true; |  | ||||||
|       } |       } | ||||||
|     } |       cleanInBackground(); | ||||||
|     if (clean) { |  | ||||||
|       System.gc(); |  | ||||||
|       processPendingCleanups(); |  | ||||||
|     } |     } | ||||||
|   } |   } | ||||||
|  |  | ||||||
| @@ -193,7 +186,7 @@ public class PluginLoader implements LifecycleListener { | |||||||
|   public synchronized void start() { |   public synchronized void start() { | ||||||
|     log.info("Loading plugins from " + pluginsDir.getAbsolutePath()); |     log.info("Loading plugins from " + pluginsDir.getAbsolutePath()); | ||||||
|     srvInfoImpl.state = ServerInformation.State.STARTUP; |     srvInfoImpl.state = ServerInformation.State.STARTUP; | ||||||
|     rescan(false); |     rescan(); | ||||||
|     srvInfoImpl.state = ServerInformation.State.RUNNING; |     srvInfoImpl.state = ServerInformation.State.RUNNING; | ||||||
|     if (scanner != null) { |     if (scanner != null) { | ||||||
|       scanner.start(); |       scanner.start(); | ||||||
| @@ -207,26 +200,18 @@ public class PluginLoader implements LifecycleListener { | |||||||
|     } |     } | ||||||
|     srvInfoImpl.state = ServerInformation.State.SHUTDOWN; |     srvInfoImpl.state = ServerInformation.State.SHUTDOWN; | ||||||
|     synchronized (this) { |     synchronized (this) { | ||||||
|       boolean clean = !running.isEmpty(); |  | ||||||
|       for (Plugin p : running.values()) { |       for (Plugin p : running.values()) { | ||||||
|         p.stop(); |         p.stop(); | ||||||
|       } |       } | ||||||
|       running.clear(); |       running.clear(); | ||||||
|       broken.clear(); |       broken.clear(); | ||||||
|       if (clean) { |       if (cleanupHandles.size() > running.size()) { | ||||||
|         System.gc(); |         System.gc(); | ||||||
|         processPendingCleanups(); |         processPendingCleanups(); | ||||||
|       } |       } | ||||||
|     } |     } | ||||||
|   } |   } | ||||||
|  |  | ||||||
|   public void rescan(boolean forceCleanup) { |  | ||||||
|     if (rescanImp() || forceCleanup) { |  | ||||||
|       System.gc(); |  | ||||||
|       processPendingCleanups(); |  | ||||||
|     } |  | ||||||
|   } |  | ||||||
|  |  | ||||||
|   public void reload(List<String> names) |   public void reload(List<String> names) | ||||||
|       throws InvalidPluginException, PluginInstallException { |       throws InvalidPluginException, PluginInstallException { | ||||||
|     synchronized (this) { |     synchronized (this) { | ||||||
| @@ -256,14 +241,14 @@ public class PluginLoader implements LifecycleListener { | |||||||
|           throw e; |           throw e; | ||||||
|         } |         } | ||||||
|       } |       } | ||||||
|       System.gc(); |  | ||||||
|       processPendingCleanups(); |       cleanInBackground(); | ||||||
|     } |     } | ||||||
|   } |   } | ||||||
|  |  | ||||||
|   private synchronized boolean rescanImp() { |   public synchronized void rescan() { | ||||||
|     List<File> jars = scanJarsInPluginsDirectory(); |     List<File> jars = scanJarsInPluginsDirectory(); | ||||||
|     boolean clean = stopRemovedPlugins(jars); |     stopRemovedPlugins(jars); | ||||||
|  |  | ||||||
|     for (File jar : jars) { |     for (File jar : jars) { | ||||||
|       String name = nameOf(jar); |       String name = nameOf(jar); | ||||||
| @@ -285,14 +270,13 @@ public class PluginLoader implements LifecycleListener { | |||||||
|         runPlugin(name, jar, active); |         runPlugin(name, jar, active); | ||||||
|         if (active == null) { |         if (active == null) { | ||||||
|           log.info(String.format("Loaded plugin %s", name)); |           log.info(String.format("Loaded plugin %s", name)); | ||||||
|         } else { |  | ||||||
|           clean = true; |  | ||||||
|         } |         } | ||||||
|       } catch (PluginInstallException e) { |       } catch (PluginInstallException e) { | ||||||
|         log.warn(String.format("Cannot load plugin %s", name), e.getCause()); |         log.warn(String.format("Cannot load plugin %s", name), e.getCause()); | ||||||
|       } |       } | ||||||
|     } |     } | ||||||
|     return clean; |  | ||||||
|  |     cleanInBackground(); | ||||||
|   } |   } | ||||||
|  |  | ||||||
|   private void runPlugin(String name, File jar, Plugin oldPlugin) |   private void runPlugin(String name, File jar, Plugin oldPlugin) | ||||||
| @@ -322,7 +306,7 @@ public class PluginLoader implements LifecycleListener { | |||||||
|     } |     } | ||||||
|   } |   } | ||||||
|  |  | ||||||
|   private boolean stopRemovedPlugins(List<File> jars) { |   private void stopRemovedPlugins(List<File> jars) { | ||||||
|     Set<String> unload = Sets.newHashSet(running.keySet()); |     Set<String> unload = Sets.newHashSet(running.keySet()); | ||||||
|     for (File jar : jars) { |     for (File jar : jars) { | ||||||
|       unload.remove(nameOf(jar)); |       unload.remove(nameOf(jar)); | ||||||
| @@ -331,15 +315,22 @@ public class PluginLoader implements LifecycleListener { | |||||||
|       log.info(String.format("Unloading plugin %s", name)); |       log.info(String.format("Unloading plugin %s", name)); | ||||||
|       running.remove(name).stop(); |       running.remove(name).stop(); | ||||||
|     } |     } | ||||||
|     return !unload.isEmpty(); |  | ||||||
|   } |   } | ||||||
|  |  | ||||||
|   private synchronized void processPendingCleanups() { |   synchronized int processPendingCleanups() { | ||||||
|     CleanupHandle h; |     CleanupHandle h; | ||||||
|     while ((h = (CleanupHandle) cleanupQueue.poll()) != null) { |     while ((h = (CleanupHandle) cleanupQueue.poll()) != null) { | ||||||
|       h.cleanup(); |       h.cleanup(); | ||||||
|       cleanupHandles.remove(h); |       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) { |   private static String nameOf(File jar) { | ||||||
|   | |||||||
| @@ -23,8 +23,10 @@ public class PluginModule extends LifecycleModule { | |||||||
|     bind(ServerInformationImpl.class); |     bind(ServerInformationImpl.class); | ||||||
|     bind(ServerInformation.class).to(ServerInformationImpl.class); |     bind(ServerInformation.class).to(ServerInformationImpl.class); | ||||||
|  |  | ||||||
|  |     bind(PluginCleanerTask.class); | ||||||
|     bind(PluginGuiceEnvironment.class); |     bind(PluginGuiceEnvironment.class); | ||||||
|     bind(PluginLoader.class); |     bind(PluginLoader.class); | ||||||
|  |  | ||||||
|     bind(CopyConfigModule.class); |     bind(CopyConfigModule.class); | ||||||
|     listener().to(PluginLoader.class); |     listener().to(PluginLoader.class); | ||||||
|   } |   } | ||||||
|   | |||||||
| @@ -38,7 +38,7 @@ class PluginScannerThread extends Thread { | |||||||
|         } |         } | ||||||
|       } catch (InterruptedException e) { |       } catch (InterruptedException e) { | ||||||
|       } |       } | ||||||
|       loader.rescan(false); |       loader.rescan(); | ||||||
|     } |     } | ||||||
|   } |   } | ||||||
|  |  | ||||||
|   | |||||||
| @@ -37,7 +37,7 @@ final class PluginReloadCommand extends SshCommand { | |||||||
|   @Override |   @Override | ||||||
|   protected void run() throws UnloggedFailure { |   protected void run() throws UnloggedFailure { | ||||||
|     if (names == null || names.isEmpty()) { |     if (names == null || names.isEmpty()) { | ||||||
|       loader.rescan(true); |       loader.rescan(); | ||||||
|     } else { |     } else { | ||||||
|       try { |       try { | ||||||
|         loader.reload(names); |         loader.reload(names); | ||||||
|   | |||||||
		Reference in New Issue
	
	Block a user
	 Shawn O. Pearce
					Shawn O. Pearce