From 67f795262113eb9181554e021193de7ce0cd66e8 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 9 May 2012 14:14:56 -0700 Subject: [PATCH] Execute plugins from a copy in $site_path/tmp When starting a plugin its JAR is copied to a unique name in $site_path/tmp. This avoids race conditions with administrators installing a new version of a plugin while the JVM is still using it to handle user current activities. The temporary file copies are automatically deleted by the server only after the URLClassLoader that wraps it has been reclaimed by the JVM GC. By waiting until GC we can ensure the JAR exists on disk as long as running code might need to use it. Change-Id: I8e211592f8901bf44d66885409db4f7d6aaaaad3 --- .../gerrit/pgm/init/SitePathInitializer.java | 2 + .../gerrit/server/config/SitePaths.java | 2 + .../gerrit/server/plugins/CleanupHandle.java | 34 +++++ .../gerrit/server/plugins/PluginLoader.java | 117 ++++++++++++++---- .../server/plugins/PluginScannerThread.java | 2 +- .../sshd/commands/PluginReloadCommand.java | 2 +- 6 files changed, 130 insertions(+), 29 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/plugins/CleanupHandle.java diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/SitePathInitializer.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/SitePathInitializer.java index 32011397de..d26d46e971 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/SitePathInitializer.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/SitePathInitializer.java @@ -67,6 +67,7 @@ public class SitePathInitializer { mkdir(site.bin_dir); mkdir(site.etc_dir); mkdir(site.lib_dir); + mkdir(site.tmp_dir); mkdir(site.logs_dir); mkdir(site.mail_dir); mkdir(site.static_dir); @@ -85,6 +86,7 @@ public class SitePathInitializer { extract(site.gerrit_sh, Init.class, "gerrit.sh"); chmod(0755, site.gerrit_sh); + chmod(0700, site.tmp_dir); extractMailExample("Abandoned.vm"); extractMailExample("ChangeFooter.vm"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java index 9565831c0a..42054201cb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java @@ -28,6 +28,7 @@ public final class SitePaths { public final File bin_dir; public final File etc_dir; public final File lib_dir; + public final File tmp_dir; public final File logs_dir; public final File plugins_dir; public final File mail_dir; @@ -63,6 +64,7 @@ public final class SitePaths { bin_dir = new File(site_path, "bin"); etc_dir = new File(site_path, "etc"); lib_dir = new File(site_path, "lib"); + tmp_dir = new File(site_path, "tmp"); plugins_dir = new File(site_path, "plugins"); logs_dir = new File(site_path, "logs"); mail_dir = new File(etc_dir, "mail"); 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 new file mode 100644 index 0000000000..418dee903b --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/CleanupHandle.java @@ -0,0 +1,34 @@ +// 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 java.io.File; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; + +class CleanupHandle extends WeakReference { + private final File tmpFile; + + CleanupHandle(File jarFile, + ClassLoader ref, + ReferenceQueue queue) { + super(ref, queue); + this.tmpFile = jarFile; + } + + void cleanup() { + tmpFile.delete(); + } +} 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 13deca8067..ca5e349ee2 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 @@ -33,16 +33,21 @@ import org.slf4j.LoggerFactory; import java.io.File; import java.io.FileFilter; +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; import java.util.Arrays; import java.util.Collections; +import java.util.Date; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; import java.util.jar.Attributes; import java.util.jar.JarFile; @@ -53,9 +58,12 @@ public class PluginLoader implements LifecycleListener { static final Logger log = LoggerFactory.getLogger(PluginLoader.class); private final File pluginsDir; + private final File tmpDir; private final PluginGuiceEnvironment env; private final Map running; private final Map broken; + private final ReferenceQueue cleanupQueue; + private final ConcurrentMap cleanupHandles; private final PluginScannerThread scanner; @Inject @@ -63,9 +71,12 @@ public class PluginLoader implements LifecycleListener { PluginGuiceEnvironment pe, @GerritServerConfig Config cfg) { pluginsDir = sitePaths.plugins_dir; + tmpDir = sitePaths.tmp_dir; env = pe; running = Maps.newHashMap(); broken = Maps.newHashMap(); + cleanupQueue = new ReferenceQueue(); + cleanupHandles = Maps.newConcurrentMap(); scanner = new PluginScannerThread( this, ConfigUtil.getTimeUnit(cfg, @@ -87,8 +98,8 @@ public class PluginLoader implements LifecycleListener { name = nameOf(jar); File old = new File(pluginsDir, ".last_" + name + ".zip"); - File tmp = copyToTemp(name, in); - + File tmp = asTemp(in, ".next_" + name, ".zip", pluginsDir); + boolean clean = false; synchronized (this) { Plugin active = running.get(name); if (active != null) { @@ -97,21 +108,31 @@ public class PluginLoader implements LifecycleListener { jar.renameTo(old); } + new File(pluginsDir, name + ".jar.disabled").delete(); tmp.renameTo(jar); try { 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(); + } } - private File copyToTemp(String name, InputStream in) throws IOException { - File tmp = File.createTempFile(".next_" + name, ".zip", pluginsDir); + private static File asTemp(InputStream in, + String prefix, String suffix, + File dir) throws IOException { + File tmp = File.createTempFile(prefix, suffix, dir); boolean keep = false; try { FileOutputStream out = new FileOutputStream(tmp); @@ -133,26 +154,34 @@ public class PluginLoader implements LifecycleListener { } } - public synchronized void disablePlugins(Set names) { - for (String name : names) { - Plugin active = running.get(name); - if (active == null) { - continue; + public void disablePlugins(Set names) { + boolean clean = false; + synchronized (this) { + for (String name : names) { + Plugin active = running.get(name); + if (active == null) { + continue; + } + + log.info(String.format("Disabling plugin %s", name)); + File off = new File(pluginsDir, active.getName() + ".jar.disabled"); + active.getJar().renameTo(off); + + active.stop(); + running.remove(name); + clean = true; } - - log.info(String.format("Disabling plugin %s", name)); - active.stop(); - running.remove(name); - - File off = new File(pluginsDir, active.getName() + ".jar.disabled"); - active.getJar().renameTo(off); + } + if (clean) { + System.gc(); + processPendingCleanups(); } } @Override public synchronized void start() { log.info("Loading plugins from " + pluginsDir.getAbsolutePath()); - rescan(); + rescan(false); scanner.start(); } @@ -160,18 +189,29 @@ public class PluginLoader implements LifecycleListener { public void stop() { scanner.end(); synchronized (this) { + boolean clean = !running.isEmpty(); for (Plugin p : running.values()) { p.stop(); } running.clear(); broken.clear(); + if (clean) { + System.gc(); + processPendingCleanups(); + } } } - public synchronized void rescan() { - List jars = scanJarsInPluginsDirectory(); + public void rescan(boolean forceCleanup) { + if (rescanImp() || forceCleanup) { + System.gc(); + processPendingCleanups(); + } + } - stopRemovedPlugins(jars); + private synchronized boolean rescanImp() { + List jars = scanJarsInPluginsDirectory(); + boolean clean = stopRemovedPlugins(jars); for (File jar : jars) { String name = nameOf(jar); @@ -186,10 +226,6 @@ public class PluginLoader implements LifecycleListener { } if (active != null) { - log.warn(String.format( - "Detected %s was replaced/overwritten." - + " This is not a safe way to update a plugin.", - jar.getAbsolutePath())); log.info(String.format("Reloading plugin %s", name)); } @@ -197,11 +233,14 @@ 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; } private void runPlugin(String name, File jar, Plugin oldPlugin) @@ -231,7 +270,7 @@ public class PluginLoader implements LifecycleListener { } } - private void stopRemovedPlugins(List jars) { + private boolean stopRemovedPlugins(List jars) { Set unload = Sets.newHashSet(running.keySet()); for (File jar : jars) { unload.remove(nameOf(jar)); @@ -240,6 +279,15 @@ public class PluginLoader implements LifecycleListener { log.info(String.format("Unloading plugin %s", name)); running.remove(name).stop(); } + return !unload.isEmpty(); + } + + private synchronized void processPendingCleanups() { + CleanupHandle h; + while ((h = (CleanupHandle) cleanupQueue.poll()) != null) { + h.cleanup(); + cleanupHandles.remove(h); + } } private static String nameOf(File jar) { @@ -250,16 +298,26 @@ public class PluginLoader implements LifecycleListener { private Plugin loadPlugin(String name, File jarFile, FileSnapshot snapshot) throws IOException, ClassNotFoundException { - Manifest manifest = new JarFile(jarFile).getManifest(); + File tmp; + FileInputStream in = new FileInputStream(jarFile); + try { + tmp = asTemp(in, tempNameFor(name), ".jar", tmpDir); + } finally { + in.close(); + } + Manifest manifest = new JarFile(tmp).getManifest(); Attributes main = manifest.getMainAttributes(); String sysName = main.getValue("Gerrit-Module"); String sshName = main.getValue("Gerrit-SshModule"); String httpName = main.getValue("Gerrit-HttpModule"); - URL[] urls = {jarFile.toURI().toURL()}; + URL[] urls = {tmp.toURI().toURL()}; ClassLoader parentLoader = PluginLoader.class.getClassLoader(); ClassLoader pluginLoader = new URLClassLoader(urls, parentLoader); + cleanupHandles.put( + new CleanupHandle(tmp, pluginLoader, cleanupQueue), + Boolean.TRUE); Class sysModule = load(sysName, pluginLoader); Class sshModule = load(sshName, pluginLoader); @@ -268,6 +326,11 @@ public class PluginLoader implements LifecycleListener { sysModule, sshModule, httpModule); } + private static String tempNameFor(String name) { + SimpleDateFormat fmt = new SimpleDateFormat("yyMMdd_HHmm"); + return "plugin_" + name + "_" + fmt.format(new Date()) + "_"; + } + private Class load(String name, ClassLoader pluginLoader) throws ClassNotFoundException { if (Strings.isNullOrEmpty(name)) { 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 a484c5d44b..b2e3fedeb3 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(); + loader.rescan(false); } } 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 548669866b..4b76942ae0 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 @@ -27,6 +27,6 @@ final class PluginReloadCommand extends SshCommand { @Override protected void run() { - loader.rescan(); + loader.rescan(true); } }