From a720a0e77dd05f4b9f93e2ff07e66777c0ed1f31 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Sat, 7 Mar 2009 16:17:15 -0800 Subject: [PATCH] Fix a race condition when loading non-mmapd byte windows Back in 439d441b0800 ("Change ByteArrayWindow to read content outside of WindowCache's lock") I introduced a race condition to WindowCache. It is possible for a ByteArrayWindow to be allocated and put into the cache, and then for the calling thread to get put to sleep before it can actually load the window from disk. By the time the thread wakes up again, the window may have already been evicted from the cache, and its underlying file was closed. This results in stack traces such as: java.lang.RuntimeException: Cannot fault in window at org.spearce.jgit.lib.ByteArrayWindow.ensureLoaded(ByteArrayWindow.java:111) ... Caused by: java.nio.channels.ClosedChannelException at sun.nio.ch.FileChannelImpl.ensureOpen(FileChannelImpl.java:91) Now when we allocate a ByteArrayWindow for a specific window reference we increment the file reference count one additional time, forcing the file to stay open even if the window was evicted from the cache. The reference count is decremented once the window loads successfully, and this puts us back into the state of 1 reference count value for each window still in the window cache. Signed-off-by: Shawn O. Pearce Signed-off-by: Robin Rosenberg Fix a race condition when loading non-mmapd byte windows "Shawn O. Pearce" wrote: > Oh hell, I missed the fact that markLoaded(ByteWindow) may be > dropping the last reference and need to close the file. *and* you need to squash this in to prevent a deadlock: Frell. What a Saturday. Signed-off-by: Robin Rosenberg --- .../src/org/spearce/jgit/lib/ByteArrayWindow.java | 28 ++++++++++++++-------- .../src/org/spearce/jgit/lib/ByteBufferWindow.java | 4 ++++ .../src/org/spearce/jgit/lib/ByteWindow.java | 2 ++ .../src/org/spearce/jgit/lib/WindowCache.java | 14 ++++++++++- .../src/org/spearce/jgit/lib/WindowedFile.java | 1 + 5 files changed, 38 insertions(+), 11 deletions(-) diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java index 7b7c7a4e..c07a260c 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java @@ -68,7 +68,6 @@ final class ByteArrayWindow extends ByteWindow { } int copy(final byte[] array, final int p, final byte[] b, final int o, int n) { - ensureLoaded(array); n = Math.min(array.length - p, n); System.arraycopy(array, p, b, o, n); return n; @@ -76,7 +75,6 @@ final class ByteArrayWindow extends ByteWindow { int inflate(final byte[] array, final int pos, final byte[] b, int o, final Inflater inf) throws DataFormatException { - ensureLoaded(array); while (!inf.finished()) { if (inf.needsInput()) { inf.setInput(array, pos, array.length - pos); @@ -91,7 +89,6 @@ final class ByteArrayWindow extends ByteWindow { void inflateVerify(final byte[] array, final int pos, final Inflater inf) throws DataFormatException { - ensureLoaded(array); while (!inf.finished()) { if (inf.needsInput()) { inf.setInput(array, pos, array.length - pos); @@ -103,14 +100,25 @@ final class ByteArrayWindow extends ByteWindow { inf.inflate(verifyGarbageBuffer, 0, verifyGarbageBuffer.length); } - private synchronized void ensureLoaded(final byte[] array) { - if (!loaded) { - try { - provider.fd.getChannel().read(ByteBuffer.wrap(array), start); - } catch (IOException e) { - throw new RuntimeException("Cannot fault in window", e); + void ensureLoaded(final byte[] array) { + boolean release = false; + try { + synchronized (this) { + if (!loaded) { + release = true; + try { + provider.fd.getChannel().read(ByteBuffer.wrap(array), + start); + } catch (IOException e) { + throw new RuntimeException("Cannot fault in window", e); + } + loaded = true; + } + } + } finally { + if (release) { + WindowCache.markLoaded(this); } - loaded = true; } } } \ No newline at end of file diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java index a6757e86..5beb79be 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java @@ -107,4 +107,8 @@ final class ByteBufferWindow extends ByteWindow { while (!inf.finished() && !inf.needsInput()) inf.inflate(verifyGarbageBuffer, 0, verifyGarbageBuffer.length); } + + void ensureLoaded(final ByteBuffer ref) { + // Do nothing. + } } \ No newline at end of file diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java index 732664b6..e957138e 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java @@ -217,4 +217,6 @@ abstract class ByteWindow extends SoftReference { abstract void inflateVerify(T ref, int pos, Inflater inf) throws DataFormatException; + + abstract void ensureLoaded(T ref); } \ No newline at end of file diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java index 0b9d20c7..4b7e10dd 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java @@ -189,7 +189,13 @@ public class WindowCache { * the window was not found in the cache and the given provider * was unable to load the window on demand. */ - public static synchronized final void get(final WindowCursor curs, + public static final void get(final WindowCursor curs, + final WindowedFile wp, final long position) throws IOException { + getImpl(curs, wp, position); + curs.window.ensureLoaded(curs.handle); + } + + private static synchronized final void getImpl(final WindowCursor curs, final WindowedFile wp, final long position) throws IOException { final int id = (int) (position >> windowSizeShift); final int idx = hash(wp, id); @@ -254,6 +260,12 @@ public class WindowCache { insertLRU(e); } + static synchronized void markLoaded(final ByteWindow w) { + if (--w.provider.openCount == 0) { + w.provider.cacheClose(); + } + } + private static void makeMostRecent(ByteWindow e) { if (lruHead != e) { unlinkLRU(e); diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java index db8ea88b..938f44c0 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java @@ -348,5 +348,6 @@ public class WindowedFile { final byte[] b = new byte[size]; curs.window = new ByteArrayWindow(this, pos, windowId, b); curs.handle = b; + openCount++; // Until the window loads, we must stay open. } } -- 2.11.4.GIT