{"thread":{"id":"18208","subject":"[JGIT PATCH] Fix a race condition when loading non-mmapd byte windows","startedAt":"2009-03-08T00:12:05Z","lastAt":"2009-03-08T01:24:15Z","messageCount":3,"participants":["Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"107360","messageId":"1236471125-21382-1-git-send-email-spearce@spearce.org","threadId":"18208","inReplyTo":null,"subject":"[JGIT PATCH] Fix a race condition when loading non-mmapd byte windows","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-03-08T00:12:05Z","receivedAt":"2009-03-08T00:12:05Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Back in 439d441b0800 (\"Change ByteArrayWindow to read content\noutside of WindowCache's lock\") I introduced a race condition\nto WindowCache.\n\nIt is possible for a ByteArrayWindow to be allocated and put\ninto the cache, and then for the calling thread to get put\nto sleep before it can actually load the window from disk.\nBy the time the thread wakes up again, the window may have\nalready been evicted from the cache, and its underlying file\nwas closed.  This results in stack traces such as:\n\njava.lang.RuntimeException: Cannot fault in window\n        at org.spearce.jgit.lib.ByteArrayWindow.ensureLoaded(ByteArrayWindow.java:111)\n...\nCaused by: java.nio.channels.ClosedChannelException\n        at sun.nio.ch.FileChannelImpl.ensureOpen(FileChannelImpl.java:91)\n\nNow when we allocate a ByteArrayWindow for a specific window reference\nwe increment the file reference count one additional time, forcing the\nfile to stay open even if the window was evicted from the cache.  The\nreference count is decremented once the window loads successfully, and\nthis puts us back into the state of 1 reference count value for each\nwindow still in the window cache.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n I've decided I hate WindowedFile, ByteWindow, WindowCache.\n\n The API and implementation is crap.  Its all my own fault.  This\n code is mine, but its impossible to work with, not as concurrent\n as I'd like the cache to be, and there's problems getting it to\n be thread-safe, as this patch just pointed out.  *sigh*\n\n I think we should apply this, but really, someone needs to come\n back here and overhaul how we do our most low level IO ops.\n Its a crime that this code is in the state it is in.\n\n Or maybe I'm just pissed off because I'm hunting down a stupid\n concurrency issue on a Saturday.\n \n .../src/org/spearce/jgit/lib/ByteArrayWindow.java  |    7 +++----\n .../src/org/spearce/jgit/lib/ByteBufferWindow.java |    4 ++++\n .../src/org/spearce/jgit/lib/ByteWindow.java       |    2 ++\n .../src/org/spearce/jgit/lib/WindowCache.java      |   12 +++++++++++-\n .../src/org/spearce/jgit/lib/WindowedFile.java     |    1 +\n 5 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java\nindex 7b7c7a4..055a329 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java\n@@ -68,7 +68,6 @@ ByteArrayWindow(final WindowedFile o, final long p, final int d,\n \t}\n \n \tint copy(final byte[] array, final int p, final byte[] b, final int o, int n) {\n-\t\tensureLoaded(array);\n \t\tn = Math.min(array.length - p, n);\n \t\tSystem.arraycopy(array, p, b, o, n);\n \t\treturn n;\n@@ -76,7 +75,6 @@ int copy(final byte[] array, final int p, final byte[] b, final int o, int n) {\n \n \tint inflate(final byte[] array, final int pos, final byte[] b, int o,\n \t\t\tfinal Inflater inf) throws DataFormatException {\n-\t\tensureLoaded(array);\n \t\twhile (!inf.finished()) {\n \t\t\tif (inf.needsInput()) {\n \t\t\t\tinf.setInput(array, pos, array.length - pos);\n@@ -91,7 +89,6 @@ int inflate(final byte[] array, final int pos, final byte[] b, int o,\n \n \tvoid inflateVerify(final byte[] array, final int pos, final Inflater inf)\n \t\t\tthrows DataFormatException {\n-\t\tensureLoaded(array);\n \t\twhile (!inf.finished()) {\n \t\t\tif (inf.needsInput()) {\n \t\t\t\tinf.setInput(array, pos, array.length - pos);\n@@ -103,12 +100,14 @@ void inflateVerify(final byte[] array, final int pos, final Inflater inf)\n \t\t\tinf.inflate(verifyGarbageBuffer, 0, verifyGarbageBuffer.length);\n \t}\n \n-\tprivate synchronized void ensureLoaded(final byte[] array) {\n+\tsynchronized void ensureLoaded(final byte[] array) {\n \t\tif (!loaded) {\n \t\t\ttry {\n \t\t\t\tprovider.fd.getChannel().read(ByteBuffer.wrap(array), start);\n \t\t\t} catch (IOException e) {\n \t\t\t\tthrow new RuntimeException(\"Cannot fault in window\", e);\n+\t\t\t} finally {\n+\t\t\t\tWindowCache.markLoaded(this);\n \t\t\t}\n \t\t\tloaded = true;\n \t\t}\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java\nindex a6757e8..5beb79b 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java\n@@ -107,4 +107,8 @@ void inflateVerify(final ByteBuffer buffer, final int pos,\n \t\twhile (!inf.finished() && !inf.needsInput())\n \t\t\tinf.inflate(verifyGarbageBuffer, 0, verifyGarbageBuffer.length);\n \t}\n+\n+\tvoid ensureLoaded(final ByteBuffer ref) {\n+\t\t// Do nothing.\n+\t}\n }\n\\ No newline at end of file\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java\nindex 732664b..e957138 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java\n@@ -217,4 +217,6 @@ final void inflateVerify(T ref, long pos, Inflater inf)\n \n \tabstract void inflateVerify(T ref, int pos, Inflater inf)\n \t\t\tthrows DataFormatException;\n+\n+\tabstract void ensureLoaded(T ref);\n }\n\\ No newline at end of file\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java\nindex 0b9d20c..fc4bab8 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java\n@@ -189,7 +189,13 @@ private static synchronized void reconfigureImpl(final int packedGitLimit,\n \t *             the window was not found in the cache and the given provider\n \t *             was unable to load the window on demand.\n \t */\n-\tpublic static synchronized final void get(final WindowCursor curs,\n+\tpublic static final void get(final WindowCursor curs,\n+\t\t\tfinal WindowedFile wp, final long position) throws IOException {\n+\t\tgetImpl(curs, wp, position);\n+\t\tcurs.window.ensureLoaded(curs.handle);\n+\t}\n+\n+\tprivate static synchronized final void getImpl(final WindowCursor curs,\n \t\t\tfinal WindowedFile wp, final long position) throws IOException {\n \t\tfinal int id = (int) (position >> windowSizeShift);\n \t\tfinal int idx = hash(wp, id);\n@@ -254,6 +260,10 @@ public static synchronized final void get(final WindowCursor curs,\n \t\tinsertLRU(e);\n \t}\n \n+\tstatic synchronized void markLoaded(final ByteWindow w) {\n+\t\tw.provider.openCount--;\n+\t}\n+\n \tprivate static void makeMostRecent(ByteWindow<?> e) {\n \t\tif (lruHead != e) {\n \t\t\tunlinkLRU(e);\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java\nindex db8ea88..938f44c 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java\n@@ -348,5 +348,6 @@ void allocWindow(final WindowCursor curs, final int windowId,\n \t\tfinal byte[] b = new byte[size];\n \t\tcurs.window = new ByteArrayWindow(this, pos, windowId, b);\n \t\tcurs.handle = b;\n+\t\topenCount++; // Until the window loads, we must stay open.\n \t}\n }\n-- \n1.6.2.185.g8b635\n"},{"id":"107361","messageId":"1236471435-21888-1-git-send-email-spearce@spearce.org","threadId":"18208","inReplyTo":"1236471125-21382-1-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH v2] Fix a race condition when loading non-mmapd byte windows","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-03-08T00:17:15Z","receivedAt":"2009-03-08T00:17:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Back in 439d441b0800 (\"Change ByteArrayWindow to read content\noutside of WindowCache's lock\") I introduced a race condition\nto WindowCache.\n\nIt is possible for a ByteArrayWindow to be allocated and put\ninto the cache, and then for the calling thread to get put\nto sleep before it can actually load the window from disk.\nBy the time the thread wakes up again, the window may have\nalready been evicted from the cache, and its underlying file\nwas closed.  This results in stack traces such as:\n\njava.lang.RuntimeException: Cannot fault in window\n        at org.spearce.jgit.lib.ByteArrayWindow.ensureLoaded(ByteArrayWindow.java:111)\n...\nCaused by: java.nio.channels.ClosedChannelException\n        at sun.nio.ch.FileChannelImpl.ensureOpen(FileChannelImpl.java:91)\n\nNow when we allocate a ByteArrayWindow for a specific window reference\nwe increment the file reference count one additional time, forcing the\nfile to stay open even if the window was evicted from the cache.  The\nreference count is decremented once the window loads successfully, and\nthis puts us back into the state of 1 reference count value for each\nwindow still in the window cache.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n Oh hell, I missed the fact that markLoaded(ByteWindow) may be\n dropping the last reference and need to close the file.\n\n .../src/org/spearce/jgit/lib/ByteArrayWindow.java  |    7 +++----\n .../src/org/spearce/jgit/lib/ByteBufferWindow.java |    4 ++++\n .../src/org/spearce/jgit/lib/ByteWindow.java       |    2 ++\n .../src/org/spearce/jgit/lib/WindowCache.java      |   14 +++++++++++++-\n .../src/org/spearce/jgit/lib/WindowedFile.java     |    1 +\n 5 files changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java\nindex 7b7c7a4..055a329 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java\n@@ -68,7 +68,6 @@ ByteArrayWindow(final WindowedFile o, final long p, final int d,\n \t}\n \n \tint copy(final byte[] array, final int p, final byte[] b, final int o, int n) {\n-\t\tensureLoaded(array);\n \t\tn = Math.min(array.length - p, n);\n \t\tSystem.arraycopy(array, p, b, o, n);\n \t\treturn n;\n@@ -76,7 +75,6 @@ int copy(final byte[] array, final int p, final byte[] b, final int o, int n) {\n \n \tint inflate(final byte[] array, final int pos, final byte[] b, int o,\n \t\t\tfinal Inflater inf) throws DataFormatException {\n-\t\tensureLoaded(array);\n \t\twhile (!inf.finished()) {\n \t\t\tif (inf.needsInput()) {\n \t\t\t\tinf.setInput(array, pos, array.length - pos);\n@@ -91,7 +89,6 @@ int inflate(final byte[] array, final int pos, final byte[] b, int o,\n \n \tvoid inflateVerify(final byte[] array, final int pos, final Inflater inf)\n \t\t\tthrows DataFormatException {\n-\t\tensureLoaded(array);\n \t\twhile (!inf.finished()) {\n \t\t\tif (inf.needsInput()) {\n \t\t\t\tinf.setInput(array, pos, array.length - pos);\n@@ -103,12 +100,14 @@ void inflateVerify(final byte[] array, final int pos, final Inflater inf)\n \t\t\tinf.inflate(verifyGarbageBuffer, 0, verifyGarbageBuffer.length);\n \t}\n \n-\tprivate synchronized void ensureLoaded(final byte[] array) {\n+\tsynchronized void ensureLoaded(final byte[] array) {\n \t\tif (!loaded) {\n \t\t\ttry {\n \t\t\t\tprovider.fd.getChannel().read(ByteBuffer.wrap(array), start);\n \t\t\t} catch (IOException e) {\n \t\t\t\tthrow new RuntimeException(\"Cannot fault in window\", e);\n+\t\t\t} finally {\n+\t\t\t\tWindowCache.markLoaded(this);\n \t\t\t}\n \t\t\tloaded = true;\n \t\t}\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java\nindex a6757e8..5beb79b 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteBufferWindow.java\n@@ -107,4 +107,8 @@ void inflateVerify(final ByteBuffer buffer, final int pos,\n \t\twhile (!inf.finished() && !inf.needsInput())\n \t\t\tinf.inflate(verifyGarbageBuffer, 0, verifyGarbageBuffer.length);\n \t}\n+\n+\tvoid ensureLoaded(final ByteBuffer ref) {\n+\t\t// Do nothing.\n+\t}\n }\n\\ No newline at end of file\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java\nindex 732664b..e957138 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteWindow.java\n@@ -217,4 +217,6 @@ final void inflateVerify(T ref, long pos, Inflater inf)\n \n \tabstract void inflateVerify(T ref, int pos, Inflater inf)\n \t\t\tthrows DataFormatException;\n+\n+\tabstract void ensureLoaded(T ref);\n }\n\\ No newline at end of file\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java\nindex 0b9d20c..4b7e10d 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java\n@@ -189,7 +189,13 @@ private static synchronized void reconfigureImpl(final int packedGitLimit,\n \t *             the window was not found in the cache and the given provider\n \t *             was unable to load the window on demand.\n \t */\n-\tpublic static synchronized final void get(final WindowCursor curs,\n+\tpublic static final void get(final WindowCursor curs,\n+\t\t\tfinal WindowedFile wp, final long position) throws IOException {\n+\t\tgetImpl(curs, wp, position);\n+\t\tcurs.window.ensureLoaded(curs.handle);\n+\t}\n+\n+\tprivate static synchronized final void getImpl(final WindowCursor curs,\n \t\t\tfinal WindowedFile wp, final long position) throws IOException {\n \t\tfinal int id = (int) (position >> windowSizeShift);\n \t\tfinal int idx = hash(wp, id);\n@@ -254,6 +260,12 @@ public static synchronized final void get(final WindowCursor curs,\n \t\tinsertLRU(e);\n \t}\n \n+\tstatic synchronized void markLoaded(final ByteWindow w) {\n+\t\tif (--w.provider.openCount == 0) {\n+\t\t\tw.provider.cacheClose();\n+\t\t}\n+\t}\n+\n \tprivate static void makeMostRecent(ByteWindow<?> e) {\n \t\tif (lruHead != e) {\n \t\t\tunlinkLRU(e);\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java\nindex db8ea88..938f44c 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java\n@@ -348,5 +348,6 @@ void allocWindow(final WindowCursor curs, final int windowId,\n \t\tfinal byte[] b = new byte[size];\n \t\tcurs.window = new ByteArrayWindow(this, pos, windowId, b);\n \t\tcurs.handle = b;\n+\t\topenCount++; // Until the window loads, we must stay open.\n \t}\n }\n-- \n1.6.2.185.g8b635\n"},{"id":"107367","messageId":"20090308012415.GT16213@spearce.org","threadId":"18208","inReplyTo":"1236471435-21888-1-git-send-email-spearce@spearce.org","subject":"Re: [JGIT PATCH v2] Fix a race condition when loading non-mmapd byte windows","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-03-08T01:24:15Z","receivedAt":"2009-03-08T01:24:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n>  Oh hell, I missed the fact that markLoaded(ByteWindow) may be\n>  dropping the last reference and need to close the file.\n\n*and* you need to squash this in to prevent a deadlock:\n\nFrell.  What a Saturday.\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java\nindex 055a329..c07a260 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ByteArrayWindow.java\n@@ -100,16 +100,25 @@ void inflateVerify(final byte[] array, final int pos, final Inflater inf)\n \t\t\tinf.inflate(verifyGarbageBuffer, 0, verifyGarbageBuffer.length);\n \t}\n \n-\tsynchronized void ensureLoaded(final byte[] array) {\n-\t\tif (!loaded) {\n-\t\t\ttry {\n-\t\t\t\tprovider.fd.getChannel().read(ByteBuffer.wrap(array), start);\n-\t\t\t} catch (IOException e) {\n-\t\t\t\tthrow new RuntimeException(\"Cannot fault in window\", e);\n-\t\t\t} finally {\n+\tvoid ensureLoaded(final byte[] array) {\n+\t\tboolean release = false;\n+\t\ttry {\n+\t\t\tsynchronized (this) {\n+\t\t\t\tif (!loaded) {\n+\t\t\t\t\trelease = true;\n+\t\t\t\t\ttry {\n+\t\t\t\t\t\tprovider.fd.getChannel().read(ByteBuffer.wrap(array),\n+\t\t\t\t\t\t\t\tstart);\n+\t\t\t\t\t} catch (IOException e) {\n+\t\t\t\t\t\tthrow new RuntimeException(\"Cannot fault in window\", e);\n+\t\t\t\t\t}\n+\t\t\t\t\tloaded = true;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t} finally {\n+\t\t\tif (release) {\n \t\t\t\tWindowCache.markLoaded(this);\n \t\t\t}\n-\t\t\tloaded = true;\n \t\t}\n \t}\n }\n\\ No newline at end of file\n\n-- \nShawn.\n"}]}