docs/attachments/LUCENE-3588/LUCENE-3588.patch

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<MMapIndexInput,Boolean> clones = new WeakHashMap<MMapIndexInput,Boolean>();
 
     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++) {