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 <spearce@xxxxxxxxxxx>
---
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(-)
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
