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

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);
   }
 }

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

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) {

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<StartPluginListener> listeners;
+  private final List<StartPluginListener> onStart;
+  private final List<ReloadPluginListener> 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<StartPluginListener>();
-    this.listeners.addAll(getListeners(sysInjector));
+
+    onStart = new CopyOnWriteArrayList<StartPluginListener>();
+    onStart.addAll(listeners(sysInjector, StartPluginListener.class));
+
+    onReload = new CopyOnWriteArrayList<ReloadPluginListener>();
+    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<StartPluginListener> getListeners(Injector src) {
-    List<Binding<StartPluginListener>> bindings =
-        src.findBindingsByType(new TypeLiteral<StartPluginListener>() {});
-    List<StartPluginListener> found =
-        Lists.newArrayListWithCapacity(bindings.size());
-    for (Binding<StartPluginListener> b : bindings) {
+  void onReloadPlugin(Plugin oldPlugin, Plugin newPlugin) {
+    for (ReloadPluginListener l : onReload) {
+      l.onReloadPlugin(oldPlugin, newPlugin);
+    }
+  }
+
+  private static <T> List<T> listeners(Injector src, Class<T> type) {
+    List<Binding<T>> bindings = src.findBindingsByType(TypeLiteral.get(type));
+    List<T> found = Lists.newArrayListWithCapacity(bindings.size());
+    for (Binding<T> b : bindings) {
       found.add(b.getProvider().get());
     }
     return found;

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();
 

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);
+}

gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommandProvider.java

@@ -72,6 +72,18 @@ public class DispatchCommandProvider implements Provider<DispatchCommand> {
     };
   }
 
+  public RegistrationHandle replace(final CommandName name,
+      final Provider<Command> cmd) {
+    final ConcurrentMap<String, Provider<Command>> m = getMap();
+    m.put(name.value(), cmd);
+    return new RegistrationHandle() {
+      @Override
+      public void remove() {
+        m.remove(name.value(), cmd);
+      }
+    };
+  }
+
   private ConcurrentMap<String, Provider<Command>> getMap() {
     if (map == null) {
       synchronized (this) {

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);
       }

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<Command> key = Commands.key(plugin.getName());
-      Provider<Command> 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<Command> cmd = load(plugin);
+    if (cmd != null) {
       plugin.add(root.register(Commands.named(plugin.getName()), cmd));
     }
   }
+
+  @Override
+  public void onReloadPlugin(Plugin oldPlugin, Plugin newPlugin) {
+    Provider<Command> cmd = load(newPlugin);
+    if (cmd != null) {
+      newPlugin.add(root.replace(Commands.named(newPlugin.getName()), cmd));
+    }
+  }
+
+  private Provider<Command> load(Plugin plugin) {
+    if (plugin.getSshInjector() != null) {
+      Key<Command> 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;
+  }
 }