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>

2 files changed, 26 insertions, 0 deletions

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 0dcb3196c5..62373a0a7b 100644
--- 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 70eeb2c82a..b973aeb8ef 100644
--- 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();