SnapshottingRefDirectory: Invalidate snapshot after locking ref for

update

When using the SnapshottingRefDirectory, if a thread has already read
packed-refs, then another actor updates packed-refs, the original
thread may create an update that is based on the old cached/snapshotted
packed-refs content. That update could effectively perform a forced
update unintentionally because it is unaware of the new content.

This seems particularly likely to happen in a scenario where a loose
ref was just packed. If the ref was loose, our thread would see the
current ref value (because we don't snapshot loose refs and always read
them from disk), but since there is no loose ref, we expect to find the
current value in packed-refs. However, (before this change) we rely
on our snapshot of packed-refs which does not contain the updated ref
value.

Invalidating the cache after the loose ref is locked ensures that the
ref value does not change again before we read it to perform the update.

Bug: jgit-21
Change-Id: Id10900a99bfd0401a1b9c39d997093af0289649e
Signed-off-by: Nasser Grainawi <quic_nasserg@quicinc.com>

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java
index 0dcb3196c5ace831eee2ee11570003c57c51c433..62373a0a7b73b62756802221eb8e2123af404b8b 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java
@@ -54,6 +54,7 @@ class RefDirectoryUpdate extends RefUpdate {
                String name = dst.getName();
                lock = new LockFile(database.fileFor(name));
                if (lock.lock()) {
+                       doAfterLocking(name);
                        dst = database.findRef(name);
                        setOldObjectId(dst != null ? dst.getObjectId() : null);
                        return true;
@@ -141,4 +142,14 @@ class RefDirectoryUpdate extends RefUpdate {
                        return Result.NEW;
                return Result.FORCED;
        }
+
+       /**
+        * Do any actions needed immediately after a lock on the ref is acquired
+        *
+        * @param name
+        *            the name of the reference.
+        */
+       protected void doAfterLocking(String name) {
+               // No actions by default
+       }
 }

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java
index 70eeb2c82adaa0f5bc7278b3d8e73213f462a37a..b973aeb8efc392914021c0a2f68f8006ef60f185 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java
@@ -213,6 +213,21 @@ class SnapshottingRefDirectory extends RefDirectory {
                                        getRefDatabase());
                }
 
+               /**
+                * Invalidate the SnapshottingRefDirectory snapshot after locking the
+                * ref.
+                * <p>
+                * Doing this after locking the ref ensures that the upcoming write is
+                * not based on a cached value.
+                *
+                * @param name
+                *            the name of the reference.
+                */
+               @Override
+               protected void doAfterLocking(String name) {
+                       getRefDatabase().invalidateSnapshot();
+               }
+
                @Override
                public SnapshottingRefDirectory getRefDatabase() {
                        return (SnapshottingRefDirectory) super.getRefDatabase();