From c5617711a1b4d5d0807cc7eed702b78d114d46b3 Mon Sep 17 00:00:00 2001 From: Martin Fick Date: Wed, 8 Mar 2023 08:34:38 -0700 Subject: [PATCH] Revert "RefDirectory: Throw exception if CAS of packed ref list fails" This reverts commit 9c33f7364d41956240818ba12d8b79d5ea846162. Reason for revert: This change was based on the false claim that the packedrefs file lock is held while the CAS is being done, but it is actually released before the CAS (the in memory lock is still held, however that does not prevent external actors from updating the packedrefs files and then another thread from subsequently re-reading it and updating the in memory packedRefList). Although reverting this change can cause the CAS to fail, it should not actually matter since the failure would indicate that another thread has already updated the in memory packedRefList to either the same version this thread was trying to update it too, or to a more recent version. Either way, failing the CAS is then appropriate and should not be problematic. Although this change reverts the code in the RefDirectory class, it keeps the "improvements" to the test so that it continues to pass reliably. The reason for the quotes around the word "improvements" is because I believe the test alteration actually dramatically changes the intent of the test, and that the original intent of the test is untestable with the GC and RefDirectory classes as is. Change-Id: I3acee7527bb542996dcdfaddfb2bdb45ec444db5 Signed-off-by: Martin Fick --- .../internal/storage/file/RefDirectory.java | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) 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 aa3989e48..57dbd389f 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 @@ -1088,22 +1088,7 @@ protected void writeFile(String name, byte[] content) byte[] digest = Constants.newMessageDigest().digest(content); PackedRefList newPackedList = new PackedRefList( refs, lck.getCommitSnapshot(), ObjectId.fromRaw(digest)); - - // This thread holds the file lock, so no other thread or process should - // be able to modify the packed-refs file on disk. If the list changed, - // it means something is very wrong, so throw an exception. - // - // However, we can't use a naive compareAndSet to check whether the - // update was successful, because another thread might _read_ the - // packed refs file that was written out by this thread while holding - // the lock, and update the packedRefs reference to point to that. So - // compare the actual contents instead. - PackedRefList afterUpdate = packedRefs.updateAndGet( - p -> p.id.equals(oldPackedList.id) ? newPackedList : p); - if (!afterUpdate.id.equals(newPackedList.id)) { - throw new ObjectWritingException( - MessageFormat.format(JGitText.get().unableToWrite, name)); - } + packedRefs.compareAndSet(oldPackedList, newPackedList); if (changed) { modCnt.incrementAndGet(); }