LUCENE-5612: fix NativeFSLockFactory to pass LockStressTest

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1588870 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -299,6 +299,11 @@ Bug fixes
 * LUCENE-5615: Validate per-segment delete counts at write time, to
   help catch bugs that might otherwise cause corruption (Mike McCandless)
 
+* LUCENE-5612: NativeFSLockFactory no longer deletes its lock file. This cannot be done
+  safely without the risk of deleting someone else's lock file. If you use NativeFSLockFactory,
+  you may see write.lock hanging around from time to time: its harmless.  
+  (Uwe Schindler, Mike McCandless, Robert Muir)
+
 Test Framework
 
 * LUCENE-5592: Incorrectly reported uncloseable files. (Dawid Weiss)

lucene/core/build.xml

@@ -169,7 +169,15 @@
     </sequential>
   </macrodef>
   
-  <target name="test-lock-factory" depends="resolve-groovy,compile-core">
+  <!-- we ignore our ant-based lock factory test, if user applies test filtering: -->
+  <condition property="-ignore-test-lock-factory">
+    <or>
+      <isset property="tests.class" />
+      <isset property="tests.method" />
+    </or>
+  </condition>
+  
+  <target name="test-lock-factory" depends="resolve-groovy,compile-core" unless="-ignore-test-lock-factory">
     <property name="lockverifyserver.host" value="127.0.0.1"/>
     <property name="lock.factory.impl" value="org.apache.lucene.store.NativeFSLockFactory"/>
     <property name="lock.factory.dir" location="${build.dir}/lockfactorytest"/>
@@ -211,8 +219,6 @@
     </parallel>
   </target>
   
-  <!-- once we fixed LUCENE-5612, reenable this:
   <target name="test" depends="common.test, test-lock-factory"/>
-  -->
 
 </project>

lucene/core/src/java/org/apache/lucene/store/LockStressTest.java

@@ -25,8 +25,6 @@ import java.net.InetSocketAddress;
 import java.net.Socket;
 import java.util.Random;
 
-import org.apache.lucene.util.IOUtils;
-
 /**
  * Simple standalone tool that forever acquires & releases a
  * lock using a specific LockFactory.  Run without any args

lucene/core/src/java/org/apache/lucene/store/NativeFSLockFactory.java

@@ -19,10 +19,12 @@ package org.apache.lucene.store;
 
 import java.nio.channels.FileChannel;
 import java.nio.channels.FileLock;
+import java.nio.channels.OverlappingFileLockException;
+import java.nio.file.StandardOpenOption;
 import java.io.File;
-import java.io.RandomAccessFile;
 import java.io.IOException;
-import java.util.HashSet;
+
+import org.apache.lucene.util.IOUtils;
 
 /**
  * <p>Implements {@link LockFactory} using native OS file
@@ -96,58 +98,17 @@ public class NativeFSLockFactory extends FSLockFactory {
 
   @Override
   public void clearLock(String lockName) throws IOException {
-    // Note that this isn't strictly required anymore
-    // because the existence of these files does not mean
-    // they are locked, but, still do this in case people
-    // really want to see the files go away:
-    if (lockDir.exists()) {
-      
-      // Try to release the lock first - if it's held by another process, this
-      // method should not silently fail.
-      // NOTE: makeLock fixes the lock name by prefixing it w/ lockPrefix.
-      // Therefore it should be called before the code block next which prefixes
-      // the given name.
     makeLock(lockName).close();
-
-      if (lockPrefix != null) {
-        lockName = lockPrefix + "-" + lockName;
-      }
-      
-      // As mentioned above, we don't care if the deletion of the file failed.
-      new File(lockDir, lockName).delete();
-    }
   }
 }
 
 class NativeFSLock extends Lock {
 
-  private RandomAccessFile f;
   private FileChannel channel;
   private FileLock lock;
   private File path;
   private File lockDir;
 
-  /*
-   * The javadocs for FileChannel state that you should have
-   * a single instance of a FileChannel (per JVM) for all
-   * locking against a given file (locks are tracked per 
-   * FileChannel instance in Java 1.4/1.5). Even using the same 
-   * FileChannel instance is not completely thread-safe with Java 
-   * 1.4/1.5 though. To work around this, we have a single (static) 
-   * HashSet that contains the file paths of all currently 
-   * locked locks.  This protects against possible cases 
-   * where different Directory instances in one JVM (each 
-   * with their own NativeFSLockFactory instance) have set 
-   * the same lock dir and lock prefix. However, this will not 
-   * work when LockFactorys are created by different 
-   * classloaders (eg multiple webapps). 
-   * 
-   * TODO: Java 1.6 tracks system wide locks in a thread safe manner 
-   * (same FileChannel instance or not), so we may want to 
-   * change this when Lucene moves to Java 1.6.
-   */
-  private static HashSet<String> LOCK_HELD = new HashSet<>();
-
   public NativeFSLock(File lockDir, String lockFileName) {
     this.lockDir = lockDir;
     path = new File(lockDir, lockFileName);
@@ -176,46 +137,12 @@ class NativeFSLock extends Lock {
           lockDir.getAbsolutePath());
     }
     
