From d60c8331143788f1ae18955d6a93d184bd1a2a3f Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Wed, 10 Sep 2014 11:02:59 +0200 Subject: [PATCH 1/6] Allow plugins to use self-provided licenses for used Maven Jars maven_jar only allowed to use licenses that were defined in '//lib:'. But plugins may need further licenses that are not yet in core (E.g.: plugins/its-jira relies on a jar with CPL1.0 license). Since, plugins cannot add rules underneath '//lib:', we add the local_license parameter to maven_jar, which allows plugins to use licenses from ':' instead of '//lib:'. Change-Id: Icc40c413eed65a0b6e3003021050d91086ca4c34 --- lib/maven.defs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/maven.defs b/lib/maven.defs index 5f4006fe27..23708ca249 100644 --- a/lib/maven.defs +++ b/lib/maven.defs @@ -40,7 +40,8 @@ def maven_jar( sha1 = '', bin_sha1 = '', src_sha1 = '', repository = MAVEN_CENTRAL, attach_source = True, - visibility = ['PUBLIC']): + visibility = ['PUBLIC'], + local_license = False): from os import path parts = id.split(':') @@ -89,7 +90,10 @@ def maven_jar( cmd = ' '.join(cmd), out = binjar, ) - license = ['//lib:LICENSE-' + license] + license = ':LICENSE-' + license + if not local_license: + license = '//lib' + license + license = [license] if src_sha1 or attach_source: cmd = ['$(exe //tools:download_file)', '-o', '$OUT', '-u', srcurl] From a4e0250cbdf9344035bcf9168b59e8e3d52ddbb1 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Tue, 26 Aug 2014 15:16:51 -0700 Subject: [PATCH 2/6] Allow PatchListLoader to use recursive merger The automerge "base" created on merge commits should use the recursive merger, if it has not been disabled on this server. Switching to recursive allows handling changes that require the virtual common ancestor to be built when multiple merge bases are discovered for the two parents. Change-Id: Ica2daa6bd0a38f7fa1430bed28bd7f54d3bfb2c4 --- .../google/gerrit/server/git/MergeUtil.java | 7 +++ .../server/index/ChangeBatchIndexer.java | 36 ++++++++++------ .../gerrit/server/patch/PatchListLoader.java | 43 +++++++++++-------- 3 files changed, 56 insertions(+), 30 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java index b35d7e478f..709a8ac9dd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java @@ -52,6 +52,7 @@ import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.Merger; +import org.eclipse.jgit.merge.ThreeWayMergeStrategy; import org.eclipse.jgit.merge.ThreeWayMerger; import org.eclipse.jgit.revwalk.FooterKey; import org.eclipse.jgit.revwalk.FooterLine; @@ -89,6 +90,12 @@ public class MergeUtil { return cfg.getBoolean("core", null, "useRecursiveMerge", true); } + public static ThreeWayMergeStrategy getMergeStrategy(Config cfg) { + return useRecursiveMerge(cfg) + ? MergeStrategy.RECURSIVE + : MergeStrategy.RESOLVE; + } + public static interface Factory { MergeUtil create(ProjectState project); MergeUtil create(ProjectState project, boolean useContentMerge); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java index 5ee240b8a6..b2ce8f3859 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java @@ -32,7 +32,9 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.change.MergeabilityChecker; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.git.MultiProgressMonitor; import com.google.gerrit.server.git.MultiProgressMonitor.Task; import com.google.gerrit.server.patch.PatchListLoader; @@ -43,6 +45,7 @@ import com.google.inject.Inject; import org.eclipse.jgit.diff.DiffEntry; import org.eclipse.jgit.diff.DiffFormatter; import org.eclipse.jgit.errors.RepositoryNotFoundException; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; @@ -50,6 +53,7 @@ import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.merge.ThreeWayMergeStrategy; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevObject; import org.eclipse.jgit.revwalk.RevTree; @@ -112,6 +116,7 @@ public class ChangeBatchIndexer { private final ListeningExecutorService executor; private final ChangeIndexer.Factory indexerFactory; private final MergeabilityChecker mergeabilityChecker; + private final ThreeWayMergeStrategy mergeStrategy; @Inject ChangeBatchIndexer(SchemaFactory schemaFactory, @@ -119,6 +124,7 @@ public class ChangeBatchIndexer { GitRepositoryManager repoManager, @IndexExecutor ListeningExecutorService executor, ChangeIndexer.Factory indexerFactory, + @GerritServerConfig Config config, @Nullable MergeabilityChecker mergeabilityChecker) { this.schemaFactory = schemaFactory; this.changeDataFactory = changeDataFactory; @@ -126,6 +132,7 @@ public class ChangeBatchIndexer { this.executor = executor; this.indexerFactory = indexerFactory; this.mergeabilityChecker = mergeabilityChecker; + this.mergeStrategy = MergeUtil.getMergeStrategy(config); } public Result indexAll(ChangeIndex index, Iterable projects, @@ -239,8 +246,13 @@ public class ChangeBatchIndexer { byId.put(r.getObjectId(), changeDataFactory.create(db, c)); } } - new ProjectIndexer(indexer, byId, repo, done, failed, verboseWriter) - .call(); + new ProjectIndexer(indexer, + mergeStrategy, + byId, + repo, + done, + failed, + verboseWriter).call(); } catch (RepositoryNotFoundException rnfe) { log.error(rnfe.getMessage()); } finally { @@ -261,6 +273,7 @@ public class ChangeBatchIndexer { public static class ProjectIndexer implements Callable { private final ChangeIndexer indexer; + private final ThreeWayMergeStrategy mergeStrategy; private final Multimap byId; private final ProgressMonitor done; private final ProgressMonitor failed; @@ -268,16 +281,15 @@ public class ChangeBatchIndexer { private final Repository repo; private RevWalk walk; - public ProjectIndexer(ChangeIndexer indexer, - Multimap changesByCommitId, Repository repo) { - this(indexer, changesByCommitId, repo, - NullProgressMonitor.INSTANCE, NullProgressMonitor.INSTANCE, null); - } - - ProjectIndexer(ChangeIndexer indexer, - Multimap changesByCommitId, Repository repo, - ProgressMonitor done, ProgressMonitor failed, PrintWriter verboseWriter) { + private ProjectIndexer(ChangeIndexer indexer, + ThreeWayMergeStrategy mergeStrategy, + Multimap changesByCommitId, + Repository repo, + ProgressMonitor done, + ProgressMonitor failed, + PrintWriter verboseWriter) { this.indexer = indexer; + this.mergeStrategy = mergeStrategy; this.byId = changesByCommitId; this.repo = repo; this.done = done; @@ -376,7 +388,7 @@ public class ChangeBatchIndexer { walk.parseBody(a); return walk.parseTree(a.getTree()); case 2: - return PatchListLoader.automerge(repo, walk, b); + return PatchListLoader.automerge(repo, walk, b, mergeStrategy); default: return null; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java index 4eede003ba..de36020d85 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java @@ -21,7 +21,9 @@ 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.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.MergeUtil; import com.google.inject.Inject; import org.eclipse.jgit.diff.DiffEntry; @@ -35,6 +37,7 @@ import org.eclipse.jgit.diff.Sequence; import org.eclipse.jgit.dircache.DirCache; import org.eclipse.jgit.dircache.DirCacheBuilder; import org.eclipse.jgit.dircache.DirCacheEntry; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.FileMode; import org.eclipse.jgit.lib.ObjectId; @@ -45,8 +48,8 @@ import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.merge.MergeFormatter; import org.eclipse.jgit.merge.MergeResult; -import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.ResolveMerger; +import org.eclipse.jgit.merge.ThreeWayMergeStrategy; import org.eclipse.jgit.patch.FileHeader; import org.eclipse.jgit.patch.FileHeader.PatchType; import org.eclipse.jgit.revwalk.RevCommit; @@ -72,11 +75,14 @@ public class PatchListLoader extends CacheLoader { private final GitRepositoryManager repoManager; private final PatchListCache patchListCache; + private final ThreeWayMergeStrategy mergeStrategy; @Inject - PatchListLoader(GitRepositoryManager mgr, PatchListCache plc) { + PatchListLoader(GitRepositoryManager mgr, PatchListCache plc, + @GerritServerConfig Config cfg) { repoManager = mgr; patchListCache = plc; + mergeStrategy = MergeUtil.getMergeStrategy(cfg); } @Override @@ -224,7 +230,7 @@ public class PatchListLoader extends CacheLoader { } } - private static RevObject aFor(final PatchListKey key, + private RevObject aFor(final PatchListKey key, final Repository repo, final RevWalk rw, final RevCommit b) throws IOException { if (key.getOldId() != null) { @@ -240,20 +246,20 @@ public class PatchListLoader extends CacheLoader { return r; } case 2: - return automerge(repo, rw, b); + return automerge(repo, rw, b, mergeStrategy); default: // TODO(sop) handle an octopus merge. return null; } } - public static RevTree automerge(Repository repo, RevWalk rw, RevCommit b) - throws IOException { - return automerge(repo, rw, b, true); + public static RevTree automerge(Repository repo, RevWalk rw, RevCommit b, + ThreeWayMergeStrategy mergeStrategy) throws IOException { + return automerge(repo, rw, b, mergeStrategy, true); } public static RevTree automerge(Repository repo, RevWalk rw, RevCommit b, - boolean save) throws IOException { + ThreeWayMergeStrategy mergeStrategy, boolean save) throws IOException { String hash = b.name(); String refName = RefNames.REFS_CACHE_AUTOMERGE + hash.substring(0, 2) @@ -264,8 +270,7 @@ public class PatchListLoader extends CacheLoader { return rw.parseTree(ref.getObjectId()); } - ObjectId treeId; - ResolveMerger m = (ResolveMerger) MergeStrategy.RESOLVE.newMerger(repo, true); + ResolveMerger m = (ResolveMerger) mergeStrategy.newMerger(repo, true); final ObjectInserter ins = repo.newObjectInserter(); try { DirCache dc = DirCache.newInCore(); @@ -297,6 +302,7 @@ public class PatchListLoader extends CacheLoader { return null; } + ObjectId treeId; if (couldMerge) { treeId = m.getResultTreeId(); @@ -381,17 +387,18 @@ public class PatchListLoader extends CacheLoader { treeId = dc.writeTree(ins); } ins.flush(); + + if (save) { + RefUpdate update = repo.updateRef(refName); + update.setNewObjectId(treeId); + update.disableRefLog(); + update.forceUpdate(); + } + + return rw.lookupTree(treeId); } finally { ins.release(); } - - if (save) { - RefUpdate update = repo.updateRef(refName); - update.setNewObjectId(treeId); - update.disableRefLog(); - update.forceUpdate(); - } - return rw.parseTree(treeId); } private static ObjectId emptyTree(final Repository repo) throws IOException { From 16133e6e84edec601a10b252dcbf0ebc28f3529f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Fri, 20 Mar 2015 11:06:54 +0100 Subject: [PATCH 3/6] Workaround a RecursiveMerger bug [1], avoid online reindexing failure When performing a mergeability check for two commits which don't have a common parent the RecursiveMerger will fail with a NPE. This prevented the OnlineReindexer from finishing Lucene index upgrade. This issue was also discussed in [2]. Workaround: consider any exception from the RecursiveMerger as an expected error and return false as the result of the mergeability check. Also, never fail the online reindexing when the mergeability check is not able to finish properly. [1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=462671 [2] https://groups.google.com/d/topic/repo-discuss/REYuk4mgIw8/discussion Change-Id: Iea88440a9e25edf9385aba0663779d30e622e4f8 --- .../com/google/gerrit/server/index/ChangeBatchIndexer.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java index b2ce8f3859..be87ea0b68 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java @@ -157,9 +157,7 @@ public class ChangeBatchIndexer { final AtomicBoolean ok = new AtomicBoolean(true); for (final Project.NameKey project : projects) { - if (!updateMergeable(project)) { - ok.set(false); - } + updateMergeable(project); final ListenableFuture future = executor.submit(reindexProject( indexerFactory.create(index), project, doneTask, failedTask, verboseWriter)); @@ -219,7 +217,7 @@ public class ChangeBatchIndexer { if (mergeabilityChecker != null) { try { mergeabilityChecker.newCheck().addProject(project).run(); - } catch (IOException e) { + } catch (Exception e) { log.error("Error in mergeability checker", e); return false; } From cffa508a9161e8680b5272da79c43655ab53a2c1 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 18 Mar 2015 14:36:19 -0700 Subject: [PATCH 4/6] Update JGit to 3.7.0.201502260915-r.58-g65c379e Change-Id: Ib4b994c10d83ffc30e478428545dfb0d1be97dfe --- lib/jgit/BUCK | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/jgit/BUCK b/lib/jgit/BUCK index 2d0af1b174..a658fc36e7 100644 --- a/lib/jgit/BUCK +++ b/lib/jgit/BUCK @@ -1,13 +1,15 @@ include_defs('//lib/maven.defs') -VERS = '3.6.2.201501210735-r' +REPO = GERRIT # Leave here even if set to MAVEN_CENTRAL. +VERS = '3.7.0.201502260915-r.58-g65c379e' maven_jar( name = 'jgit', id = 'org.eclipse.jgit:org.eclipse.jgit:' + VERS, - bin_sha1 = '47d59dffb5f02470ccfb6c1a5a31b6040a1636e5', - src_sha1 = '52e133360f2c38046de262c827dfec8ec5b7c885', + bin_sha1 = '8fc9620ec499169facad3355f7417eb6a8aff511', + src_sha1 = '40bd9ae8af8e0b03eb4e43f44f5feda8b7325221', license = 'jgit', + repository = REPO, unsign = True, deps = [':ewah'], exclude = [ @@ -20,8 +22,9 @@ maven_jar( maven_jar( name = 'jgit-servlet', id = 'org.eclipse.jgit:org.eclipse.jgit.http.server:' + VERS, - sha1 = 'f7788bbd0c0414e856c84ddf353e6f4c62fe1d28', + sha1 = 'cecc2b9c0b94455348c3a0c63eb83f72cc595757', license = 'jgit', + repository = REPO, deps = [':jgit'], unsign = True, exclude = [ @@ -33,8 +36,9 @@ maven_jar( maven_jar( name = 'jgit-archive', id = 'org.eclipse.jgit:org.eclipse.jgit.archive:' + VERS, - sha1 = 'ca9da919275dad5ac2844529f4cdccdd919bab5f', + sha1 = '7ccc7c78bf47566045ea7a3c08508ba18e4684ca', license = 'jgit', + repository = REPO, deps = [':jgit', '//lib/commons:compress', '//lib:tukaani-xz', @@ -49,8 +53,9 @@ maven_jar( maven_jar( name = 'junit', id = 'org.eclipse.jgit:org.eclipse.jgit.junit:' + VERS, - sha1 = 'b7418e19efbeb9490b359c8a921cf32bfc57ebbd', + sha1 = '87d64d722447dc3971ace30d2a72593c72a4d05f', license = 'DO_NOT_DISTRIBUTE', + repository = REPO, unsign = True, deps = [':jgit'], ) From 3f2a1e1b2eaf9f30945794c32f0fa605fd9523f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Wed, 18 Mar 2015 15:48:41 -0400 Subject: [PATCH 5/6] Rework intra line diff to interrupt threads instead of killing them Now that MyersDiff is interruptible[1], interrupt threads instead of killing them when MyersDiff goes into infinite loop. Replace IntraLineWorkerPool that was created to allow killing threads by a standard CachedThreadPool. [1] https://git.eclipse.org/r/44041 Change-Id: I39ceef66f503fb9f0b6036fc32b671818772c258 --- Documentation/config-gerrit.txt | 9 - .../java/com/google/gerrit/pgm/Daemon.java | 4 +- .../gerrit/server/patch/DiffExecutor.java | 31 +++ .../server/patch/DiffExecutorModule.java | 39 ++++ .../gerrit/server/patch/IntraLineLoader.java | 50 +++-- .../server/patch/IntraLineWorkerPool.java | 182 ------------------ .../gerrit/testutil/InMemoryModule.java | 15 ++ .../gerrit/httpd/WebAppInitializer.java | 4 +- 8 files changed, 119 insertions(+), 215 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutorModule.java delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineWorkerPool.java diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 4ad7d21da9..f174b28229 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -706,15 +706,6 @@ See also link:cmd-flush-caches.html[gerrit flush-caches]. ==== [[cache_options]]Cache Options -[[cache.diff_intraline.maxIdleWorkers]]cache.diff_intraline.maxIdleWorkers:: -+ -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. -+ -Default is 1.5x number of available CPUs. - [[cache.diff_intraline.timeout]]cache.diff_intraline.timeout:: + Maximum number of milliseconds to wait for intraline difference data diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java index e28b5ba9ad..1c667e3ad0 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java @@ -63,7 +63,7 @@ import com.google.gerrit.server.index.IndexModule; 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.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; @@ -320,7 +320,7 @@ public class Daemon extends SiteProgram { modules.add(new ChangeHookRunner.Module()); modules.add(new ReceiveCommitsExecutorModule()); modules.add(new MergeabilityChecksExecutorModule()); - modules.add(new IntraLineWorkerPool.Module()); + modules.add(new DiffExecutorModule()); modules.add(cfgInjector.getInstance(GerritGlobalModule.class)); modules.add(new InternalAccountDirectory.Module()); modules.add(new DefaultCacheFactory.Module()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java new file mode 100644 index 0000000000..23589e3e3a --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java @@ -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}. + */ +@Retention(RUNTIME) +@BindingAnnotation +public @interface DiffExecutor { +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutorModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutorModule.java new file mode 100644 index 0000000000..9eaea3a62f --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutorModule.java @@ -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()); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java index 4961befe4d..009b4920a6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java @@ -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 { @@ -41,12 +47,13 @@ class IntraLineLoader extends CacheLoader { 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 { } @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 result = diffExecutor.submit(new Callable() { + @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 { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineWorkerPool.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineWorkerPool.java deleted file mode 100644 index 49ae9503ac..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineWorkerPool.java +++ /dev/null @@ -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 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; - private final ArrayBlockingQueue 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; - } - } - } -} diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java index 6b2f7c7556..d96e86126c 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java @@ -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; @@ -49,6 +50,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.Current; import com.google.gerrit.server.schema.DataSourceType; import com.google.gerrit.server.schema.SchemaCreator; @@ -75,6 +77,7 @@ import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.net.InetSocketAddress; import java.net.SocketAddress; +import java.util.concurrent.ExecutorService; public class InMemoryModule extends FactoryModule { public static Config newDefaultConfig() { @@ -157,6 +160,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.sameThreadExecutor(); + } + }); install(new DefaultCacheFactory.Module()); install(new SmtpEmailSender.Module()); install(new SignedTokenEmailTokenVerifier.Module()); diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index e9bd296ea9..f59401c47f 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -46,7 +46,7 @@ import com.google.gerrit.server.git.WorkQueue; 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.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; @@ -280,7 +280,7 @@ public class WebAppInitializer extends GuiceServletContextListener modules.add(new ChangeHookRunner.Module()); modules.add(new ReceiveCommitsExecutorModule()); modules.add(new MergeabilityChecksExecutorModule()); - modules.add(new IntraLineWorkerPool.Module()); + modules.add(new DiffExecutorModule()); modules.add(cfgInjector.getInstance(GerritGlobalModule.class)); modules.add(new InternalAccountDirectory.Module()); modules.add(new DefaultCacheFactory.Module()); From 61074ca4fbe7881212c6a921fb843378ed4fb124 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Mon, 9 Mar 2015 15:20:28 -0400 Subject: [PATCH 6/6] Work around MyersDiff infinite loop in PatchListLoader This infinite loop is happening for some files when the PatchListLoader is computing the differences between 2 commits. It first showed up[1] in the mergeability checks done in background and was easy to work around by killing the thread using Javamelody and abandoning the faulty commits. The issue showed up again, when upgrading from 2.9.x to 2.10: the online reindexer getting stuck because of that infinite loop and this time, no easy work around. Use a similar approach that was done in intraline diff to work around the MyersDiff infinite loop. Instead of returning a timeout error message when the infinite loop is detected, fallback to a diff algorithm that does not use MyersDiff. Returning a timeout error was not an option because the failing operation is not always triggered by a user. From the user perspective, the only difference when the infinite loop is detected is that the files in the commit will not be compared in-depth, which will result in bigger edit regions. [1]https://groups.google.com/d/msg/repo-discuss/ZtiCilM3wFA/LijfZ4YkLHsJ Change-Id: Ib00de070dd8df1722d4ade0a83c0ffa8eaa37f8e --- Documentation/config-gerrit.txt | 19 ++++++ .../java/com/google/gerrit/pgm/Reindex.java | 2 + .../gerrit/server/patch/DiffExecutor.java | 2 +- .../server/patch/PatchListCacheImpl.java | 2 +- .../gerrit/server/patch/PatchListLoader.java | 62 ++++++++++++++++++- 5 files changed, 82 insertions(+), 5 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index f174b28229..3368db9f58 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -706,6 +706,25 @@ See also link:cmd-flush-caches.html[gerrit flush-caches]. ==== [[cache_options]]Cache Options +[[cache.diff.timeout]]cache.diff.timeout:: ++ +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. ++ +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:: + Maximum number of milliseconds to wait for intraline difference data diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java index c0cf8f9222..161efc1ed6 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java @@ -70,6 +70,7 @@ import com.google.gerrit.server.index.IndexModule; import com.google.gerrit.server.index.IndexModule.IndexType; import com.google.gerrit.server.mail.ReplacePatchSetSender; import com.google.gerrit.server.notedb.NoteDbModule; +import com.google.gerrit.server.patch.DiffExecutorModule; import com.google.gerrit.server.patch.PatchListCacheImpl; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.CommentLinkInfo; @@ -191,6 +192,7 @@ public class Reindex extends SiteProgram { private Injector createSysInjector() { List modules = Lists.newArrayList(); + modules.add(new DiffExecutorModule()); modules.add(PatchListCacheImpl.module()); AbstractModule changeIndexModule; switch (IndexModule.getIndexType(dbInjector)) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java index 23589e3e3a..564ca58453 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java @@ -23,7 +23,7 @@ import java.util.concurrent.ExecutorService; /** * Marker on {@link ExecutorService} used by - * {@link IntraLineLoader}. + * {@link IntraLineLoader} and {@link PatchListLoader}. */ @Retention(RUNTIME) @BindingAnnotation diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java index 7b7c73108a..6c769f742a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java @@ -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() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java index de36020d85..d9e796958c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java @@ -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 { static final Logger log = LoggerFactory.getLogger(PatchListLoader.class); @@ -76,13 +84,23 @@ public class PatchListLoader extends CacheLoader { 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 @@ -164,7 +182,7 @@ public class PatchListLoader extends CacheLoader { 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)); } } @@ -175,6 +193,44 @@ public class PatchListLoader extends CacheLoader { } } + private FileHeader toFileHeader(PatchListKey key, + final DiffFormatter diffFormatter, final DiffEntry diffEntry) + throws IOException { + + Future result = diffExecutor.submit(new Callable() { + @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().name() + + " on path " + diffEntry.getNewPath() + + " comparing " + diffEntry.getOldId().name() + + ".." + diffEntry.getNewId().name()); + 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 Repository db, final ObjectReader reader, final RevCommit aCommit, final RevCommit bCommit) throws IOException {