From a576cc84793b1566975c78cf999f8610950acb36 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 9 May 2012 12:58:06 -0700 Subject: [PATCH] Atomic replace SSH and HTTP bindings during plugin reload When a plugin is reloaded with a new version, start the new one before stopping the old copy of the same plugin. This allows the SSH and HTTP bindings to atomically swap in the new plugin before dropping the old one, so users never see the command or URL with a not found error. Allow Gerrit-ReloadMode: restart to prevent this atomic replace, which may be necessary for plugins that acquire an exclusive lock on a resource in a LifecycleListener's start method, and release it during stop. When the ReloadMode is restart Gerrit will first stop the old copy of the plugin before starting the new one. This means there is a race condition when the command disappears and is later registered. Change-Id: Iec165991dfcc89f669e50a9fb21525282307f074 --- .../httpd/plugins/HttpPluginModule.java | 6 ++ .../httpd/plugins/HttpPluginServlet.java | 30 ++++++-- .../google/gerrit/server/plugins/Plugin.java | 17 ++++- .../plugins/PluginGuiceEnvironment.java | 43 ++++++----- .../gerrit/server/plugins/PluginLoader.java | 72 +++++++++++-------- .../server/plugins/ReloadPluginListener.java | 20 ++++++ .../gerrit/sshd/DispatchCommandProvider.java | 12 ++++ .../com/google/gerrit/sshd/SshModule.java | 7 ++ .../gerrit/sshd/SshPluginStarterCallback.java | 38 +++++++--- 9 files changed, 179 insertions(+), 66 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/plugins/ReloadPluginListener.java diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginModule.java index 1de330fc00..0ad90c2cda 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginModule.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginModule.java @@ -14,6 +14,7 @@ package com.google.gerrit.httpd.plugins; +import com.google.gerrit.server.plugins.ReloadPluginListener; import com.google.gerrit.server.plugins.StartPluginListener; import com.google.inject.internal.UniqueAnnotations; import com.google.inject.servlet.ServletModule; @@ -21,10 +22,15 @@ import com.google.inject.servlet.ServletModule; public class HttpPluginModule extends ServletModule { @Override protected void configureServlets() { + bind(HttpPluginServlet.class); serve("/plugins/*").with(HttpPluginServlet.class); bind(StartPluginListener.class) .annotatedWith(UniqueAnnotations.create()) .to(HttpPluginServlet.class); + + bind(ReloadPluginListener.class) + .annotatedWith(UniqueAnnotations.create()) + .to(HttpPluginServlet.class); } } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java index b73d6e6b1e..c3f78e6171 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java @@ -19,6 +19,7 @@ import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.gerrit.server.plugins.Plugin; import com.google.gerrit.server.plugins.RegistrationHandle; +import com.google.gerrit.server.plugins.ReloadPluginListener; import com.google.gerrit.server.plugins.StartPluginListener; import com.google.inject.Singleton; import com.google.inject.servlet.GuiceFilter; @@ -42,7 +43,7 @@ import javax.servlet.http.HttpServletResponse; @Singleton class HttpPluginServlet extends HttpServlet - implements StartPluginListener { + implements StartPluginListener, ReloadPluginListener { private static final long serialVersionUID = 1L; private static final Logger log = LoggerFactory.getLogger(HttpPluginServlet.class); @@ -59,7 +60,10 @@ class HttpPluginServlet extends HttpServlet String path = config.getServletContext().getContextPath(); base = Strings.nullToEmpty(path) + "/plugins/"; for (Plugin plugin : pending) { - start(plugin); + GuiceFilter filter = load(plugin); + if (filter != null) { + plugins.put(plugin.getName(), filter); + } } pending = null; } @@ -69,11 +73,22 @@ class HttpPluginServlet extends HttpServlet if (pending != null) { pending.add(plugin); } else { - start(plugin); + GuiceFilter filter = load(plugin); + if (filter != null) { + plugins.put(plugin.getName(), filter); + } } } - private void start(Plugin plugin) { + @Override + public void onReloadPlugin(Plugin oldPlugin, Plugin newPlugin) { + GuiceFilter filter = load(newPlugin); + if (filter != null) { + plugins.put(newPlugin.getName(), filter); + } + } + + private GuiceFilter load(Plugin plugin) { if (plugin.getHttpInjector() != null) { final String name = plugin.getName(); final GuiceFilter filter; @@ -81,7 +96,7 @@ class HttpPluginServlet extends HttpServlet filter = plugin.getHttpInjector().getInstance(GuiceFilter.class); } catch (RuntimeException e) { log.warn(String.format("Plugin %s cannot load GuiceFilter", name), e); - return; + return null; } try { @@ -89,7 +104,7 @@ class HttpPluginServlet extends HttpServlet filter.init(new WrappedFilterConfig(ctx)); } catch (ServletException e) { log.warn(String.format("Plugin %s failed to initialize HTTP", name), e); - return; + return null; } plugin.add(new RegistrationHandle() { @@ -102,8 +117,9 @@ class HttpPluginServlet extends HttpServlet } } }); - plugins.put(name, filter); + return filter; } + return null; } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/Plugin.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/Plugin.java index e9a63085c1..497b0f5141 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/Plugin.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/Plugin.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.plugins; +import com.google.common.base.Strings; import com.google.gerrit.lifecycle.LifecycleListener; import com.google.gerrit.lifecycle.LifecycleManager; import com.google.inject.AbstractModule; @@ -80,6 +81,21 @@ public class Plugin { return main.getValue(Attributes.Name.IMPLEMENTATION_VERSION); } + boolean canReload() { + Attributes main = manifest.getMainAttributes(); + String v = main.getValue("Gerrit-ReloadMode"); + if (Strings.isNullOrEmpty(v) || "reload".equalsIgnoreCase(v)) { + return true; + } else if ("restart".equalsIgnoreCase(v)) { + return false; + } else { + PluginLoader.log.warn(String.format( + "Plugin %s has invalid Gerrit-ReloadMode %s; assuming restart", + name, v)); + return false; + } + } + boolean isModified(File jar) { return snapshot.lastModified() != jar.lastModified(); } @@ -110,7 +126,6 @@ public class Plugin { } manager.start(); - env.onStartPlugin(this); } private Injector newRootInjector(PluginGuiceEnvironment env) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginGuiceEnvironment.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginGuiceEnvironment.java index 4b6f49710c..0e8a95debe 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginGuiceEnvironment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginGuiceEnvironment.java @@ -42,7 +42,8 @@ import javax.inject.Inject; public class PluginGuiceEnvironment { private final Injector sysInjector; private final CopyConfigModule copyConfigModule; - private final List listeners; + private final List onStart; + private final List onReload; private Module sysModule; private Module sshModule; private Module httpModule; @@ -51,8 +52,12 @@ public class PluginGuiceEnvironment { PluginGuiceEnvironment(Injector sysInjector, CopyConfigModule ccm) { this.sysInjector = sysInjector; this.copyConfigModule = ccm; - this.listeners = new CopyOnWriteArrayList(); - this.listeners.addAll(getListeners(sysInjector)); + + onStart = new CopyOnWriteArrayList(); + onStart.addAll(listeners(sysInjector, StartPluginListener.class)); + + onReload = new CopyOnWriteArrayList(); + onReload.addAll(listeners(sysInjector, ReloadPluginListener.class)); } Module getSysModule() { @@ -72,9 +77,10 @@ public class PluginGuiceEnvironment { }; } - public void setSshInjector(Injector sshInjector) { - sshModule = copy(sshInjector); - listeners.addAll(getListeners(sshInjector)); + public void setSshInjector(Injector injector) { + sshModule = copy(injector); + onStart.addAll(listeners(injector, StartPluginListener.class)); + onReload.addAll(listeners(injector, ReloadPluginListener.class)); } boolean hasSshModule() { @@ -85,9 +91,10 @@ public class PluginGuiceEnvironment { return sshModule; } - public void setHttpInjector(Injector httpInjector) { - httpModule = copy(httpInjector); - listeners.addAll(getListeners(httpInjector)); + public void setHttpInjector(Injector injector) { + httpModule = copy(injector); + onStart.addAll(listeners(injector, StartPluginListener.class)); + onReload.addAll(listeners(injector, ReloadPluginListener.class)); } boolean hasHttpModule() { @@ -99,17 +106,21 @@ public class PluginGuiceEnvironment { } void onStartPlugin(Plugin plugin) { - for (StartPluginListener l : listeners) { + for (StartPluginListener l : onStart) { l.onStartPlugin(plugin); } } - private static List getListeners(Injector src) { - List> bindings = - src.findBindingsByType(new TypeLiteral() {}); - List found = - Lists.newArrayListWithCapacity(bindings.size()); - for (Binding b : bindings) { + void onReloadPlugin(Plugin oldPlugin, Plugin newPlugin) { + for (ReloadPluginListener l : onReload) { + l.onReloadPlugin(oldPlugin, newPlugin); + } + } + + private static List listeners(Injector src, Class type) { + List> bindings = src.findBindingsByType(TypeLiteral.get(type)); + List found = Lists.newArrayListWithCapacity(bindings.size()); + for (Binding b : bindings) { found.add(b.getProvider().get()); } return found; 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 2ee6b04f81..13deca8067 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 @@ -50,7 +50,7 @@ import java.util.jar.Manifest; @Singleton public class PluginLoader implements LifecycleListener { - private static final Logger log = LoggerFactory.getLogger(PluginLoader.class); + static final Logger log = LoggerFactory.getLogger(PluginLoader.class); private final File pluginsDir; private final PluginGuiceEnvironment env; @@ -93,26 +93,19 @@ public class PluginLoader implements LifecycleListener { Plugin active = running.get(name); if (active != null) { log.info(String.format("Replacing plugin %s", name)); - active.stop(); - running.remove(name); old.delete(); jar.renameTo(old); } tmp.renameTo(jar); - FileSnapshot snapshot = FileSnapshot.save(jar); - Plugin next; try { - next = loadPlugin(name, snapshot, jar); - next.start(env); - } catch (Throwable err) { + runPlugin(name, jar, active); + if (active == null) { + log.info(String.format("Installed plugin %s", name)); + } + } catch (PluginInstallException e) { jar.delete(); - throw new PluginInstallException(err); - } - broken.remove(name); - running.put(name, next); - if (active == null) { - log.info(String.format("Installed plugin %s", name)); + throw e; } } } @@ -198,26 +191,43 @@ public class PluginLoader implements LifecycleListener { + " This is not a safe way to update a plugin.", jar.getAbsolutePath())); log.info(String.format("Reloading plugin %s", name)); - active.stop(); + } + + try { + runPlugin(name, jar, active); + if (active == null) { + log.info(String.format("Loaded plugin %s", name)); + } + } catch (PluginInstallException e) { + log.warn(String.format("Cannot load plugin %s", name), e.getCause()); + } + } + } + + private void runPlugin(String name, File jar, Plugin oldPlugin) + throws PluginInstallException { + FileSnapshot snapshot = FileSnapshot.save(jar); + try { + Plugin newPlugin = loadPlugin(name, jar, snapshot); + boolean reload = oldPlugin != null + && oldPlugin.canReload() + && newPlugin.canReload(); + if (!reload && oldPlugin != null) { + oldPlugin.stop(); running.remove(name); } - - FileSnapshot snapshot = FileSnapshot.save(jar); - Plugin next; - try { - next = loadPlugin(name, snapshot, jar); - next.start(env); - } catch (Throwable err) { - log.warn(String.format("Cannot load plugin %s", name), err); - broken.put(name, snapshot); - continue; + newPlugin.start(env); + if (reload) { + env.onReloadPlugin(oldPlugin, newPlugin); + oldPlugin.stop(); + } else { + env.onStartPlugin(newPlugin); } + running.put(name, newPlugin); broken.remove(name); - running.put(name, next); - - if (active == null) { - log.info(String.format("Loaded plugin %s", name)); - } + } catch (Throwable err) { + broken.put(name, snapshot); + throw new PluginInstallException(err); } } @@ -238,7 +248,7 @@ public class PluginLoader implements LifecycleListener { return 0 < ext ? name.substring(0, ext) : name; } - private Plugin loadPlugin(String name, FileSnapshot snapshot, File jarFile) + private Plugin loadPlugin(String name, File jarFile, FileSnapshot snapshot) throws IOException, ClassNotFoundException { Manifest manifest = new JarFile(jarFile).getManifest(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/ReloadPluginListener.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/ReloadPluginListener.java new file mode 100644 index 0000000000..72a499edb7 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/ReloadPluginListener.java @@ -0,0 +1,20 @@ +// 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; + +/** Broadcasts event indicating a plugin was reloaded. */ +public interface ReloadPluginListener { + public void onReloadPlugin(Plugin oldPlugin, Plugin newPlugin); +} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommandProvider.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommandProvider.java index 349cc7915b..3560d99e86 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommandProvider.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommandProvider.java @@ -72,6 +72,18 @@ public class DispatchCommandProvider implements Provider { }; } + public RegistrationHandle replace(final CommandName name, + final Provider cmd) { + final ConcurrentMap> m = getMap(); + m.put(name.value(), cmd); + return new RegistrationHandle() { + @Override + public void remove() { + m.remove(name.value(), cmd); + } + }; + } + private ConcurrentMap> getMap() { if (map == null) { synchronized (this) { diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java index a08f8423dd..284f2313e8 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java @@ -30,6 +30,7 @@ import com.google.gerrit.server.config.FactoryModule; import com.google.gerrit.server.config.GerritRequestModule; import com.google.gerrit.server.git.QueueProvider; import com.google.gerrit.server.git.WorkQueue; +import com.google.gerrit.server.plugins.ReloadPluginListener; import com.google.gerrit.server.plugins.StartPluginListener; import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.ssh.SshInfo; @@ -91,9 +92,15 @@ public class SshModule extends FactoryModule { install(new LifecycleModule() { @Override protected void configure() { + bind(SshPluginStarterCallback.class); bind(StartPluginListener.class) .annotatedWith(UniqueAnnotations.create()) .to(SshPluginStarterCallback.class); + + bind(ReloadPluginListener.class) + .annotatedWith(UniqueAnnotations.create()) + .to(SshPluginStarterCallback.class); + listener().to(SshLog.class); listener().to(SshDaemon.class); } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java index b82eb8f56f..4f9fe3319d 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java @@ -15,6 +15,7 @@ package com.google.gerrit.sshd; import com.google.gerrit.server.plugins.Plugin; +import com.google.gerrit.server.plugins.ReloadPluginListener; import com.google.gerrit.server.plugins.StartPluginListener; import com.google.inject.Key; import com.google.inject.Provider; @@ -27,7 +28,8 @@ import org.slf4j.LoggerFactory; import javax.inject.Inject; @Singleton -class SshPluginStarterCallback implements StartPluginListener { +class SshPluginStarterCallback + implements StartPluginListener, ReloadPluginListener { private static final Logger log = LoggerFactory .getLogger(SshPluginStarterCallback.class); @@ -41,17 +43,31 @@ class SshPluginStarterCallback implements StartPluginListener { @Override public void onStartPlugin(Plugin plugin) { - if (plugin.getSshInjector() != null) { - Key key = Commands.key(plugin.getName()); - Provider cmd; - try { - cmd = plugin.getSshInjector().getProvider(key); - } catch (RuntimeException err) { - log.warn(String.format("Plugin %s does not define command", - plugin.getName()), err); - return; - } + Provider cmd = load(plugin); + if (cmd != null) { plugin.add(root.register(Commands.named(plugin.getName()), cmd)); } } + + @Override + public void onReloadPlugin(Plugin oldPlugin, Plugin newPlugin) { + Provider cmd = load(newPlugin); + if (cmd != null) { + newPlugin.add(root.replace(Commands.named(newPlugin.getName()), cmd)); + } + } + + private Provider load(Plugin plugin) { + if (plugin.getSshInjector() != null) { + Key key = Commands.key(plugin.getName()); + try { + return plugin.getSshInjector().getProvider(key); + } catch (RuntimeException err) { + log.warn(String.format( + "Plugin %s did not define its top-level command", + plugin.getName()), err); + } + } + return null; + } }