-    String canonicalPath = path.getCanonicalPath();
+    channel = FileChannel.open(path.toPath(), StandardOpenOption.CREATE, StandardOpenOption.WRITE);
-
+    boolean success = false;
-    boolean markedHeld = false;
-
-    try {
-
-      // Make sure nobody else in-process has this lock held
-      // already, and, mark it held if not:
-
-      synchronized(LOCK_HELD) {
-        if (LOCK_HELD.contains(canonicalPath)) {
-          // Someone else in this JVM already has the lock:
-          return false;
-        } else {
-          // This "reserves" the fact that we are the one
-          // thread trying to obtain this lock, so we own
-          // the only instance of a channel against this
-          // file:
-          LOCK_HELD.add(canonicalPath);
-          markedHeld = true;
-        }
-      }
-
-      try {
-        f = new RandomAccessFile(path, "rw");
-      } catch (IOException e) {
-        // On Windows, we can get intermittent "Access
-        // Denied" here.  So, we treat this as failure to
-        // acquire the lock, but, store the reason in case
-        // there is in fact a real error case.
-        failureReason = e;
-        f = null;
-      }
-
-      if (f != null) {
-        try {
-          channel = f.getChannel();
     try {
       lock = channel.tryLock();
-          } catch (IOException e) {
+      success = true;
+    } catch (IOException | OverlappingFileLockException e) {
       // At least on OS X, we will sometimes get an
       // intermittent "Permission Denied" IOException,
       // which seems to simply mean "you failed to get
@@ -227,34 +154,14 @@ class NativeFSLock extends Lock {
       // if it fails to get the lock.
       failureReason = e;
     } finally {
-            if (lock == null) {
+      if (!success) {
         try {
-                channel.close();
+          IOUtils.closeWhileHandlingException(channel);
         } finally {
           channel = null;
         }
       }
     }
-        } finally {
-          if (channel == null) {
-            try {
-              f.close();
-            } finally {
-              f = null;
-            }
-          }
-        }
-      }
-
-    } finally {
-      if (markedHeld && !lockExists()) {
-        synchronized(LOCK_HELD) {
-          if (LOCK_HELD.contains(canonicalPath)) {
-            LOCK_HELD.remove(canonicalPath);
-          }
-        }
-      }
-    }
     return lockExists();
   }
 
@@ -269,20 +176,8 @@ class NativeFSLock extends Lock {
           channel.close();
         } finally {
           channel = null;
-          try {
-            f.close();
-          } finally {
-            f = null;
-            synchronized(LOCK_HELD) {
-              LOCK_HELD.remove(path.getCanonicalPath());
         }
       }
-        }
-      }
-      // LUCENE-2421: we don't care anymore if the file cannot be deleted
-      // because it's held up by another process (e.g. AntiVirus). NativeFSLock
-      // does not depend on the existence/absence of the lock file
-      path.delete();
     } else {
       // if we don't hold the lock, and somebody still called release(), for
       // example as a result of calling IndexWriter.unlock(), we should attempt

lucene/core/src/java/org/apache/lucene/store/VerifyingLockFactory.java

@@ -17,7 +17,6 @@ package org.apache.lucene.store;
  * limitations under the License.
  */
 
-import java.net.Socket;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;

lucene/core/src/test/org/apache/lucene/index/TestAllFilesHaveChecksumFooter.java

@@ -67,6 +67,9 @@ public class TestAllFilesHaveChecksumFooter extends LuceneTestCase {
   
   private void checkHeaders(Directory dir) throws IOException {
     for (String file : dir.listAll()) {
+      if (file.equals(IndexWriter.WRITE_LOCK_NAME)) {
+        continue; // write.lock has no footer, thats ok
+      }
       if (file.endsWith(IndexFileNames.COMPOUND_FILE_EXTENSION)) {
         CompoundFileDirectory cfsDir = new CompoundFileDirectory(dir, file, newIOContext(random()), false);
         checkHeaders(cfsDir); // recurse into cfs

lucene/core/src/test/org/apache/lucene/index/TestAllFilesHaveCodecHeader.java

@@ -68,6 +68,9 @@ public class TestAllFilesHaveCodecHeader extends LuceneTestCase {
   
   private void checkHeaders(Directory dir) throws IOException {
     for (String file : dir.listAll()) {
+      if (file.equals(IndexWriter.WRITE_LOCK_NAME)) {
+        continue; // write.lock has no header, thats ok
+      }
       if (file.equals(IndexFileNames.SEGMENTS_GEN)) {
         continue; // segments.gen has no header, thats ok
       }

lucene/core/src/test/org/apache/lucene/index/TestBackwardsCompatibility.java

@@ -641,85 +641,6 @@ public class TestBackwardsCompatibility extends LuceneTestCase {
     return indexDir;
   }
 
-  /* Verifies that the expected file names were produced */
-
-  public void testExactFileNames() throws IOException {
-
-    String outputDirName = "lucene.backwardscompat0.index";
-    File outputDir = createTempDir(outputDirName);
-    TestUtil.rm(outputDir);
-
-    try {
-      Directory dir = newFSDirectory(outputDir);
-
-      MergePolicy mergePolicy = newLogMergePolicy(true, 10);
-      
-      // This test expects all of its segments to be in CFS:
-      mergePolicy.setNoCFSRatio(1.0); 
-      mergePolicy.setMaxCFSSegmentSizeMB(Double.POSITIVE_INFINITY);
-
-      IndexWriter writer = new IndexWriter(
-          dir,
-          newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random())).
-              setMaxBufferedDocs(-1).
-              setRAMBufferSizeMB(16.0).
-              setMergePolicy(mergePolicy).setUseCompoundFile(true)
-      );
-      for(int i=0;i<35;i++) {
-        addDoc(writer, i);
-      }
-      assertEquals("wrong doc count", 35, writer.maxDoc());
-      writer.shutdown();
-
-      // Delete one doc so we get a .del file:
-      writer = new IndexWriter(
-          dir,
-          newIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(random()))
-            .setMergePolicy(NoMergePolicy.NO_COMPOUND_FILES).setUseCompoundFile(true)
-      );
-      Term searchTerm = new Term("id", "7");
-      writer.deleteDocuments(searchTerm);
-      writer.shutdown();
-
-      // Now verify file names... TODO: fix this test better, we could populate from 
-      // separateFiles() or something.
-      String[] expected = new String[] {"_0.cfs", "_0.cfe",
-                                        "_0_1.del",
-                                        "_0.si",
-                                        "segments_2",
-                                        "segments.gen"};
-      
-      String[] expectedSimpleText = new String[] {"_0.cfs", "_0.cfe",
-                                                  "_0_1.liv",
-                                                  "_0.si",
-                                                  "segments_2",
-                                                  "segments.gen"};
-
-      String[] actual = dir.listAll();
-      Arrays.sort(expected);
-      Arrays.sort(expectedSimpleText);
-      Arrays.sort(actual);
-      if (!Arrays.equals(expected, actual) && !Arrays.equals(expectedSimpleText, actual)) {
-        fail("incorrect filenames in index: expected:\n    " + asString(expected) 
-            + "\n or " + asString(expectedSimpleText) + "\n actual:\n    " + asString(actual));
-      }
-      dir.close();
-    } finally {
-      TestUtil.rm(outputDir);
-    }
-  }
-
-  private String asString(String[] l) {
-    String s = "";
-    for(int i=0;i<l.length;i++) {
-      if (i > 0) {
-        s += "\n    ";
-      }
-      s += l[i];
-    }
-    return s;
-  }
-
   private void addDoc(IndexWriter writer, int id) throws IOException
   {
     Document doc = new Document();

lucene/core/src/test/org/apache/lucene/index/TestIndexWriter.java

@@ -1544,11 +1544,13 @@ public class TestIndexWriter extends LuceneTestCase {
 
     // After rollback, IW should remove all files
     writer.rollback();
-    assertEquals("no files should exist in the directory after rollback", 0, dir.listAll().length);
+    String allFiles[] = dir.listAll();
+    assertTrue("no files should exist in the directory after rollback", allFiles.length == 0 || Arrays.equals(allFiles, new String[] { IndexWriter.WRITE_LOCK_NAME }));
 
     // Since we rolled-back above, that close should be a no-op
     writer.close();
-    assertEquals("expected a no-op close after IW.rollback()", 0, dir.listAll().length);
+    allFiles = dir.listAll();
+    assertTrue("expected a no-op close after IW.rollback()", allFiles.length == 0 || Arrays.equals(allFiles, new String[] { IndexWriter.WRITE_LOCK_NAME }));
     dir.close();
   }
 

lucene/core/src/test/org/apache/lucene/index/TestIndexWriterExceptions.java

@@ -22,6 +22,7 @@ import java.io.IOException;
 import java.io.StringReader;
 import java.nio.file.NoSuchFileException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Random;
@@ -939,7 +940,8 @@ public class TestIndexWriterExceptions extends LuceneTestCase {
       }
       assertTrue(failure.failOnCommit && failure.failOnDeleteFile);
       w.rollback();
-      assertEquals(0, dir.listAll().length);
+      String files[] = dir.listAll();
+      assertTrue(files.length == 0 || Arrays.equals(files, new String[] { IndexWriter.WRITE_LOCK_NAME }));
       dir.close();
     }
   }

lucene/test-framework/src/java/org/apache/lucene/index/BaseIndexFileFormatTestCase.java

@@ -18,9 +18,11 @@ package org.apache.lucene.index;
  */
 
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
 
 import org.apache.lucene.analysis.MockAnalyzer;
@@ -71,10 +73,13 @@ abstract class BaseIndexFileFormatTestCase extends LuceneTestCase {
    * comparing indices that store the same content.
    */
   protected Collection<String> excludedExtensionsFromByteCounts() {
+    return new HashSet<String>(Arrays.asList(new String[] { 
     // segment infos store various pieces of information that don't solely depend
     // on the content of the index in the diagnostics (such as a timestamp) so we
     // exclude this file from the bytes counts
-    return Collections.singleton("si");
+                        "si", 
+    // lock files are 0 bytes (one directory in the test could be RAMDir, the other FSDir)
+                        "lock" }));
   }
 
   /** The purpose of this test is to make sure that bulk merge doesn't accumulate useless data over runs. */

solr/core/src/test/org/apache/solr/core/SolrCoreCheckLockOnStartupTest.java

@@ -38,15 +38,10 @@ public class SolrCoreCheckLockOnStartupTest extends SolrTestCaseJ4 {
     super.setUp();
 
     System.setProperty("solr.directoryFactory", "org.apache.solr.core.SimpleFSDirectoryFactory");
-
+    // test tests native and simple in the same jvm in the same exact directory:
-    IndexWriterConfig indexWriterConfig = new IndexWriterConfig(TEST_VERSION_CURRENT, null);
+    // the file will remain after the native test (it cannot safely be deleted without the risk of deleting another guys lock)
-    Directory directory = newFSDirectory(createTempDir("index"));
+    // its ok, these aren't "compatible" anyway: really this test should not re-use the same directory at all.
-    //creates a new index on the known location
+    new File(new File(initCoreDataDir, "index"), IndexWriter.WRITE_LOCK_NAME).delete();
-    new IndexWriter(
-        directory,
-        indexWriterConfig.setOpenMode(IndexWriterConfig.OpenMode.CREATE)
-    ).shutdown();
-    directory.close();
   }
 
   @Test