From 96d9e3eb197c2cd95fc5277b71d8f03f1893bcda Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Thu, 30 Mar 2023 13:43:17 +0200 Subject: [PATCH] Prevent infinite loop rescanning the pack list on PackMismatchException We found, when analysing an incident where Gerrit's gc runner thread got stuck, that we can end up in an infinite loop in ObjectDirectory#openPackedObject which tries to rescan the pack list and starts over trying to open a packed object in an unconfined loop if it catches a PackMismatchException. Here the relevant part of a thread dump we created while the gc runner was stuck: "WorkQueue-2[java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask@350812a3[Not completed, task = java.util.concurrent.Executors$RunnableAdapter@5425d7ee]]" #72 tid=0x00007f73cee1c800 nid=0x584 runnable [0x00007f7392d57000] java.lang.Thread.State: RUNNABLE at org.eclipse.jgit.internal.storage.file.WindowCache.removeAll(WindowCache.java:716) at org.eclipse.jgit.internal.storage.file.WindowCache.purge(WindowCache.java:399) at org.eclipse.jgit.internal.storage.file.PackFile.close(PackFile.java:296) at org.eclipse.jgit.internal.storage.file.ObjectDirectory.reuseMap(ObjectDirectory.java:973) at org.eclipse.jgit.internal.storage.file.ObjectDirectory.scanPacksImpl(ObjectDirectory.java:904) at org.eclipse.jgit.internal.storage.file.ObjectDirectory.scanPacks(ObjectDirectory.java:895) - locked <0x000000050a498f60> (a java.util.concurrent.atomic.AtomicReference) at org.eclipse.jgit.internal.storage.file.ObjectDirectory.searchPacksAgain(ObjectDirectory.java:794) at org.eclipse.jgit.internal.storage.file.ObjectDirectory.openPackedObject(ObjectDirectory.java:465) at org.eclipse.jgit.internal.storage.file.ObjectDirectory.openPackedFromSelfOrAlternate(ObjectDirectory.java:417) at org.eclipse.jgit.internal.storage.file.ObjectDirectory.openObject(ObjectDirectory.java:408) at org.eclipse.jgit.internal.storage.file.WindowCursor.open(WindowCursor.java:132) at org.eclipse.jgit.lib.ObjectReader$1.open(ObjectReader.java:279) at org.eclipse.jgit.revwalk.RevWalk$2.next(RevWalk.java:1031) at org.eclipse.jgit.internal.storage.pack.PackWriter.findObjectsToPack(PackWriter.java:1911) at org.eclipse.jgit.internal.storage.pack.PackWriter.preparePack(PackWriter.java:960) at org.eclipse.jgit.internal.storage.pack.PackWriter.preparePack(PackWriter.java:876) at org.eclipse.jgit.internal.storage.file.GC.writePack(GC.java:1168) at org.eclipse.jgit.internal.storage.file.GC.repack(GC.java:852) at org.eclipse.jgit.internal.storage.file.GC.doGc(GC.java:269) at org.eclipse.jgit.internal.storage.file.GC.gc(GC.java:220) at org.eclipse.jgit.api.GarbageCollectCommand.call(GarbageCollectCommand.java:179) at com.google.gerrit.server.git.GarbageCollection.run(GarbageCollection.java:112) at com.google.gerrit.server.git.GarbageCollection.run(GarbageCollection.java:75) at com.google.gerrit.server.git.GarbageCollection.run(GarbageCollection.java:71) at com.google.gerrit.server.git.GarbageCollectionRunner.run(GarbageCollectionRunner.java:76) at com.google.gerrit.server.logging.LoggingContextAwareRunnable.run(LoggingContextAwareRunnable.java:103) at java.util.concurrent.Executors$RunnableAdapter.call(java.base@11.0.18/Executors.java:515) at java.util.concurrent.FutureTask.runAndReset(java.base@11.0.18/FutureTask.java:305) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(java.base@11.0.18/ScheduledThreadPoolExecutor.java:305) at com.google.gerrit.server.git.WorkQueue$Task.run(WorkQueue.java:612) at java.util.concurrent.ThreadPoolExecutor.runWorker(java.base@11.0.18/ThreadPoolExecutor.java:1128) at java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base@11.0.18/ThreadPoolExecutor.java:628) at java.lang.Thread.run(java.base@11.0.18/Thread.java:829) The code in ObjectDirectory#openPackedObject [1] apparently assumes that this is caused by a transient problem which it can resume from by retrying. We use `core.trustFolderStat = false` on this server since it uses NFS. The incident we had showed that we can enter into an infinite loop here if there is a permanent mismatch between a pack file and its corresponding pack index. I am not yet sure how this can happen. Break the infinite loop by limiting the number of attempts rescanning the pack list to 5 retries. When we exceed this threshold set the type of the PackMismatchException to permanent and rethrow it which breaks the infinite loop. Also apply the same limit in #getPackedObjectSize and #selectObjectRepresentation where we use similar retry loops. [1] https://git.eclipse.org/r/plugins/gitiles/jgit/jgit/+/011c26ff36b9e76c84fc2459e337f159c0f55a9a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java#465 Change-Id: I20fb63bcc1fdc3a03d39b963f06a90e6f0ba73dc --- org.eclipse.jgit/.settings/.api_filters | 17 +++++++++ .../jgit/errors/PackMismatchException.java | 29 +++++++++++++++ .../storage/file/ObjectDirectory.java | 37 +++++++++++++++---- 3 files changed, 76 insertions(+), 7 deletions(-) create mode 100644 org.eclipse.jgit/.settings/.api_filters diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters new file mode 100644 index 0000000000..7a3dc84085 --- /dev/null +++ b/org.eclipse.jgit/.settings/.api_filters @@ -0,0 +1,17 @@ + + + + + + + + + + + + + + + + + diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/errors/PackMismatchException.java b/org.eclipse.jgit/src/org/eclipse/jgit/errors/PackMismatchException.java index ad5664ceb2..5f3f0c39fd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/errors/PackMismatchException.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/errors/PackMismatchException.java @@ -18,6 +18,8 @@ import java.io.IOException; public class PackMismatchException extends IOException { private static final long serialVersionUID = 1L; + private boolean permanent; + /** * Construct a pack modification error. * @@ -27,4 +29,31 @@ public class PackMismatchException extends IOException { public PackMismatchException(String why) { super(why); } + + /** + * Set the type of the exception + * + * @param permanent + * whether the exception is considered permanent + * @since 5.9.1 + */ + public void setPermanent(boolean permanent) { + this.permanent = permanent; + } + + /** + * Check if this is a permanent problem + * + * @return if this is a permanent problem and repeatedly scanning the + * packlist couldn't fix it + * @since 5.9.1 + */ + public boolean isPermanent() { + return permanent; + } + + @Override + public String toString() { + return super.toString() + ", permanent: " + permanent; //$NON-NLS-1$ + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java index 9d1d457369..2131e5f670 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java @@ -89,6 +89,8 @@ public class ObjectDirectory extends FileObjectDatabase { * handle exception is thrown */ final static int MAX_LOOSE_OBJECT_STALE_READ_ATTEMPTS = 5; + private static final int MAX_PACKLIST_RESCAN_ATTEMPTS = 5; + private final AlternateHandle handle = new AlternateHandle(this); private final Config config; @@ -413,7 +415,8 @@ public class ObjectDirectory extends FileObjectDatabase { } private ObjectLoader openPackedFromSelfOrAlternate(WindowCursor curs, - AnyObjectId objectId, Set skips) { + AnyObjectId objectId, Set skips) + throws PackMismatchException { ObjectLoader ldr = openPackedObject(curs, objectId); if (ldr != null) { return ldr; @@ -449,9 +452,11 @@ public class ObjectDirectory extends FileObjectDatabase { return null; } - ObjectLoader openPackedObject(WindowCursor curs, AnyObjectId objectId) { + ObjectLoader openPackedObject(WindowCursor curs, AnyObjectId objectId) + throws PackMismatchException { PackList pList; do { + int retries = 0; SEARCH: for (;;) { pList = packList.get(); for (PackFile p : pList.packs) { @@ -462,8 +467,10 @@ public class ObjectDirectory extends FileObjectDatabase { return ldr; } catch (PackMismatchException e) { // Pack was modified; refresh the entire pack list. - if (searchPacksAgain(pList)) + if (searchPacksAgain(pList)) { + retries = checkRescanPackThreshold(retries, e); continue SEARCH; + } } catch (IOException e) { handlePackError(e, p); } @@ -555,7 +562,8 @@ public class ObjectDirectory extends FileObjectDatabase { } private long getPackedSizeFromSelfOrAlternate(WindowCursor curs, - AnyObjectId id, Set skips) { + AnyObjectId id, Set skips) + throws PackMismatchException { long len = getPackedObjectSize(curs, id); if (0 <= len) { return len; @@ -590,9 +598,11 @@ public class ObjectDirectory extends FileObjectDatabase { return -1; } - private long getPackedObjectSize(WindowCursor curs, AnyObjectId id) { + private long getPackedObjectSize(WindowCursor curs, AnyObjectId id) + throws PackMismatchException { PackList pList; do { + int retries = 0; SEARCH: for (;;) { pList = packList.get(); for (PackFile p : pList.packs) { @@ -603,8 +613,10 @@ public class ObjectDirectory extends FileObjectDatabase { return len; } catch (PackMismatchException e) { // Pack was modified; refresh the entire pack list. - if (searchPacksAgain(pList)) + if (searchPacksAgain(pList)) { + retries = checkRescanPackThreshold(retries, e); continue SEARCH; + } } catch (IOException e) { handlePackError(e, p); } @@ -632,13 +644,14 @@ public class ObjectDirectory extends FileObjectDatabase { @Override void selectObjectRepresentation(PackWriter packer, ObjectToPack otp, - WindowCursor curs) throws IOException { + WindowCursor curs) throws IOException { selectObjectRepresentation(packer, otp, curs, null); } private void selectObjectRepresentation(PackWriter packer, ObjectToPack otp, WindowCursor curs, Set skips) throws IOException { PackList pList = packList.get(); + int retries = 0; SEARCH: for (;;) { for (PackFile p : pList.packs) { try { @@ -649,6 +662,7 @@ public class ObjectDirectory extends FileObjectDatabase { } catch (PackMismatchException e) { // Pack was modified; refresh the entire pack list. // + retries = checkRescanPackThreshold(retries, e); pList = scanPacks(pList); continue SEARCH; } catch (IOException e) { @@ -666,6 +680,15 @@ public class ObjectDirectory extends FileObjectDatabase { } } + private int checkRescanPackThreshold(int retries, PackMismatchException e) + throws PackMismatchException { + if (retries++ > MAX_PACKLIST_RESCAN_ATTEMPTS) { + e.setPermanent(true); + throw e; + } + return retries; + } + private void handlePackError(IOException e, PackFile p) { String warnTmpl = null; int transientErrorCount = 0; -- 2.39.5