Index: lucene/src/java/org/apache/lucene/store/MMapDirectory.java =================================================================== --- lucene/src/java/org/apache/lucene/store/MMapDirectory.java (revision 1205157) +++ lucene/src/java/org/apache/lucene/store/MMapDirectory.java (working copy) @@ -26,6 +26,9 @@ import java.nio.channels.FileChannel; import java.nio.channels.FileChannel.MapMode; +import java.util.Map; +import java.util.WeakHashMap; + import java.security.AccessController; import java.security.PrivilegedExceptionAction; import java.security.PrivilegedActionException; @@ -256,6 +259,7 @@ private ByteBuffer curBuf; // redundant for speed: buffers[curBufIndex] private boolean isClone = false; + private final Map clones = new WeakHashMap(); MMapIndexInput(String resourceDescription, RandomAccessFile raf, long offset, long length, int chunkSizePower) throws IOException { super(resourceDescription); @@ -304,6 +308,8 @@ curBuf.position(0); } while (!curBuf.hasRemaining()); return curBuf.get(); + } catch (NullPointerException npe) { + throw new AlreadyClosedException("MMapIndexInput already closed: " + this); } } @@ -326,6 +332,8 @@ curAvail = curBuf.remaining(); } curBuf.get(b, offset, len); + } catch (NullPointerException npe) { + throw new AlreadyClosedException("MMapIndexInput already closed: " + this); } } @@ -335,6 +343,8 @@ return curBuf.getShort(); } catch (BufferUnderflowException e) { return super.readShort(); + } catch (NullPointerException npe) { + throw new AlreadyClosedException("MMapIndexInput already closed: " + this); } } @@ -344,6 +354,8 @@ return curBuf.getInt(); } catch (BufferUnderflowException e) { return super.readInt(); + } catch (NullPointerException npe) { + throw new AlreadyClosedException("MMapIndexInput already closed: " + this); } } @@ -353,6 +365,8 @@ return curBuf.getLong(); } catch (BufferUnderflowException e) { return super.readLong(); + } catch (NullPointerException npe) { + throw new AlreadyClosedException("MMapIndexInput already closed: " + this); } } @@ -381,6 +395,8 @@ throw new IllegalArgumentException("Seeking to negative position: " + this); } throw new IOException("seek past EOF: " + this); + } catch (NullPointerException npe) { + throw new AlreadyClosedException("MMapIndexInput already closed: " + this); } } @@ -396,9 +412,9 @@ } final MMapIndexInput clone = (MMapIndexInput)super.clone(); clone.isClone = true; + // we keep clone.clones, so it shares the same map with original and we have no additional cost on clones + assert clone.clones == this.clones; clone.buffers = new ByteBuffer[buffers.length]; - // Since most clones will use only one buffer, duplicate() could also be - // done lazy in clones, e.g. when adapting curBuf. for (int bufNr = 0; bufNr < buffers.length; bufNr++) { clone.buffers[bufNr] = buffers[bufNr].duplicate(); } @@ -407,26 +423,55 @@ } catch(IOException ioe) { throw new RuntimeException("Should never happen: " + this, ioe); } + + // register the new clone in our clone list to clean it up on closing: + synchronized(this.clones) { + this.clones.put(clone, Boolean.TRUE); + } + return clone; } + + private void unsetBuffers() { + buffers = null; + curBuf = null; + curBufIndex = 0; + } @Override public void close() throws IOException { - curBuf = null; curBufIndex = 0; try { if (isClone || buffers == null) return; - for (int bufNr = 0; bufNr < buffers.length; bufNr++) { - // unmap the buffer (if enabled) and at least unset it for GC - try { - cleanMapping(buffers[bufNr]); - } finally { - buffers[bufNr] = null; + + // for extra safety unset also all clones' buffers: + synchronized(this.clones) { + for (final MMapIndexInput clone : this.clones.keySet()) { + assert clone.isClone; + clone.unsetBuffers(); } + this.clones.clear(); } + + curBuf = null; curBufIndex = 0; // nuke curr pointer early + for (int bufNr = 0; bufNr < buffers.length; bufNr++) { + cleanMapping(buffers[bufNr]); + } } finally { - buffers = null; + unsetBuffers(); } } + + // make sure we have identity on equals/hashCode for WeakHashMap + @Override + public int hashCode() { + return System.identityHashCode(this); + } + + // make sure we have identity on equals/hashCode for WeakHashMap + @Override + public boolean equals(Object obj) { + return obj == this; + } } } Index: lucene/src/test/org/apache/lucene/store/TestMultiMMap.java =================================================================== --- lucene/src/test/org/apache/lucene/store/TestMultiMMap.java (revision 1205157) +++ lucene/src/test/org/apache/lucene/store/TestMultiMMap.java (working copy) @@ -18,6 +18,7 @@ */ import java.io.File; +import java.io.IOException; import java.util.Random; import org.apache.lucene.analysis.MockAnalyzer; @@ -47,6 +48,35 @@ workDir = _TestUtil.getTempDir("TestMultiMMap"); workDir.mkdirs(); } + + public void testCloneSafety() throws Exception { + MMapDirectory mmapDir = new MMapDirectory(_TestUtil.getTempDir("testCloneSafety")); + IndexOutput io = mmapDir.createOutput("bytes", newIOContext(random)); + io.writeVInt(5); + io.close(); + IndexInput one = mmapDir.openInput("bytes", IOContext.DEFAULT); + IndexInput two = (IndexInput) one.clone(); + IndexInput three = (IndexInput) two.clone(); // clone of clone + one.close(); + try { + one.readVInt(); + fail("Must throw AlreadyClosedException"); + } catch (AlreadyClosedException ignore) { + // pass + } + try { + two.readVInt(); + fail("Must throw AlreadyClosedException"); + } catch (AlreadyClosedException ignore) { + // pass + } + try { + three.readVInt(); + fail("Must throw AlreadyClosedExveption"); + } catch (AlreadyClosedException ignore) { + // pass + } + } public void testSeekZero() throws Exception { for (int i = 0; i < 31; i++) {