opendev/gerrit
Code Issues Proposed changes

Merge changes Ib00de070,I39ceef66

Browse Source 
* changes:
  Work around MyersDiff infinite loop in PatchListLoader
  Rework intra line diff to interrupt threads instead of killing them
This commit is contained in:
Shawn Pearce 2015-03-23 16:16:23 +00:00 committed by Gerrit Code Review
commit 4c4452abe5
10 changed files with 195 additions and 216 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
Documentation/config-gerrit.txt
View File

@ -720,14 +720,24 @@ See also link:cmd-flush-caches.html[gerrit flush-caches].
==== [[cache_options]]Cache Options
[[cache.diff_intraline.maxIdleWorkers]]cache.diff_intraline.maxIdleWorkers::
[[cache.diff.timeout]]cache.diff.timeout::
+
Number of idle worker threads to maintain for the intraline difference
computations. There is no upper bound on how many concurrent requests
can occur at once, if additional threads are started to handle a peak
load, only this many will remain idle afterwards.
Maximum number of milliseconds to wait for diff data before giving up and
falling back on a simpler diff algorithm that will not be able to break down
modified regions into smaller ones. This is a work around for an infinite loop
bug in the default difference algorithm implementation.
+
Default is 1.5x number of available CPUs.
Values should use common unit suffixes to express their setting:
+
* ms, milliseconds
* s, sec, second, seconds
* m, min, minute, minutes
* h, hr, hour, hours
+
If a unit suffix is not specified, `milliseconds` is assumed.
+
Default is 5 seconds.
[[cache.diff_intraline.timeout]]cache.diff_intraline.timeout::
+

4
gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java
View File

