DfsPackFile: Do not set primery index local ref from cache callback

DfsPackFile assumes the indices are in pack streams, but we would like
to consider other formats and storage. Currently, the local ref in the
DfsPackFile to the index is set in the cache loading callback, which
prevents abstracting the loading.

Reorganize the code so: the loadPackIndex function just parses the bytes
returning a reference and the caller sets the loaded index in the local
ref and DfsBlockCache.

We will follow this pattern with other indices in follow-up
changes. Note that although DfsPackFile is used only in one thread,
the loading in DfsBlockCache can happen from multiple threads
concurrently and we want to keep only one ref around.

Change-Id: Ie99de6c80784225484c0f99c51caa44c6a372c45

1 files changed, 27 insertions, 16 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java
index 5cc2a57aba..3e4d4d300c 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java
@@ -29,6 +29,7 @@ import java.nio.channels.Channels;
 import java.text.MessageFormat;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.zip.CRC32;
 import java.util.zip.DataFormatException;
 import java.util.zip.Inflater;
@@ -196,18 +197,24 @@ public final class DfsPackFile extends BlockBasedFile {
 				.dispatch(new BeforeDfsPackIndexLoadedEvent(this));
 		try {
 			DfsStreamKey idxKey = desc.getStreamKey(INDEX);
-			AtomicBoolean cacheHit = new AtomicBoolean(true);
-			DfsBlockCache.Ref<PackIndex> idxref = cache.getOrLoadRef(idxKey,
+			// Keep the value parsed in the loader, in case the Ref<> is
+			// nullified in ClockBlockCacheTable#reserveSpace
+			// before we read its value.
+			AtomicReference<PackIndex> loadedRef = new AtomicReference<>(null);
+			DfsBlockCache.Ref<PackIndex> cachedRef = cache.getOrLoadRef(idxKey,
 					REF_POSITION, () -> {
-						cacheHit.set(false);
-						return loadPackIndex(ctx, idxKey);
+						RefWithSize<PackIndex> idx = loadPackIndex(ctx);
+						loadedRef.set(idx.ref);
+						return new DfsBlockCache.Ref<>(idxKey, REF_POSITION,
+								idx.size, idx.ref);
 					});
-			if (cacheHit.get()) {
+			if (loadedRef.get() == null) {
 				ctx.stats.idxCacheHit++;
 			}
-			PackIndex idx = idxref.get();
-			if (index == null && idx != null) {
-				index = idx;
+			index = cachedRef.get() != null ? cachedRef.get() : loadedRef.get();
+			if (index == null) {
+				throw new IOException(
+						"Couldn't get a reference to the primary index"); //$NON-NLS-1$
 			}
 			ctx.emitIndexLoad(desc, INDEX, index);
 			return index;
@@ -1210,20 +1217,15 @@ public final class DfsPackFile extends BlockBasedFile {
 		}
 	}
 
-	private DfsBlockCache.Ref<PackIndex> loadPackIndex(
-			DfsReader ctx, DfsStreamKey idxKey) throws IOException {
+	private RefWithSize<PackIndex> loadPackIndex(DfsReader ctx)
+			throws IOException {
 		try {
 			ctx.stats.readIdx++;
 			long start = System.nanoTime();
 			try (ReadableChannel rc = ctx.db.openFile(desc, INDEX)) {
 				PackIndex idx = PackIndex.read(alignTo8kBlocks(rc));
 				ctx.stats.readIdxBytes += rc.position();
-				index = idx;
-				return new DfsBlockCache.Ref<>(
-						idxKey,
-						REF_POSITION,
-						idx.getObjectCount() * REC_SIZE,
-						idx);
+				return new RefWithSize<>(idx, idx.getObjectCount() * REC_SIZE);
 			} finally {
 				ctx.stats.readIdxMicros += elapsedMicros(start);
 			}
@@ -1449,4 +1451,13 @@ public final class DfsPackFile extends BlockBasedFile {
 			}
 		}
 	}
+
+	private static final class RefWithSize<V> {
+		final V ref;
+		final long size;
+		RefWithSize(V ref, long size) {
+			this.ref = ref;
+			this.size = size;
+		}
+	}
 }