From 35d66cbcfe44b9388d03178ba23ef091de700ae3 Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Mon, 4 Mar 2013 21:19:28 +0100 Subject: [PATCH] Fix unloading of plugins When two plugins, say pluginA, and pluginB have been loaded, and pluginA is removed from $sitePath/plugins, pluginA gets stopped, and a cleaning run is ordered. But this cleaning run cleans both plugins and both plugins have their jars removed. This leaves pluginB visible to gerrit although it's backing jar is gone. Upon calling not yet initialized parts of pluginB (e.g.: viewing not yet viewed Documentation pages on pluginB), exceptions as java.lang.IllegalStateException: zip file closed at java.util.zip.ZipFile.ensureOpen(ZipFile.java:420) at java.util.zip.ZipFile.getEntry(ZipFile.java:165) get thrown. Additionally, it seems that since commit fac27716d8e1883c78c2c7313c31f9393c25a1a7 a plugin's class loader is garbage collected only after the plugin itself is garbage collected, as plugins hold a reference to their class loader. Thereby making the burden of using ReferenceQueue and WeakReferences unneeded. As the solution to those two issues cannot be properly isolated we fix both in one go. We * stop enqueuing a plugin for cleaning up already upon creating of the plugin, * get rid of using ReferenceQueue and WeakReference, * introduce a single point to unload a plugin, and * stop determining how many handles have to be cleaned by subtracting the number of running handles from the number of all (i.e.: running, disabled, and to be cleaned) handles, as this avoids settling in the cleaning thread, when having disabled plugins. Thereby, we can reliably unload a single plugin, without affecting the other loaded plugins. Change-Id: I962b4009907c1815cc5859c06343ca9a0de7a293 --- .../gerrit/server/plugins/CleanupHandle.java | 9 +-- .../gerrit/server/plugins/PluginLoader.java | 63 +++++++++++-------- 2 files changed, 38 insertions(+), 34 deletions(-) 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 50efb68fe5..b04f337d18 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 @@ -16,19 +16,14 @@ package com.google.gerrit.server.plugins; import java.io.File; import java.io.IOException; -import java.lang.ref.ReferenceQueue; -import java.lang.ref.WeakReference; import java.util.jar.JarFile; -class CleanupHandle extends WeakReference { +class CleanupHandle { private final File tmpFile; private final JarFile jarFile; CleanupHandle(File tmpFile, - JarFile jarFile, - ClassLoader ref, - ReferenceQueue queue) { - super(ref, queue); + JarFile jarFile) { this.tmpFile = tmpFile; this.jarFile = jarFile; } 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 4e565d3978..b3f3183382 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 @@ -18,6 +18,7 @@ import com.google.common.base.Joiner; import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Queues; import com.google.common.collect.Sets; import com.google.gerrit.extensions.annotations.PluginName; import com.google.gerrit.extensions.events.LifecycleListener; @@ -42,7 +43,6 @@ import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; -import java.lang.ref.ReferenceQueue; import java.net.URL; import java.net.URLClassLoader; import java.text.SimpleDateFormat; @@ -50,8 +50,11 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Date; +import java.util.Hashtable; +import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Queue; import java.util.Set; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; @@ -72,8 +75,8 @@ public class PluginLoader implements LifecycleListener { private final ConcurrentMap running; private final ConcurrentMap disabled; private final Map broken; - private final ReferenceQueue cleanupQueue; - private final ConcurrentMap cleanupHandles; + private final Map cleanupHandles; + private final Queue toCleanup; private final Provider cleaner; private final PluginScannerThread scanner; @@ -91,8 +94,8 @@ public class PluginLoader implements LifecycleListener { running = Maps.newConcurrentMap(); disabled = Maps.newConcurrentMap(); broken = Maps.newHashMap(); - cleanupQueue = new ReferenceQueue(); - cleanupHandles = Maps.newConcurrentMap(); + toCleanup = Queues.newArrayDeque(); + cleanupHandles = new Hashtable(); cleaner = pct; long checkFrequency = ConfigUtil.getTimeUnit(cfg, @@ -188,6 +191,15 @@ public class PluginLoader implements LifecycleListener { } } + synchronized private void unloadPlugin(Plugin plugin) { + String name = plugin.getName(); + log.info(String.format("Unloading plugin %s", name)); + plugin.stop(); + running.remove(name); + disabled.remove(name); + toCleanup.add(plugin); + } + public void disablePlugins(Set names) { synchronized (this) { for (String name : names) { @@ -200,8 +212,7 @@ public class PluginLoader implements LifecycleListener { File off = new File(pluginsDir, active.getName() + ".jar.disabled"); active.getSrcJar().renameTo(off); - active.stop(); - running.remove(name); + unloadPlugin(active); try { FileSnapshot snapshot = FileSnapshot.save(off); Plugin offPlugin = loadPlugin(name, off, snapshot); @@ -254,12 +265,12 @@ public class PluginLoader implements LifecycleListener { srvInfoImpl.state = ServerInformation.State.SHUTDOWN; synchronized (this) { for (Plugin p : running.values()) { - p.stop(); + unloadPlugin(p); } running.clear(); disabled.clear(); broken.clear(); - if (cleanupHandles.size() > running.size()) { + if (!toCleanup.isEmpty()) { System.gc(); processPendingCleanups(); } @@ -347,15 +358,14 @@ public class PluginLoader implements LifecycleListener { && oldPlugin.canReload() && newPlugin.canReload(); if (!reload && oldPlugin != null) { - oldPlugin.stop(); - running.remove(name); + unloadPlugin(oldPlugin); } if (!newPlugin.isDisabled()) { newPlugin.start(env); } if (reload) { env.onReloadPlugin(oldPlugin, newPlugin); - oldPlugin.stop(); + unloadPlugin(oldPlugin); } else if (!newPlugin.isDisabled()) { env.onStartPlugin(newPlugin); } @@ -380,8 +390,7 @@ public class PluginLoader implements LifecycleListener { } } for (String name : unload){ - log.info(String.format("Unloading plugin %s", name)); - running.remove(name).stop(); + unloadPlugin(running.get(name)); } } @@ -398,16 +407,19 @@ public class PluginLoader implements LifecycleListener { } synchronized int processPendingCleanups() { - CleanupHandle h; - while ((h = (CleanupHandle) cleanupQueue.poll()) != null) { - h.cleanup(); - cleanupHandles.remove(h); + Iterator iterator = toCleanup.iterator(); + while (iterator.hasNext()) { + Plugin plugin = iterator.next(); + iterator.remove(); + + CleanupHandle cleanupHandle = cleanupHandles.remove(plugin); + cleanupHandle.cleanup(); } - return Math.max(0, cleanupHandles.size() - running.size()); + return toCleanup.size(); } private void cleanInBackground() { - int cnt = Math.max(0, cleanupHandles.size() - running.size()); + int cnt = toCleanup.size(); if (0 < cnt) { cleaner.get().clean(cnt); } @@ -451,20 +463,17 @@ public class PluginLoader implements LifecycleListener { URL[] urls = {tmp.toURI().toURL()}; ClassLoader parentLoader = parentFor(type); ClassLoader pluginLoader = new URLClassLoader(urls, parentLoader); - final CleanupHandle cleanupHandle = - new CleanupHandle(tmp, jarFile, pluginLoader, cleanupQueue); - cleanupHandle.enqueue(); - cleanupHandles.put(cleanupHandle, Boolean.TRUE); - Class sysModule = load(sysName, pluginLoader); Class sshModule = load(sshName, pluginLoader); Class httpModule = load(httpName, pluginLoader); - keep = true; - return new Plugin(name, + Plugin plugin = new Plugin(name, srcJar, snapshot, jarFile, manifest, new File(dataDir, name), type, pluginLoader, sysModule, sshModule, httpModule); + cleanupHandles.put(plugin, new CleanupHandle(tmp, jarFile)); + keep = true; + return plugin; } finally { if (!keep) { jarFile.close();