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>
This commit is contained in:
@@ -55,7 +55,7 @@ public abstract class AbstractGitCommand extends BaseCommand {
|
|||||||
|
|
||||||
@Override
|
@Override
|
||||||
public void start(final Environment env) {
|
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);
|
final Context old = SshScope.set(ctx);
|
||||||
try {
|
try {
|
||||||
startThread(new CommandRunnable() {
|
startThread(new CommandRunnable() {
|
||||||
|
@@ -14,6 +14,7 @@
|
|||||||
|
|
||||||
package com.google.gerrit.sshd;
|
package com.google.gerrit.sshd;
|
||||||
|
|
||||||
|
import com.google.gerrit.server.RequestCleanup;
|
||||||
import com.google.inject.Key;
|
import com.google.inject.Key;
|
||||||
import com.google.inject.OutOfScopeException;
|
import com.google.inject.OutOfScopeException;
|
||||||
import com.google.inject.Provider;
|
import com.google.inject.Provider;
|
||||||
@@ -25,6 +26,10 @@ import java.util.Map;
|
|||||||
/** Guice scopes for state during an SSH connection. */
|
/** Guice scopes for state during an SSH connection. */
|
||||||
class SshScope {
|
class SshScope {
|
||||||
static class Context {
|
static class Context {
|
||||||
|
private static final Key<RequestCleanup> RC_KEY =
|
||||||
|
Key.get(RequestCleanup.class);
|
||||||
|
|
||||||
|
private final RequestCleanup cleanup;
|
||||||
private final SshSession session;
|
private final SshSession session;
|
||||||
private final String commandLine;
|
private final String commandLine;
|
||||||
private final Map<Key<?>, Object> map;
|
private final Map<Key<?>, Object> map;
|
||||||
@@ -34,9 +39,12 @@ class SshScope {
|
|||||||
volatile long finished;
|
volatile long finished;
|
||||||
|
|
||||||
Context(final SshSession s, final String c) {
|
Context(final SshSession s, final String c) {
|
||||||
|
cleanup = new RequestCleanup();
|
||||||
session = s;
|
session = s;
|
||||||
commandLine = c;
|
commandLine = c;
|
||||||
|
|
||||||
map = new HashMap<Key<?>, Object>();
|
map = new HashMap<Key<?>, Object>();
|
||||||
|
map.put(RC_KEY, cleanup);
|
||||||
|
|
||||||
final long now = System.currentTimeMillis();
|
final long now = System.currentTimeMillis();
|
||||||
created = now;
|
created = now;
|
||||||
@@ -44,6 +52,21 @@ class SshScope {
|
|||||||
finished = now;
|
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() {
|
String getCommandLine() {
|
||||||
return commandLine;
|
return commandLine;
|
||||||
}
|
}
|
||||||
@@ -57,6 +80,10 @@ class SshScope {
|
|||||||
}
|
}
|
||||||
return t;
|
return t;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
synchronized Context subContext(SshSession newSession, String newCommandLine) {
|
||||||
|
return new Context(this, newSession, newCommandLine);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
static class ContextProvider implements Provider<Context> {
|
static class ContextProvider implements Provider<Context> {
|
||||||
|
@@ -46,6 +46,7 @@ public final class SuExec extends BaseCommand {
|
|||||||
private Provider<CurrentUser> caller;
|
private Provider<CurrentUser> caller;
|
||||||
private Provider<SshSession> session;
|
private Provider<SshSession> session;
|
||||||
private IdentifiedUser.GenericFactory userFactory;
|
private IdentifiedUser.GenericFactory userFactory;
|
||||||
|
private SshScope.Context callingContext;
|
||||||
|
|
||||||
@Option(name = "--as", required = true)
|
@Option(name = "--as", required = true)
|
||||||
private Account.Id accountId;
|
private Account.Id accountId;
|
||||||
@@ -61,11 +62,13 @@ public final class SuExec extends BaseCommand {
|
|||||||
@Inject
|
@Inject
|
||||||
SuExec(@CommandName(Commands.ROOT) final DispatchCommandProvider dispatcher,
|
SuExec(@CommandName(Commands.ROOT) final DispatchCommandProvider dispatcher,
|
||||||
final Provider<CurrentUser> caller, final Provider<SshSession> session,
|
final Provider<CurrentUser> caller, final Provider<SshSession> session,
|
||||||
final IdentifiedUser.GenericFactory userFactory) {
|
final IdentifiedUser.GenericFactory userFactory,
|
||||||
|
final SshScope.Context callingContext) {
|
||||||
this.dispatcher = dispatcher;
|
this.dispatcher = dispatcher;
|
||||||
this.caller = caller;
|
this.caller = caller;
|
||||||
this.session = session;
|
this.session = session;
|
||||||
this.userFactory = userFactory;
|
this.userFactory = userFactory;
|
||||||
|
this.callingContext = callingContext;
|
||||||
}
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
@@ -76,7 +79,7 @@ public final class SuExec extends BaseCommand {
|
|||||||
|
|
||||||
parseCommandLine();
|
parseCommandLine();
|
||||||
|
|
||||||
final Context ctx = new Context(newSession(), join(args));
|
final Context ctx = callingContext.subContext(newSession(), join(args));
|
||||||
final Context old = SshScope.set(ctx);
|
final Context old = SshScope.set(ctx);
|
||||||
try {
|
try {
|
||||||
final BaseCommand cmd = dispatcher.get();
|
final BaseCommand cmd = dispatcher.get();
|
||||||
|
Reference in New Issue
Block a user