From 6f23210781666506bde36b48cf00ffc9506348b0 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 26 Jul 2017 16:53:21 -0400 Subject: [PATCH] RefDirectory: Retry acquiring ref locks with backoff If a repo frequently uses PackedBatchRefUpdates, there is likely to be contention on the packed-refs file, so it's not appropriate to fail immediately the first time we fail to acquire a lock. Add some logic to RefDirectory to support general retrying of lock acquisition. Currently, there is a hard-coded wait starting at 100ms and backing off exponentially to 1600ms, for about 3s of total wait. This is no worse than the hard-coded backoff that JGit does elsewhere, e.g. in FileUtils#delete. One can imagine a scheme that uses per-repository configuration of backoff, and the current interface would support this without changing any callers. Change-Id: I4764e11270d9336882483eb698f67a78a401c251 --- .../storage/file/BatchRefUpdateTest.java | 86 +++++++++++++++++ .../storage/file/RefDirectoryTest.java | 20 ++++ .../storage/file/PackedBatchRefUpdate.java | 95 +++++++++++++++---- .../internal/storage/file/RefDirectory.java | 95 +++++++++++++++++-- 4 files changed, 267 insertions(+), 29 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java index 5a4090722..ead9a5dad 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java @@ -54,6 +54,7 @@ import static org.eclipse.jgit.transport.ReceiveCommand.Type.UPDATE; import static org.eclipse.jgit.transport.ReceiveCommand.Type.UPDATE_NONFASTFORWARD; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -96,6 +97,7 @@ import org.junit.runners.Parameterized.Parameter; import org.junit.runners.Parameterized.Parameters; +@SuppressWarnings("boxing") @RunWith(Parameterized.class) public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase { @Parameter @@ -125,6 +127,7 @@ public void setUp() throws Exception { cfg.save(); refdir = (RefDirectory) diskRepo.getRefDatabase(); + refdir.setRetrySleepMs(Arrays.asList(0, 0)); repo = new TestRepository<>(diskRepo); A = repo.commit().create(); @@ -584,6 +587,85 @@ public void overrideDisableRefLog() throws Exception { getLastReflog("refs/heads/branch")); } + @Test + public void packedRefsLockFailure() throws Exception { + writeLooseRef("refs/heads/master", A); + + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE), + new ReceiveCommand(zeroId(), B, "refs/heads/branch", CREATE)); + + LockFile myLock = refdir.lockPackedRefs(); + try { + execute(newBatchUpdate(cmds).setAllowNonFastForwards(true)); + + assertFalse(getLockFile("refs/heads/master").exists()); + assertFalse(getLockFile("refs/heads/branch").exists()); + + if (atomic) { + assertResults(cmds, LOCK_FAILURE, TRANSACTION_ABORTED); + assertRefs("refs/heads/master", A); + } else { + // Only operates on loose refs, doesn't care that packed-refs is locked. + assertResults(cmds, OK, OK); + assertRefs( + "refs/heads/master", B, + "refs/heads/branch", B); + } + } finally { + myLock.unlock(); + } + } + + @Test + public void oneRefLockFailure() throws Exception { + writeLooseRef("refs/heads/master", A); + + List cmds = Arrays.asList( + new ReceiveCommand(zeroId(), B, "refs/heads/branch", CREATE), + new ReceiveCommand(A, B, "refs/heads/master", UPDATE)); + + LockFile myLock = new LockFile(refdir.fileFor("refs/heads/master")); + assertTrue(myLock.lock()); + try { + execute(newBatchUpdate(cmds).setAllowNonFastForwards(true)); + + assertFalse(LockFile.getLockFile(refdir.packedRefsFile).exists()); + assertFalse(getLockFile("refs/heads/branch").exists()); + + if (atomic) { + assertResults(cmds, TRANSACTION_ABORTED, LOCK_FAILURE); + assertRefs("refs/heads/master", A); + } else { + assertResults(cmds, OK, LOCK_FAILURE); + assertRefs( + "refs/heads/branch", B, + "refs/heads/master", A); + } + } finally { + myLock.unlock(); + } + } + + @Test + public void singleRefUpdateDoesNotRequirePackedRefsLock() throws Exception { + writeLooseRef("refs/heads/master", A); + + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE)); + + LockFile myLock = refdir.lockPackedRefs(); + try { + execute(newBatchUpdate(cmds).setAllowNonFastForwards(true)); + + assertFalse(getLockFile("refs/heads/master").exists()); + assertResults(cmds, OK); + assertRefs("refs/heads/master", B); + } finally { + myLock.unlock(); + } + } + private void writeLooseRef(String name, AnyObjectId id) throws IOException { write(new File(diskRepo.getDirectory(), name), id.name() + "\n"); } @@ -712,6 +794,10 @@ private ReflogEntry getLastReflog(String name) throws IOException { return r.getLastEntry(); } + private File getLockFile(String refName) { + return LockFile.getLockFile(refdir.fileFor(refName)); + } + private void assertReflogUnchanged( Map old, String name) throws IOException { assertReflogEquals(old.get(name), getLastReflog(name), true); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java index 145fed6b1..fefccf314 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java @@ -60,10 +60,12 @@ import java.io.File; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import org.eclipse.jgit.errors.LockFailedException; import org.eclipse.jgit.events.ListenerHandle; import org.eclipse.jgit.events.RefsChangedEvent; import org.eclipse.jgit.events.RefsChangedListener; @@ -79,6 +81,7 @@ import org.junit.Before; import org.junit.Test; +@SuppressWarnings("boxing") public class RefDirectoryTest extends LocalDiskRepositoryTestCase { private Repository diskRepo; @@ -1284,6 +1287,23 @@ public void onRefsChanged(RefsChangedEvent event) { assertEquals(1, changeCount.get()); } + @Test + public void testPackedRefsLockFailure() throws Exception { + writeLooseRef("refs/heads/master", A); + refdir.setRetrySleepMs(Arrays.asList(0, 0)); + LockFile myLock = refdir.lockPackedRefs(); + try { + refdir.pack(Arrays.asList("refs/heads/master")); + fail("expected LockFailedException"); + } catch (LockFailedException e) { + assertEquals(refdir.packedRefsFile.getPath(), e.getFile().getPath()); + } finally { + myLock.unlock(); + } + Ref ref = refdir.getRef("refs/heads/master"); + assertEquals(Storage.LOOSE, ref.getStorage()); + } + private void writeLooseRef(String name, AnyObjectId id) throws IOException { writeLooseRef(name, id.name() + "\n"); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java index 06c31f2c2..e71977dcc 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java @@ -49,6 +49,7 @@ import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_NONFASTFORWARD; import java.io.IOException; +import java.text.MessageFormat; import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; @@ -57,7 +58,10 @@ import java.util.Map; import java.util.Set; +import org.eclipse.jgit.annotations.Nullable; +import org.eclipse.jgit.errors.LockFailedException; import org.eclipse.jgit.errors.MissingObjectException; +import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.file.RefDirectory.PackedRefList; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.ObjectId; @@ -164,12 +168,18 @@ public void execute(RevWalk walk, ProgressMonitor monitor, // Pack refs normally, so we can create lock files even in the case where // refs/x is deleted and refs/x/y is created in this batch. - refdb.pack( - pending.stream().map(ReceiveCommand::getRefName).collect(toList())); - - Map locks = new HashMap<>(); try { - if (!lockLooseRefs(pending, locks)) { + refdb.pack( + pending.stream().map(ReceiveCommand::getRefName).collect(toList())); + } catch (LockFailedException e) { + lockFailure(pending.get(0), pending); + return; + } + + Map locks = null; + try { + locks = lockLooseRefs(pending); + if (locks == null) { return; } PackedRefList oldPackedList = refdb.pack(locks); @@ -177,15 +187,15 @@ public void execute(RevWalk walk, ProgressMonitor monitor, if (newRefs == null) { return; } - LockFile packedRefsLock = new LockFile(refdb.packedRefsFile); - try { - packedRefsLock.lock(); - refdb.commitPackedRefs(packedRefsLock, newRefs, oldPackedList); - } finally { - packedRefsLock.unlock(); + LockFile packedRefsLock = refdb.lockPackedRefs(); + if (packedRefsLock == null) { + lockFailure(pending.get(0), pending); + return; } + // commitPackedRefs removes lock file (by renaming over real file). + refdb.commitPackedRefs(packedRefsLock, newRefs, oldPackedList); } finally { - locks.values().forEach(LockFile::unlock); + unlockAll(locks); } refdb.fireRefsChanged(); @@ -271,17 +281,54 @@ private boolean checkNonFastForwards(RevWalk walk, return true; } - private boolean lockLooseRefs(List commands, - Map locks) throws IOException { - for (ReceiveCommand c : commands) { - LockFile lock = new LockFile(refdb.fileFor(c.getRefName())); - if (!lock.lock()) { - lockFailure(c, commands); - return false; + /** + * Lock loose refs corresponding to a list of commands. + * + * @param commands + * commands that we intend to execute. + * @return map of ref name in the input commands to lock file. Always contains + * one entry for each ref in the input list. All locks are acquired + * before returning. If any lock was not able to be acquired: the + * return value is null; no locks are held; and all commands that were + * pending are set to fail with {@code LOCK_FAILURE}. + * @throws IOException + * an error occurred other than a failure to acquire; no locks are + * held if this exception is thrown. + */ + @Nullable + private Map lockLooseRefs(List commands) + throws IOException { + ReceiveCommand failed = null; + Map locks = new HashMap<>(); + try { + RETRY: for (int ms : refdb.getRetrySleepMs()) { + failed = null; + // Release all locks before trying again, to prevent deadlock. + unlockAll(locks); + locks.clear(); + RefDirectory.sleep(ms); + + for (ReceiveCommand c : commands) { + String name = c.getRefName(); + LockFile lock = new LockFile(refdb.fileFor(name)); + if (locks.put(name, lock) != null) { + throw new IOException( + MessageFormat.format(JGitText.get().duplicateRef, name)); + } + if (!lock.lock()) { + failed = c; + continue RETRY; + } + } + Map result = locks; + locks = null; + return result; } - locks.put(c.getRefName(), lock); + } finally { + unlockAll(locks); } - return true; + lockFailure(failed != null ? failed : commands.get(0), commands); + return null; } private static RefList applyUpdates(RevWalk walk, RefList refs, @@ -444,6 +491,12 @@ private static Ref peeledRef(RevWalk walk, ReceiveCommand cmd) Ref.Storage.PACKED, cmd.getRefName(), newId); } + private static void unlockAll(@Nullable Map locks) { + if (locks != null) { + locks.values().forEach(LockFile::unlock); + } + } + private static void lockFailure(ReceiveCommand cmd, List commands) { reject(cmd, LOCK_FAILURE, commands); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java index eb569749b..20944ad31 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java @@ -63,6 +63,7 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStreamReader; +import java.io.InterruptedIOException; import java.security.DigestInputStream; import java.security.MessageDigest; import java.text.MessageFormat; @@ -76,6 +77,7 @@ import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jgit.annotations.NonNull; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.errors.InvalidObjectIdException; import org.eclipse.jgit.errors.LockFailedException; import org.eclipse.jgit.errors.MissingObjectException; @@ -137,6 +139,10 @@ public class RefDirectory extends RefDatabase { Constants.MERGE_HEAD, Constants.FETCH_HEAD, Constants.ORIG_HEAD, Constants.CHERRY_PICK_HEAD }; + @SuppressWarnings("boxing") + private static final List RETRY_SLEEP_MS = + Collections.unmodifiableList(Arrays.asList(0, 100, 200, 400, 800, 1600)); + private final FileRepository parent; private final File gitDir; @@ -176,6 +182,8 @@ public class RefDirectory extends RefDatabase { */ private final AtomicInteger lastNotifiedModCnt = new AtomicInteger(); + private List retrySleepMs = RETRY_SLEEP_MS; + RefDirectory(final FileRepository db) { final FS fs = db.getFS(); parent = db; @@ -602,9 +610,7 @@ void delete(RefDirectoryUpdate update) throws IOException { // we don't miss an edit made externally. final PackedRefList packed = getPackedRefs(); if (packed.contains(name)) { - LockFile lck = new LockFile(packedRefsFile); - if (!lck.lock()) - throw new LockFailedException(packedRefsFile); + LockFile lck = lockPackedRefsOrThrow(); try { PackedRefList cur = readPackedRefs(); int idx = cur.find(name); @@ -665,11 +671,7 @@ private PackedRefList pack(Collection refs, FS fs = parent.getFS(); // Lock the packed refs file and read the content - LockFile lck = new LockFile(packedRefsFile); - if (!lck.lock()) { - throw new IOException(MessageFormat.format( - JGitText.get().cannotLock, packedRefsFile)); - } + LockFile lck = lockPackedRefsOrThrow(); try { final PackedRefList packed = getPackedRefs(); @@ -765,6 +767,26 @@ private PackedRefList pack(Collection refs, } } + @Nullable + LockFile lockPackedRefs() throws IOException { + LockFile lck = new LockFile(packedRefsFile); + for (int ms : getRetrySleepMs()) { + sleep(ms); + if (lck.lock()) { + return lck; + } + } + return null; + } + + private LockFile lockPackedRefsOrThrow() throws IOException { + LockFile lck = lockPackedRefs(); + if (lck == null) { + throw new LockFailedException(packedRefsFile); + } + return lck; + } + /** * Make sure a ref is peeled and has the Storage PACKED. If the given ref * has this attributes simply return it. Otherwise create a new peeled @@ -1175,6 +1197,63 @@ private static void delete(final File file, final int depth, LockFile rLck) } } + /** + * Get times to sleep while retrying a possibly contentious operation. + *

+ * For retrying an operation that might have high contention, such as locking + * the {@code packed-refs} file, the caller may implement a retry loop using + * the returned values: + * + * 

+	 * for (int toSleepMs : getRetrySleepMs()) {
+	 *   sleep(toSleepMs);
+	 *   if (isSuccessful(doSomething())) {
+	 *     return success;
+	 *   }
+	 * }
+	 * return failure;
+	 *
+ * + * The first value in the returned iterable is 0, and the caller should treat + * a fully-consumed iterator as a timeout. + * + * @return iterable of times, in milliseconds, that the caller should sleep + * before attempting an operation. + */ + Iterable getRetrySleepMs() { + return retrySleepMs; + } + + void setRetrySleepMs(List retrySleepMs) { + if (retrySleepMs == null || retrySleepMs.isEmpty() + || retrySleepMs.get(0).intValue() != 0) { + throw new IllegalArgumentException(); + } + this.retrySleepMs = retrySleepMs; + } + + /** + * Sleep with {@link Thread#sleep(long)}, converting {@link + * InterruptedException} to {@link InterruptedIOException}. + * + * @param ms + * time to sleep, in milliseconds; zero or negative is a no-op. + * @throws InterruptedIOException + * if sleeping was interrupted. + */ + static void sleep(long ms) throws InterruptedIOException { + if (ms <= 0) { + return; + } + try { + Thread.sleep(ms); + } catch (InterruptedException e) { + InterruptedIOException ie = new InterruptedIOException(); + ie.initCause(e); + throw ie; + } + } + static class PackedRefList extends RefList { private final FileSnapshot snapshot;