Fix database connection leak in git-receive-pack

git receive-pack was leaking a database connection per execution
because we we created a new context when the command started, in
order to set the access path to GIT.  This meant that Guice setup
the RequestScoped ReviewDb instance inside of the new context,
but our thread management code ran cleanup inside of the old one.
So we never actually cleaned up that per-request connection.

When making a new context recursively in an SSH command, chain the
new context's RequestCleanup into the old context's RequestCleanup,
thereby ensuring we'll clean up both contexts when the command exits.

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

3 files changed, 33 insertions, 3 deletions

diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/AbstractGitCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/AbstractGitCommand.java
index b466bea745..99d2434849 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/AbstractGitCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/AbstractGitCommand.java
@@ -55,7 +55,7 @@ public abstract class AbstractGitCommand extends BaseCommand {
 
   @Override
   public void start(final Environment env) {
-    final Context ctx = new Context(newSession(), context.getCommandLine());
+    Context ctx = context.subContext(newSession(), context.getCommandLine());
     final Context old = SshScope.set(ctx);
     try {
       startThread(new CommandRunnable() {
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshScope.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshScope.java
index 247948cd40..2b89d07dda 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshScope.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshScope.java
@@ -14,6 +14,7 @@
 
 package com.google.gerrit.sshd;
 
+import com.google.gerrit.server.RequestCleanup;
 import com.google.inject.Key;
 import com.google.inject.OutOfScopeException;
 import com.google.inject.Provider;
@@ -25,6 +26,10 @@ import java.util.Map;
 /** Guice scopes for state during an SSH connection. */
 class SshScope {
   static class Context {
+    private static final Key<RequestCleanup> RC_KEY =
+        Key.get(RequestCleanup.class);
+
+    private final RequestCleanup cleanup;
     private final SshSession session;
     private final String commandLine;
     private final Map<Key<?>, Object> map;
@@ -34,9 +39,12 @@ class SshScope {
     volatile long finished;
 
     Context(final SshSession s, final String c) {
+      cleanup = new RequestCleanup();
       session = s;
       commandLine = c;
+
       map = new HashMap<Key<?>, Object>();
+      map.put(RC_KEY, cleanup);
 
       final long now = System.currentTimeMillis();
       created = now;
@@ -44,6 +52,21 @@ class SshScope {
       finished = now;
     }
 
+    private Context(Context p, SshSession s, String c) {
+      cleanup = new RequestCleanup();
+      session = s;
+      commandLine = c;
+
+      map = new HashMap<Key<?>, Object>();
+      map.put(RC_KEY, cleanup);
+
+      created = p.created;
+      started = p.started;
+      finished = p.finished;
+
+      p.cleanup.add(cleanup);
+    }
+
     String getCommandLine() {
       return commandLine;
     }
@@ -57,6 +80,10 @@ class SshScope {
       }
       return t;
     }
+
+    synchronized Context subContext(SshSession newSession, String newCommandLine) {
+      return new Context(this, newSession, newCommandLine);
+    }
   }
 
   static class ContextProvider implements Provider<Context> {
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SuExec.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SuExec.java
index 620e389bc5..45aaad5437 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SuExec.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SuExec.java
@@ -46,6 +46,7 @@ public final class SuExec extends BaseCommand {
   private Provider<CurrentUser> caller;
   private Provider<SshSession> session;
   private IdentifiedUser.GenericFactory userFactory;
+  private SshScope.Context callingContext;
 
   @Option(name = "--as", required = true)
   private Account.Id accountId;
@@ -61,11 +62,13 @@ public final class SuExec extends BaseCommand {
   @Inject
   SuExec(@CommandName(Commands.ROOT) final DispatchCommandProvider dispatcher,
       final Provider<CurrentUser> caller, final Provider<SshSession> session,
-      final IdentifiedUser.GenericFactory userFactory) {
+      final IdentifiedUser.GenericFactory userFactory,
+      final SshScope.Context callingContext) {
     this.dispatcher = dispatcher;
     this.caller = caller;
     this.session = session;
     this.userFactory = userFactory;
+    this.callingContext = callingContext;
   }
 
   @Override
@@ -76,7 +79,7 @@ public final class SuExec extends BaseCommand {
 
         parseCommandLine();
 
-        final Context ctx = new Context(newSession(), join(args));
+        final Context ctx = callingContext.subContext(newSession(), join(args));
         final Context old = SshScope.set(ctx);
         try {
           final BaseCommand cmd = dispatcher.get();