From 29c89d1f02e84d0395e12f55204bd129e61c43bf Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Thu, 25 Jan 2024 16:29:05 -0700 Subject: 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 --- .../jgit/internal/storage/file/RefDirectoryUpdate.java | 11 +++++++++++ .../internal/storage/file/SnapshottingRefDirectory.java | 15 +++++++++++++++ 2 files changed, 26 insertions(+) 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. + *

+ * 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(); -- cgit v1.2.3