Perform per-request cleanup actions at the end of a request

In both HTTP and SSH requests we now perform a custom list of cleanup
actions at the end of the request.  This permits a request scoped
provider to register a cleanup action for when the request is over,
like to close a database connection or a JGit repository handle.

Signed-off-by: Shawn O. Pearce <sop@google.com>

src/main/java/com/google/gerrit/server/RequestCleanup.java

@@ -0,0 +1,56 @@
+// Copyright (C) 2009 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;
+
+import com.google.inject.servlet.RequestScoped;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.List;
+
+
+/**
+ * Registers cleanup activities to be completed when a scope ends.
+ */
+@RequestScoped
+public class RequestCleanup implements Runnable {
+  private static final Logger log =
+      LoggerFactory.getLogger(RequestCleanup.class);
+
+  private final List<Runnable> cleanup = new LinkedList<Runnable>();
+
+  /** Register a task to be completed after the request ends. */
+  public void add(final Runnable task) {
+    synchronized (cleanup) {
+      cleanup.add(task);
+    }
+  }
+
+  public void run() {
+    synchronized (cleanup) {
+      for (final Iterator<Runnable> i = cleanup.iterator(); i.hasNext();) {
+        try {
+          i.next().run();
+        } catch (Throwable err) {
+          log.error("Failed to execute per-request cleanup", err);
+        }
+        i.remove();
+      }
+    }
+  }
+}

src/main/java/com/google/gerrit/server/config/GerritRequestModule.java

@@ -0,0 +1,28 @@
+// Copyright (C) 2009 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.config;
+
+import com.google.gerrit.client.reviewdb.ReviewDb;
+import com.google.gerrit.server.RequestCleanup;
+import com.google.inject.servlet.RequestScoped;
+
+/** Bindings for {@link RequestScoped} entities. */
+public class GerritRequestModule extends FactoryModule {
+  @Override
+  protected void configure() {
+    bind(RequestCleanup.class).in(RequestScoped.class);
+    bind(ReviewDb.class).toProvider(RequestScopedReviewDbProvider.class);
+  }
+}

src/main/java/com/google/gerrit/server/config/RequestScopedReviewDbProvider.java

@@ -0,0 +1,63 @@
+// Copyright (C) 2009 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.config;
+
+import com.google.gerrit.client.reviewdb.ReviewDb;
+import com.google.gerrit.server.RequestCleanup;
+import com.google.gwtorm.client.OrmException;
+import com.google.gwtorm.jdbc.Database;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.ProvisionException;
+import com.google.inject.Singleton;
+
+/** Provides {@link ReviewDb} database handle live only for this request. */
+@Singleton
+final class RequestScopedReviewDbProvider implements Provider<ReviewDb> {
+  private final Database<ReviewDb> schema;
+  private final Provider<RequestCleanup> cleanup;
+
+  @Inject
+  RequestScopedReviewDbProvider(final Database<ReviewDb> schema,
+      final Provider<RequestCleanup> cleanup) {
+    this.schema = schema;
+    this.cleanup = cleanup;
+  }
+
+  @Override
+  public ReviewDb get() {
+    final ReviewDb c;
+    try {
+      c = schema.open();
+    } catch (OrmException e) {
+      throw new ProvisionException("Cannot open ReviewDb", e);
+    }
+    try {
+      cleanup.get().add(new Runnable() {
+        @Override
+        public void run() {
+          c.close();
+        }
+      });
+      return c;
+    } catch (Error e) {
+      c.close();
+      throw e;
+    } catch (RuntimeException e) {
+      c.close();
+      throw e;
+    }
+  }
+}

src/main/java/com/google/gerrit/server/http/RequestCleanupFilter.java

@@ -0,0 +1,59 @@
+// Copyright (C) 2009 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.http;
+
+import com.google.gerrit.server.RequestCleanup;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+
+import java.io.IOException;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+
+/** Executes any pending {@link RequestCleanup} at the end of a request. */
+@Singleton
+class RequestCleanupFilter implements Filter {
+  private final Provider<RequestCleanup> cleanup;
+
+  @Inject
+  RequestCleanupFilter(final Provider<RequestCleanup> r) {
+    cleanup = r;
+  }
+
+  @Override
+  public void init(FilterConfig filterConfig) {
+  }
+
+  @Override
+  public void destroy() {
+  }
+
+  @Override
+  public void doFilter(final ServletRequest request,
+      final ServletResponse response, final FilterChain chain)
+      throws IOException, ServletException {
+    try {
+      chain.doFilter(request, response);
+    } finally {
+      cleanup.get().run();
+    }
+  }
+}

