PatchListLoader: Synchronize MyersDiff and HistogramDiff invocations
To overcome MyersDiff endless loop problem, the computation thread can
be interrupted and set to cancel with Future.cancel(boolean) method.
However it cannot be assumed that after this method returns the thread
is terminated. When the system is under heavy load or/and the thread in
question is in a tight loop, it won't stop. It can be seen with this
instrumentation change in JGit: [1] with the corresponding dump [2].
Synchronize the MyersDiff and alternative HistogramDiff algorithm
invocations to prevent pack file corruption, as WindowCursor.inflate()
method isn't synchronized.
[1] https://git.eclipse.org/r/#/c/57583
[2] http://paste.openstack.org/show/475785
Bug-JGit: https://bugs.eclipse.org/bugs/show_bug.cgi?id=467467
Bug: Issue 3361
Contributed-By: Khai Do <zaro0508@gmail.com>
Change-Id: I4516bcc2c41792acdb8174cb9d3cc198ddfaf8ef
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 5444e4a..95172b0 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
@@ -86,6 +86,7 @@
private final ThreeWayMergeStrategy mergeStrategy;
private final ExecutorService diffExecutor;
private final long timeoutMillis;
+ private final Object lock;
@Inject
@@ -97,6 +98,7 @@
patchListCache = plc;
mergeStrategy = MergeUtil.getMergeStrategy(cfg);
diffExecutor = de;
+ lock = new Object();
timeoutMillis =
ConfigUtil.getTimeUnit(cfg, "cache", PatchListCacheImpl.FILE_NAME,
"timeout", TimeUnit.MILLISECONDS.convert(5, TimeUnit.SECONDS),
@@ -200,7 +202,9 @@
Future<FileHeader> result = diffExecutor.submit(new Callable<FileHeader>() {
@Override
public FileHeader call() throws IOException {
- return diffFormatter.toFileHeader(diffEntry);
+ synchronized (lock) {
+ return diffFormatter.toFileHeader(diffEntry);
+ }
}
});
@@ -214,7 +218,9 @@
+ " comparing " + diffEntry.getOldId().name()
+ ".." + diffEntry.getNewId().name());
result.cancel(true);
- return toFileHeaderWithoutMyersDiff(diffFormatter, diffEntry);
+ synchronized (lock) {
+ 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.
