opendev/gerrit
Code Issues Proposed changes

Fix deadlock on destroy of CommandFactoryProvider.

Browse Source 
A deadlock can occur when:
1) the CommandFactoryProvider is running in a thread from the
   startExecutor. It holds the this lock while running.
2) CommandFactoryProvider.destroy is called from the network thread,
   and blocks on the this lock. It also holds the network io lock.
3) The CommandFactoryProvider attempts to do io and blocks on the lock.

Fix the problem by offloading the destroy into thread in the
startExecutor.

Bug: Issue 1162
Change-Id: Iaa8658bd937a671ac0714c029ed6b7baa4637a4b
This commit is contained in:
Colby Ranger
2012-05-08 11:41:21 -07:00
committed by Sasa Zivkov
parent b2187baf0e
commit 89f91d09ab
1 changed files with 30 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java
View File

@@ -14,6 +14,8 @@
package com.google.gerrit.sshd;
import com.google.common.util.concurrent.Atomics;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.WorkQueue;
import com.google.gerrit.sshd.SshScope.Context;
@@ -36,7 +38,11 @@ import java.io.OutputStream;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
/**
* Creates a CommandFactory using commands registered by {@link CommandModule}.
@@ -47,7 +53,8 @@ class CommandFactoryProvider implements Provider<CommandFactory> {
private final DispatchCommandProvider dispatcher;
private final SshLog log;
private final Executor startExecutor;
private final ScheduledExecutorService startExecutor;
private final Executor destroyExecutor;
@Inject
CommandFactoryProvider(
@@ -59,6 +66,11 @@ class CommandFactoryProvider implements Provider<CommandFactory> {
int threads = cfg.getInt("sshd","commandStartThreads", 2);
startExecutor = workQueue.createQueue(threads, "SshCommandStart");
destroyExecutor = Executors.newSingleThreadExecutor(
new ThreadFactoryBuilder()
.setNameFormat("SshCommandDestroy-%s")
.setDaemon(true)
.build());
}
@Override
@@ -81,11 +93,13 @@ class CommandFactoryProvider implements Provider<CommandFactory> {
private Context ctx;
private DispatchCommand cmd;
private final AtomicBoolean logged;
private final AtomicReference<Future<?>> task;
Trampoline(final String cmdLine) {
commandLine = cmdLine;
argv = split(cmdLine);
logged = new AtomicBoolean();
task = Atomics.newReference();
}
public void setInputStream(final InputStream in) {
@@ -112,7 +126,7 @@ class CommandFactoryProvider implements Provider<CommandFactory> {
public void start(final Environment env) throws IOException {
this.env = env;
final Context ctx = this.ctx;
startExecutor.execute(new Runnable() {
task.set(startExecutor.submit(new Runnable() {
public void run() {
try {
onStart();
@@ -126,7 +140,7 @@ class CommandFactoryProvider implements Provider<CommandFactory> {
public String toString() {
return "start (user " + ctx.getSession().getUsername() + ")";
}
});
}));
}
private void onStart() throws IOException {
@@ -182,6 +196,19 @@ class CommandFactoryProvider implements Provider<CommandFactory> {
@Override
public void destroy() {
Future<?> future = task.getAndSet(null);
if (future != null) {
future.cancel(true);
destroyExecutor.execute(new Runnable() {
@Override
public void run() {
onDestroy();
}
});
}
}
private void onDestroy() {
synchronized (this) {
if (cmd != null) {
final Context old = SshScope.set(ctx);