src/main/java/com/google/gerrit/server/http/UrlRewriteFilter.java

@@ -44,7 +44,7 @@ import javax.servlet.http.HttpServletResponse;
 
 /** Rewrites Gerrit 1 style URLs to Gerrit 2 style URLs. */
 @Singleton
-public class UrlRewriteFilter implements Filter {
+class UrlRewriteFilter implements Filter {
   private static final Pattern CHANGE_ID = Pattern.compile("^/(\\d+)/?$");
   private static final Pattern REV_ID =
       Pattern.compile("^/r/([0-9a-fA-F]{4," + RevId.LEN + "})/?$");

src/main/java/com/google/gerrit/server/http/WebModule.java

@@ -21,6 +21,7 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.RemotePeer;
 import com.google.gerrit.server.config.FactoryModule;
 import com.google.gerrit.server.config.GerritConfigProvider;
+import com.google.gerrit.server.config.GerritRequestModule;
 import com.google.gerrit.server.rpc.UiRpcModule;
 import com.google.gerrit.server.ssh.SshInfo;
 import com.google.gwtexpui.server.CacheControlFilter;
@@ -45,6 +46,7 @@ class WebModule extends FactoryModule {
     install(new ServletModule() {
       @Override
       protected void configureServlets() {
+        filter("/*").through(RequestCleanupFilter.class);
         filter("/*").through(UrlRewriteFilter.class);
 
         filter("/*").through(Key.get(CacheControlFilter.class));
@@ -59,6 +61,7 @@ class WebModule extends FactoryModule {
       }
     });
     install(new UiRpcModule());
+    install(new GerritRequestModule());
 
     bind(SshInfo.class).toProvider(sshInfoProvider);
     bind(GerritConfig.class).toProvider(GerritConfigProvider.class).in(

src/main/java/com/google/gerrit/server/ssh/BaseCommand.java

@@ -16,12 +16,13 @@ package com.google.gerrit.server.ssh;
 
 import com.google.gerrit.client.reviewdb.Account;
 import com.google.gerrit.pgm.CmdLineParser;
+import com.google.gerrit.server.RequestCleanup;
 import com.google.gerrit.server.ssh.SshScopes.Context;
+import com.google.inject.Inject;
 
 import org.apache.sshd.common.SshException;
 import org.apache.sshd.server.CommandFactory.Command;
 import org.apache.sshd.server.CommandFactory.ExitCallback;
-import org.apache.sshd.server.CommandFactory.SessionAware;
 import org.apache.sshd.server.session.ServerSession;
 import org.kohsuke.args4j.Argument;
 import org.kohsuke.args4j.CmdLineException;
@@ -36,7 +37,7 @@ import java.io.StringWriter;
 import java.util.ArrayList;
 import java.util.List;
 
-public abstract class BaseCommand implements Command, SessionAware {
+public abstract class BaseCommand implements Command {
   private static final Logger log = LoggerFactory.getLogger(BaseCommand.class);
 
   @Option(name = "--help", usage = "display this help text", aliases = {"-h"})
@@ -45,8 +46,11 @@ public abstract class BaseCommand implements Command, SessionAware {
   protected InputStream in;
   protected OutputStream out;
   protected OutputStream err;
-  protected ExitCallback exit;
-  protected ServerSession session;
+
+  private ExitCallback exit;
+
+  @Inject
+  private RequestCleanup cleanup;
 
   /** Text of the command line which lead up to invoking this instance. */
   protected String commandPrefix = "";
@@ -70,10 +74,6 @@ public abstract class BaseCommand implements Command, SessionAware {
     this.exit = callback;
   }
 
-  public void setSession(final ServerSession session) {
-    this.session = session;
-  }
-
   public void setCommandPrefix(final String prefix) {
     this.commandPrefix = prefix;
   }
@@ -101,9 +101,6 @@ public abstract class BaseCommand implements Command, SessionAware {
    * @param cmd the command that will receive the current state.
    */
   protected void provideStateTo(final Command cmd) {
-    if (cmd instanceof SessionAware) {
-      ((SessionAware) cmd).setSession(session);
-    }
     cmd.setInputStream(in);
     cmd.setOutputStream(out);
     cmd.setErrorStream(err);
@@ -222,15 +219,15 @@ public abstract class BaseCommand implements Command, SessionAware {
    */
   protected void startThread(final CommandRunnable thunk) {
     final Context context = SshScopes.getContext();
-    final List<Command> activeList = session.getAttribute(SshUtil.ACTIVE);
+    final List<Command> active = context.session.getAttribute(SshUtil.ACTIVE);
     final Command cmd = this;
     new Thread(threadName()) {
       @Override
       public void run() {
         int rc = 0;
         try {
-          synchronized (activeList) {
-            activeList.add(cmd);
+          synchronized (active) {
+            active.add(cmd);
           }
           SshScopes.current.set(context);
           thunk.run();
@@ -247,16 +244,31 @@ public abstract class BaseCommand implements Command, SessionAware {
           }
           rc = handleError(e);
         } finally {
-          synchronized (activeList) {
-            activeList.remove(cmd);
+          synchronized (active) {
+            active.remove(cmd);
           }
-          exit.onExit(rc);
+          onExit(rc);
         }
       }
     }.start();
   }
 
+  /**
+   * Terminate this command and return a result code to the remote client.
+   *<p>
+   * Commands should invoke this at most once. Once invoked, the command may
+   * lose access to request based resources as any callbacks previously
+   * registered with {@link RequestCleanup} will fire.
+   *
+   * @param rc exit code for the remote client.
+   */
+  protected void onExit(final int rc) {
+    exit.onExit(rc);
+    cleanup.run();
+  }
+
   private String threadName() {
+    final ServerSession session = SshScopes.getContext().session;
     final String who = session.getUsername();
     final Account.Id id = session.getAttribute(SshUtil.CURRENT_ACCOUNT);
     return "SSH " + getFullCommandLine() + " / " + who + " " + id;
@@ -283,6 +295,7 @@ public abstract class BaseCommand implements Command, SessionAware {
 
     if (e instanceof UnloggedFailure) {
     } else {
+      final ServerSession session = SshScopes.getContext().session;
       final StringBuilder m = new StringBuilder();
       m.append("Internal server error (");
       m.append("user ");

src/main/java/com/google/gerrit/server/ssh/CommandFactoryProvider.java

@@ -14,32 +14,87 @@
 
 package com.google.gerrit.server.ssh;
 
+import com.google.gerrit.server.ssh.SshScopes.Context;
 import com.google.inject.Inject;
-import com.google.inject.Injector;
 import com.google.inject.Provider;
 
 import org.apache.sshd.server.CommandFactory;
+import org.apache.sshd.server.CommandFactory.Command;
+import org.apache.sshd.server.CommandFactory.ExitCallback;
+import org.apache.sshd.server.CommandFactory.SessionAware;
+import org.apache.sshd.server.session.ServerSession;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
 
 /**
  * Creates a CommandFactory using commands registered by {@link CommandModule}.
  */
 class CommandFactoryProvider implements Provider<CommandFactory> {
-  private static final String SERVER = "Gerrit Code Review";
   private final DispatchCommandProvider dispatcher;
 
   @Inject
-  CommandFactoryProvider(final Injector i) {
-    dispatcher = new DispatchCommandProvider(i, SERVER, Commands.ROOT);
+  CommandFactoryProvider(
+      @CommandName(Commands.ROOT) final DispatchCommandProvider d) {
+    dispatcher = d;
   }
 
   @Override
   public CommandFactory get() {
     return new CommandFactory() {
       public Command createCommand(final String requestCommand) {
-        final DispatchCommand c = dispatcher.get();
-        c.setCommandLine(requestCommand);
-        return c;
+        return new Trampoline(requestCommand);
       }
     };
   }
+
+  private class Trampoline implements Command, SessionAware {
+    private final String commandLine;
+    private InputStream in;
+    private OutputStream out;
+    private OutputStream err;
+    private ExitCallback exit;
+    private ServerSession session;
+
+    Trampoline(final String cmdLine) {
+      commandLine = cmdLine;
+    }
+
+    public void setInputStream(final InputStream in) {
+      this.in = in;
+    }
+
+    public void setOutputStream(final OutputStream out) {
+      this.out = out;
+    }
+
+    public void setErrorStream(final OutputStream err) {
+      this.err = err;
+    }
+
+    public void setExitCallback(final ExitCallback callback) {
+      this.exit = callback;
+    }
+
+    public void setSession(final ServerSession session) {
+      this.session = session;
+    }
+
+    public void start() throws IOException {
+      final Context old = SshScopes.current.get();
+      try {
+        SshScopes.current.set(new Context(session));
+        final DispatchCommand c = dispatcher.get();
+        c.setCommandLine(commandLine);
+        c.setInputStream(in);
+        c.setOutputStream(out);
+        c.setErrorStream(err);
+        c.setExitCallback(exit);
+        c.start();
+      } finally {
+        SshScopes.current.set(old);
+      }
+    }
+  }
 }

src/main/java/com/google/gerrit/server/ssh/Commands.java

@@ -23,7 +23,10 @@ import java.lang.annotation.Annotation;
 /** Utilities to support {@link CommandName} construction. */
 public class Commands {
   /** Magic value signaling the top level. */
-  public static final CommandName ROOT = named("");
+  public static final String ROOT = "";
+
+  /** Magic value signaling the top level. */
+  public static final CommandName CMD_ROOT = named(ROOT);
 
   public static Key<CommandFactory.Command> key(final String name) {
     return key(named(name));
@@ -53,7 +56,8 @@ public class Commands {
 
       @Override
       public int hashCode() {
-        return value().hashCode();
+        // This is specified in java.lang.Annotation.
+        return (127 * "value".hashCode()) ^ value().hashCode();
       }
 
       @Override
@@ -64,7 +68,7 @@ public class Commands {
 
       @Override
       public String toString() {
-        return "CommandName[" + value() + "]";
+        return "@" + CommandName.class.getName() + "(value=" + value() + ")";
       }
     };
   }
@@ -87,7 +91,7 @@ public class Commands {
     if (name instanceof NestedCommandNameImpl) {
       return parent.equals(((NestedCommandNameImpl) name).parent);
     }
-    if (parent == ROOT) {
+    if (parent == CMD_ROOT) {
       return true;
     }
     return false;

src/main/java/com/google/gerrit/server/ssh/DispatchCommand.java

@@ -14,8 +14,9 @@
 
 package com.google.gerrit.server.ssh;
 
-import com.google.gerrit.server.ssh.SshScopes.Context;
+import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.google.inject.assistedinject.Assisted;
 
 import org.apache.sshd.server.CommandFactory.Command;
 
@@ -26,12 +27,17 @@ import java.util.Map;
 /**
  * Command that dispatches to a subcommand from its command table.
  */
-public class DispatchCommand extends BaseCommand {
+class DispatchCommand extends BaseCommand {
+  interface Factory {
+    DispatchCommand create(String prefix, Map<String, Provider<Command>> map);
+  }
+
   private final String prefix;
   private final Map<String, Provider<Command>> commands;
 
-  public DispatchCommand(final String pfx,
-      final Map<String, Provider<Command>> all) {
+  @Inject
+  DispatchCommand(@Assisted final String pfx,
+      @Assisted final Map<String, Provider<Command>> all) {
     prefix = pfx;
     commands = all;
   }
@@ -58,30 +64,22 @@ public class DispatchCommand extends BaseCommand {
 
     final Provider<Command> p = commands.get(name);
     if (p != null) {
-      final Context old = SshScopes.current.get();
-      try {
-        if (old == null) {
-          SshScopes.current.set(new Context(session));
-        }
-        final Command cmd = p.get();
-        provideStateTo(cmd);
-        if (cmd instanceof BaseCommand) {
-          final BaseCommand bc = (BaseCommand) cmd;
-          if (commandPrefix.isEmpty())
-            bc.setCommandPrefix(name);
-          else
-            bc.setCommandPrefix(commandPrefix + " " + name);
-          bc.setCommandLine(args);
-        }
-        cmd.start();
-      } finally {
-        SshScopes.current.set(old);
+      final Command cmd = p.get();
+      provideStateTo(cmd);
+      if (cmd instanceof BaseCommand) {
+        final BaseCommand bc = (BaseCommand) cmd;
+        if (commandPrefix.isEmpty())
+          bc.setCommandPrefix(name);
+        else
+          bc.setCommandPrefix(commandPrefix + " " + name);
+        bc.setCommandLine(args);
       }
+      cmd.start();
     } else {
       final String msg = prefix + ": " + name + ": not found\n";
       err.write(msg.getBytes("UTF-8"));
       err.flush();
-      exit.onExit(127);
+      onExit(127);
     }
   }
 
@@ -98,6 +96,6 @@ public class DispatchCommand extends BaseCommand {
     usage.append("\n");
     err.write(usage.toString().getBytes("UTF-8"));
     err.flush();
-    exit.onExit(1);
+    onExit(1);
   }
 }

src/main/java/com/google/gerrit/server/ssh/DispatchCommandProvider.java

@@ -36,6 +36,9 @@ public class DispatchCommandProvider implements Provider<DispatchCommand> {
   @Inject
   private Injector injector;
 
+  @Inject
+  private DispatchCommand.Factory factory;
+
   private final String dispatcherName;
   private final CommandName parent;
 
@@ -51,16 +54,9 @@ public class DispatchCommandProvider implements Provider<DispatchCommand> {
     this.parent = cn;
   }
 
-  public DispatchCommandProvider(final Injector i, final String dispatcherName,
-      final CommandName cn) {
-    this.injector = i;
-    this.dispatcherName = dispatcherName;
-    this.parent = cn;
-  }
-
   @Override
   public DispatchCommand get() {
-    return new DispatchCommand(dispatcherName, getMap());
+    return factory.create(dispatcherName, getMap());
   }
 
   private Map<String, Provider<Command>> getMap() {
@@ -83,7 +79,7 @@ public class DispatchCommandProvider implements Provider<DispatchCommand> {
       final Annotation annotation = b.getKey().getAnnotation();
       if (annotation instanceof CommandName) {
         final CommandName n = (CommandName) annotation;
-        if (Commands.isChild(parent, n)) {
+        if (!Commands.CMD_ROOT.equals(n) && Commands.isChild(parent, n)) {
           m.put(n.value(), (Provider<Command>) b.getProvider());
         }
       }

src/main/java/com/google/gerrit/server/ssh/SshDaemonModule.java

@@ -19,8 +19,10 @@ import static com.google.inject.Scopes.SINGLETON;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.RemotePeer;
+import com.google.gerrit.server.RequestCleanup;
+import com.google.gerrit.server.config.FactoryModule;
+import com.google.gerrit.server.config.GerritRequestModule;
 import com.google.gerrit.server.ssh.commands.DefaultCommandModule;
-import com.google.inject.AbstractModule;
 import com.google.inject.Provider;
 import com.google.inject.servlet.RequestScoped;
 import com.google.inject.servlet.SessionScoped;
@@ -29,14 +31,12 @@ import org.apache.sshd.common.session.AbstractSession;
 import org.apache.sshd.server.CommandFactory;
 import org.apache.sshd.server.PublickeyAuthenticator;
 import org.apache.sshd.server.session.ServerSession;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 import java.net.SocketAddress;
 
 /** Configures standard dependencies for {@link SshDaemon}. */
-public class SshDaemonModule extends AbstractModule {
-  static final Logger log = LoggerFactory.getLogger(SshDaemonModule.class);
+public class SshDaemonModule extends FactoryModule {
+  private static final String NAME = "Gerrit Code Review";
 
   @Override
   protected void configure() {
@@ -47,8 +47,13 @@ public class SshDaemonModule extends AbstractModule {
     configureRequestScope();
 
     bind(SshInfo.class).to(SshDaemon.class).in(SINGLETON);
+    factory(DispatchCommand.Factory.class);
+
+    bind(DispatchCommandProvider.class).annotatedWith(Commands.CMD_ROOT)
+        .toInstance(new DispatchCommandProvider(NAME, Commands.CMD_ROOT));
     bind(CommandFactoryProvider.class);
     bind(CommandFactory.class).toProvider(CommandFactoryProvider.class);
+
     bind(PublickeyAuthenticator.class).to(DatabasePubKeyAuth.class);
 
     install(new DefaultCommandModule());
@@ -74,6 +79,7 @@ public class SshDaemonModule extends AbstractModule {
   }
 
   private void configureRequestScope() {
+    install(new GerritRequestModule());
     bind(IdentifiedUser.class).toProvider(SshCurrentUserProvider.class).in(
         SshScopes.REQUEST);
     bind(CurrentUser.class).to(IdentifiedUser.class);