@ -64,7 +64,7 @@ import com.google.gerrit.server.index.IndexModule.IndexType;
import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier;
import com.google.gerrit.server.mail.SmtpEmailSender;
import com.google.gerrit.server.mime.MimeUtil2Module;
import com.google.gerrit.server.patch.IntraLineWorkerPool;
import com.google.gerrit.server.patch.DiffExecutorModule;
import com.google.gerrit.server.plugins.PluginGuiceEnvironment;
import com.google.gerrit.server.plugins.PluginRestApiModule;
import com.google.gerrit.server.schema.DataSourceProvider;
@ -317,7 +317,7 @@ public class Daemon extends SiteProgram {
modules.add(new WorkQueue.Module());
modules.add(new ChangeHookRunner.Module());
modules.add(new ReceiveCommitsExecutorModule());
modules.add(new IntraLineWorkerPool.Module());
modules.add(new DiffExecutorModule());
modules.add(new MimeUtil2Module());
modules.add(cfgInjector.getInstance(GerritGlobalModule.class));
modules.add(new InternalAccountDirectory.Module());

31
gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java Normal file
View File

@ -0,0 +1,31 @@
// Copyright (C) 2015 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.patch;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
import com.google.inject.BindingAnnotation;
import java.lang.annotation.Retention;
import java.util.concurrent.ExecutorService;
/**
* Marker on {@link ExecutorService} used by
* {@link IntraLineLoader} and {@link PatchListLoader}.
*/
@Retention(RUNTIME)
@BindingAnnotation
public @interface DiffExecutor {
}

39
gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutorModule.java Normal file
View File

@ -0,0 +1,39 @@
// Copyright (C) 2015 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.patch;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.inject.AbstractModule;
import com.google.inject.Provides;
import com.google.inject.Singleton;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
/** Module providing the {@link DiffExecutor}. */
public class DiffExecutorModule extends AbstractModule {
@Override
protected void configure() {
}
@Provides
@Singleton
@DiffExecutor
public ExecutorService createDiffExecutor() {
return Executors.newCachedThreadPool(new ThreadFactoryBuilder()
.setNameFormat("Diff-%d").setDaemon(true).build());
}
}

50
gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java
View File

@ -15,6 +15,7 @@
package com.google.gerrit.server.patch;
import com.google.common.base.Throwables;
import com.google.common.cache.CacheLoader;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.GerritServerConfig;
@ -29,7 +30,12 @@ import org.slf4j.LoggerFactory;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.regex.Pattern;
class IntraLineLoader extends CacheLoader<IntraLineDiffKey, IntraLineDiff> {
@ -41,12 +47,13 @@ class IntraLineLoader extends CacheLoader<IntraLineDiffKey, IntraLineDiff> {
private static final Pattern CONTROL_BLOCK_START_RE = Pattern
.compile("[{:][ \\t]*$");
private final IntraLineWorkerPool workerPool;
private final ExecutorService diffExecutor;
private final long timeoutMillis;
@Inject
IntraLineLoader(IntraLineWorkerPool pool, @GerritServerConfig Config cfg) {
workerPool = pool;
IntraLineLoader(@DiffExecutor ExecutorService diffExecutor,
@GerritServerConfig Config cfg) {
this.diffExecutor = diffExecutor;
timeoutMillis =
ConfigUtil.getTimeUnit(cfg, "cache", PatchListCacheImpl.INTRA_NAME,
"timeout", TimeUnit.MILLISECONDS.convert(5, TimeUnit.SECONDS),
@ -54,27 +61,30 @@ class IntraLineLoader extends CacheLoader<IntraLineDiffKey, IntraLineDiff> {
}
@Override
public IntraLineDiff load(IntraLineDiffKey key) throws Exception {
IntraLineWorkerPool.Worker w = workerPool.acquire();
IntraLineWorkerPool.Worker.Result r = w.computeWithTimeout(key, timeoutMillis);
if (r == IntraLineWorkerPool.Worker.Result.TIMEOUT) {
// Don't keep this thread. We have to murder it unsafely, which
// means its unable to be reused in the future. Return back a
// null result, indicating the cache cannot load this key.
//
public IntraLineDiff load(final IntraLineDiffKey key) throws Exception {
Future<IntraLineDiff> result = diffExecutor.submit(new Callable<IntraLineDiff>() {
@Override
public IntraLineDiff call() throws Exception {
return IntraLineLoader.compute(key);
}
});
try {
return result.get(timeoutMillis, TimeUnit.MILLISECONDS);
} catch (InterruptedException | TimeoutException e) {
log.warn(timeoutMillis + " ms timeout reached for IntraLineDiff"
+ " in project " + key.getProject().get()
+ " on commit " + key.getCommit().name()
+ " for path " + key.getPath()
+ " comparing " + key.getBlobA().name()
+ ".." + key.getBlobB().name());
result.cancel(true);
return new IntraLineDiff(IntraLineDiff.Status.TIMEOUT);
}
workerPool.release(w);
if (r.error != null) {
} catch (ExecutionException e) {
// If there was an error computing the result, carry it
// up to the caller so the cache knows this key is invalid.
//
throw r.error;
Throwables.propagateIfInstanceOf(e.getCause(), Exception.class);
throw new Exception(e.getMessage(), e.getCause());
}
return r.diff;
}
static IntraLineDiff compute(IntraLineDiffKey key) throws Exception {

182
gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineWorkerPool.java
View File

@ -1,182 +0,0 @@
// 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.patch;
import static com.google.gerrit.server.patch.IntraLineLoader.log;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import org.eclipse.jgit.lib.Config;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
@Singleton
public class IntraLineWorkerPool {
public static class Module extends AbstractModule {
@Override
protected void configure() {
bind(IntraLineWorkerPool.class);
}
}
private final BlockingQueue<Worker> workerPool;
@Inject
public IntraLineWorkerPool(@GerritServerConfig Config cfg) {
int workers = cfg.getInt(
"cache", PatchListCacheImpl.INTRA_NAME, "maxIdleWorkers",
Runtime.getRuntime().availableProcessors() * 3 / 2);
workerPool = new ArrayBlockingQueue<>(workers, true /* fair */);
}
Worker acquire() {
Worker w = workerPool.poll();
if (w == null) {
// If no worker is immediately available, start a new one.
// Maximum parallelism is controlled by the web server.
w = new Worker();
w.start();
}
return w;
}
void release(Worker w) {
if (!workerPool.offer(w)) {
// If the idle worker pool is full, terminate the worker.
w.shutdownGracefully();
}
}
static class Worker extends Thread {
private static final AtomicInteger count = new AtomicInteger(1);
private final ArrayBlockingQueue<Input> input;
private final ArrayBlockingQueue<Result> result;
Worker() {
input = new ArrayBlockingQueue<>(1);
result = new ArrayBlockingQueue<>(1);
setName("IntraLineDiff-" + count.getAndIncrement());
setDaemon(true);
}
Result computeWithTimeout(IntraLineDiffKey key, long timeoutMillis)
throws Exception {
if (!input.offer(new Input(key))) {
log.error("Cannot enqueue task to thread " + getName());
return Result.TIMEOUT;
}
Result r = result.poll(timeoutMillis, TimeUnit.MILLISECONDS);
if (r != null) {
return r;
} else {
log.warn(timeoutMillis + " ms timeout reached for IntraLineDiff"
+ " in project " + key.getProject().get()
+ " on commit " + key.getCommit().name()
+ " for path " + key.getPath()
+ " comparing " + key.getBlobA().name()
+ ".." + key.getBlobB().name()
+ ". Killing " + getName());
forcefullyKillThreadInAnUglyWay();
return Result.TIMEOUT;
}
}
@SuppressWarnings("deprecation")
private void forcefullyKillThreadInAnUglyWay() {
try {
stop();
} catch (Throwable error) {
// Ignore any reason the thread won't stop.
log.error("Cannot stop runaway thread " + getName(), error);
}
}
private void shutdownGracefully() {
if (!input.offer(Input.END_THREAD)) {
log.error("Cannot gracefully stop thread " + getName());
}
}
@Override
public void run() {
try {
for (;;) {
Input in;
try {
in = input.take();
} catch (InterruptedException e) {
log.error("Unexpected interrupt on " + getName());
continue;
}
if (in == Input.END_THREAD) {
return;
}
Result r;
try {
r = new Result(IntraLineLoader.compute(in.key));
} catch (Exception error) {
r = new Result(error);
}
if (!result.offer(r)) {
log.error("Cannot return result from " + getName());
}
}
} catch (ThreadDeath iHaveBeenShot) {
// Handle thread death by gracefully returning to the caller,
// allowing the thread to be destroyed.
}
}
private static class Input {
static final Input END_THREAD = new Input(null);
final IntraLineDiffKey key;
Input(IntraLineDiffKey key) {
this.key = key;
}
}
static class Result {
static final Result TIMEOUT = new Result((IntraLineDiff) null);
final IntraLineDiff diff;
final Exception error;
Result(IntraLineDiff diff) {
this.diff = diff;
this.error = null;
}
Result(Exception error) {
this.diff = null;
this.error = error;
}
}
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
View File

@ -35,7 +35,7 @@ import java.util.concurrent.ExecutionException;
/** Provides a cached list of {@link PatchListEntry}. */
@Singleton
public class PatchListCacheImpl implements PatchListCache {
private static final String FILE_NAME = "diff";
static final String FILE_NAME = "diff";
static final String INTRA_NAME = "diff_intraline";
public static Module module() {

62
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java
View File

@ -16,11 +16,13 @@
package com.google.gerrit.server.patch;
import com.google.common.base.Function;
import com.google.common.base.Throwables;
import com.google.common.cache.CacheLoader;
import com.google.common.collect.FluentIterable;
import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace;
import com.google.gerrit.reviewdb.client.Patch;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil;
@ -69,6 +71,12 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
public class PatchListLoader extends CacheLoader<PatchListKey, PatchList> {
static final Logger log = LoggerFactory.getLogger(PatchListLoader.class);
@ -76,13 +84,23 @@ public class PatchListLoader extends CacheLoader<PatchListKey, PatchList> {
private final GitRepositoryManager repoManager;
private final PatchListCache patchListCache;
private final ThreeWayMergeStrategy mergeStrategy;
private final ExecutorService diffExecutor;
private final long timeoutMillis;
@Inject
PatchListLoader(GitRepositoryManager mgr, PatchListCache plc,
@GerritServerConfig Config cfg) {
PatchListLoader(GitRepositoryManager mgr,
PatchListCache plc,
@GerritServerConfig Config cfg,
@DiffExecutor ExecutorService de) {
repoManager = mgr;
patchListCache = plc;
mergeStrategy = MergeUtil.getMergeStrategy(cfg);
diffExecutor = de;
timeoutMillis =
ConfigUtil.getTimeUnit(cfg, "cache", PatchListCacheImpl.FILE_NAME,
"timeout", TimeUnit.MILLISECONDS.convert(5, TimeUnit.SECONDS),
TimeUnit.MILLISECONDS);
}
@Override
@ -163,7 +181,7 @@ public class PatchListLoader extends CacheLoader<PatchListKey, PatchList> {
DiffEntry diffEntry = diffEntries.get(i);
if (paths == null || paths.contains(diffEntry.getNewPath())
|| paths.contains(diffEntry.getOldPath())) {
FileHeader fh = df.toFileHeader(diffEntry);
FileHeader fh = toFileHeader(key, df, diffEntry);
entries.add(newEntry(aTree, fh));
}
}
@ -172,6 +190,44 @@ public class PatchListLoader extends CacheLoader<PatchListKey, PatchList> {
}
}
private FileHeader toFileHeader(PatchListKey key,
final DiffFormatter diffFormatter, final DiffEntry diffEntry)
throws IOException {
Future<FileHeader> result = diffExecutor.submit(new Callable<FileHeader>() {
@Override
public FileHeader call() throws IOException {
return diffFormatter.toFileHeader(diffEntry);
}
});
try {
return result.get(timeoutMillis, TimeUnit.MILLISECONDS);
} catch (InterruptedException | TimeoutException e) {
log.warn(timeoutMillis + " ms timeout reached for Diff loader"
+ " in project " + key.projectKey.get()
+ " on commit " + key.getNewId()
+ " on path " + diffEntry.getNewPath()
+ " comparing " + diffEntry.getOldId()
+ ".." + diffEntry.getNewId());
result.cancel(true);
return toFileHeaderWithoutMyersDiff(diffFormatter, diffEntry);
} catch (ExecutionException e) {
// If there was an error computing the result, carry it
// up to the caller so the cache knows this key is invalid.
Throwables.propagateIfInstanceOf(e.getCause(), IOException.class);
throw new IOException(e.getMessage(), e.getCause());
}
}
private FileHeader toFileHeaderWithoutMyersDiff(DiffFormatter diffFormatter,
DiffEntry diffEntry) throws IOException {
HistogramDiff histogramDiff = new HistogramDiff();
histogramDiff.setFallbackAlgorithm(null);
diffFormatter.setDiffAlgorithm(histogramDiff);
return diffFormatter.toFileHeader(diffEntry);
}
private PatchListEntry newCommitMessage(final RawTextComparator cmp,
final ObjectReader reader,
final RevCommit aCommit, final RevCommit bCommit) throws IOException {

15
gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java
View File

@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState;
import static com.google.inject.Scopes.SINGLETON;
import com.google.common.net.InetAddresses;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.DisabledChangeHooks;
import com.google.gerrit.reviewdb.client.AuthType;
@ -48,6 +49,7 @@ import com.google.gerrit.server.index.ChangeSchemas;
import com.google.gerrit.server.index.IndexModule.IndexType;
import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier;
import com.google.gerrit.server.mail.SmtpEmailSender;
import com.google.gerrit.server.patch.DiffExecutor;
import com.google.gerrit.server.schema.DataSourceType;
import com.google.gerrit.server.schema.SchemaCreator;
import com.google.gerrit.server.securestore.DefaultSecureStore;
@ -75,6 +77,7 @@ import java.net.InetSocketAddress;
import java.net.SocketAddress;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.concurrent.ExecutorService;
public class InMemoryModule extends FactoryModule {
public static Config newDefaultConfig() {
@ -162,6 +165,18 @@ public class InMemoryModule extends FactoryModule {
return CanonicalWebUrlProvider.class;
}
});
//Replacement of DiffExecutorModule to not use thread pool in the tests
install(new AbstractModule() {
@Override
protected void configure() {
}
@Provides
@Singleton
@DiffExecutor
public ExecutorService createDiffExecutor() {
return MoreExecutors.newDirectExecutorService();
}
});
install(new DefaultCacheFactory.Module());
install(new SmtpEmailSender.Module());
install(new SignedTokenEmailTokenVerifier.Module());

4
gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java
View File

@ -47,7 +47,7 @@ import com.google.gerrit.server.index.IndexModule;
import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier;
import com.google.gerrit.server.mail.SmtpEmailSender;
import com.google.gerrit.server.mime.MimeUtil2Module;
import com.google.gerrit.server.patch.IntraLineWorkerPool;
import com.google.gerrit.server.patch.DiffExecutorModule;
import com.google.gerrit.server.plugins.PluginGuiceEnvironment;
import com.google.gerrit.server.plugins.PluginRestApiModule;
import com.google.gerrit.server.schema.DataSourceModule;
@ -281,7 +281,7 @@ public class WebAppInitializer extends GuiceServletContextListener
modules.add(new WorkQueue.Module());
modules.add(new ChangeHookRunner.Module());
modules.add(new ReceiveCommitsExecutorModule());
modules.add(new IntraLineWorkerPool.Module());
modules.add(new DiffExecutorModule());
modules.add(new MimeUtil2Module());
modules.add(cfgInjector.getInstance(GerritGlobalModule.class));
modules.add(new InternalAccountDirectory.Module());