From d4a19c328fb07e6adafaf0f6a88c22f007ac5753 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Wed, 15 Aug 2018 09:54:29 -0400 Subject: [PATCH] Fix GC run in foreground to not use executor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since I3870cadb4, GC task was always delegated to an executor even when background option was set to false. This was an issue because if more than one GC object was instantiated and executed in parallel, only one GC was actually running because of the single thread executor. Change-Id: I8c587d22d63c1601b7d75914692644a385cd86d6 Signed-off-by: Hugo Arès --- .../jgit/internal/storage/file/GC.java | 55 +++++++------------ .../jgit/internal/storage/file/GcLog.java | 22 +------- 2 files changed, 22 insertions(+), 55 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java index d99a48524..e4b0a46a3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java @@ -78,16 +78,13 @@ import java.util.Objects; import java.util.Set; import java.util.TreeMap; import java.util.concurrent.Callable; -import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; -import java.util.concurrent.Future; import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; import org.eclipse.jgit.annotations.NonNull; -import org.eclipse.jgit.api.errors.JGitInternalException; import org.eclipse.jgit.dircache.DirCacheIterator; import org.eclipse.jgit.errors.CancelledException; import org.eclipse.jgit.errors.CorruptObjectException; @@ -232,8 +229,11 @@ public class GC { */ // TODO(ms): in 5.0 change signature and return Future> public Collection gc() throws IOException, ParseException { - final GcLog gcLog = background ? new GcLog(repo) : null; - if (gcLog != null && !gcLog.lock(background)) { + if (!background) { + return doGc(); + } + final GcLog gcLog = new GcLog(repo); + if (!gcLog.lock()) { // there is already a background gc running return Collections.emptyList(); } @@ -241,48 +241,31 @@ public class GC { Callable> gcTask = () -> { try { Collection newPacks = doGc(); - if (automatic && tooManyLooseObjects() && gcLog != null) { + if (automatic && tooManyLooseObjects()) { String message = JGitText.get().gcTooManyUnpruned; gcLog.write(message); gcLog.commit(); } return newPacks; } catch (IOException | ParseException e) { - if (background) { - if (gcLog == null) { - // Lacking a log, there's no way to report this. - return Collections.emptyList(); - } - try { - gcLog.write(e.getMessage()); - StringWriter sw = new StringWriter(); - e.printStackTrace(new PrintWriter(sw)); - gcLog.write(sw.toString()); - gcLog.commit(); - } catch (IOException e2) { - e2.addSuppressed(e); - LOG.error(e2.getMessage(), e2); - } - } else { - throw new JGitInternalException(e.getMessage(), e); + try { + gcLog.write(e.getMessage()); + StringWriter sw = new StringWriter(); + e.printStackTrace(new PrintWriter(sw)); + gcLog.write(sw.toString()); + gcLog.commit(); + } catch (IOException e2) { + e2.addSuppressed(e); + LOG.error(e2.getMessage(), e2); } } finally { - if (gcLog != null) { - gcLog.unlock(); - } + gcLog.unlock(); } return Collections.emptyList(); }; - Future> result = executor.submit(gcTask); - if (background) { - // TODO(ms): in 5.0 change signature and return the Future - return Collections.emptyList(); - } - try { - return result.get(); - } catch (InterruptedException | ExecutionException e) { - throw new IOException(e); - } + // TODO(ms): in 5.0 change signature and return the Future + executor.submit(gcTask); + return Collections.emptyList(); } private Collection doGc() throws IOException, ParseException { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GcLog.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GcLog.java index 9ea77cc7d..35049d406 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GcLog.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GcLog.java @@ -44,20 +44,17 @@ package org.eclipse.jgit.internal.storage.file; import org.eclipse.jgit.api.errors.JGitInternalException; -import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.ConfigConstants; import org.eclipse.jgit.util.GitDateParser; import org.eclipse.jgit.util.SystemReader; import static java.nio.charset.StandardCharsets.UTF_8; -import java.io.BufferedReader; import java.io.File; import java.io.IOException; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.attribute.FileTime; -import java.text.MessageFormat; import java.text.ParseException; import java.time.Instant; @@ -103,22 +100,11 @@ class GcLog { return gcLogExpire; } - private boolean autoGcBlockedByOldLockFile(boolean background) { + private boolean autoGcBlockedByOldLockFile() { try { FileTime lastModified = Files.getLastModifiedTime(logFile.toPath()); if (lastModified.toInstant().compareTo(getLogExpiry()) > 0) { // There is an existing log file, which is too recent to ignore - if (!background) { - try (BufferedReader reader = Files - .newBufferedReader(logFile.toPath())) { - char[] buf = new char[1000]; - int len = reader.read(buf, 0, 1000); - String oldError = new String(buf, 0, len); - - throw new JGitInternalException(MessageFormat.format( - JGitText.get().gcLogExists, oldError, logFile)); - } - } return true; } } catch (NoSuchFileException e) { @@ -132,11 +118,9 @@ class GcLog { /** * Lock the GC log file for updates * - * @param background - * If true, and if gc.log already exists, unlock and return false * @return {@code true} if we hold the lock */ - boolean lock(boolean background) { + boolean lock() { try { if (!lock.lock()) { return false; @@ -144,7 +128,7 @@ class GcLog { } catch (IOException e) { throw new JGitInternalException(e.getMessage(), e); } - if (autoGcBlockedByOldLockFile(background)) { + if (autoGcBlockedByOldLockFile()) { lock.unlock(); return false; }