Shawn O. PearceBack 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 <spearce@spearce.org> ---
I've decided I hate WindowedFile, ByteWindow, WindowCache.
The API and implementation is crap. Its all my own fault. This code is mine, but its impossible to work with, not as concurrent as I'd like the cache to be, and there's problems getting it to be thread-safe, as this patch just pointed out. *sigh*
I think we should apply this, but really, someone needs to come back here and overhaul how we do our most low level IO ops. Its a crime that this code is in the state it is in.
Or maybe I'm just pissed off because I'm hunting down a stupid concurrency issue on a Saturday. .../src/org/spearce/jgit/lib/ByteArrayWindow.java | 7 +++---- .../src/org/spearce/jgit/lib/ByteBufferWindow.java | 4 ++++ .../src/org/spearce/jgit/lib/ByteWindow.java | 2 ++ .../src/org/spearce/jgit/lib/WindowCache.java | 12 +++++++++++- .../src/org/spearce/jgit/lib/WindowedFile.java | 1 + 5 files changed, 21 insertions(+), 5 deletions(-)
Show changes to 5 files +21 −5
org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java, org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java, org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java, org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java, org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
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 7b7c7a4..055a329 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 @@ ByteArrayWindow(final WindowedFile o, final long p, final int d, } 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 @@ int copy(final byte[] array, final int p, final byte[] b, final int o, int n) { 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 @@ int inflate(final byte[] array, final int pos, final byte[] b, int o, 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,12 +100,14 @@ void inflateVerify(final byte[] array, final int pos, final Inflater inf) inf.inflate(verifyGarbageBuffer, 0, verifyGarbageBuffer.length); } - private synchronized void ensureLoaded(final byte[] array) { + 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); + } finally { + WindowCache.markLoaded(this); } loaded = true; } 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 a6757e8..5beb79b 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 @@ void inflateVerify(final ByteBuffer buffer, final int pos, 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 732664b..e957138 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 @@ final void inflateVerify(T ref, long pos, Inflater inf) 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 0b9d20c..fc4bab8 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 @@ private static synchronized void reconfigureImpl(final int packedGitLimit, * 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,10 @@ public static synchronized final void get(final WindowCursor curs, insertLRU(e); } + static synchronized void markLoaded(final ByteWindow w) { + w.provider.openCount--; + } + 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 db8ea88..938f44c 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 @@ void allocWindow(final WindowCursor curs, final int windowId, 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. } }
-- 1.6.2.185.g8b635