LUCENE-5624: fix NativeFS file handle leak, improve lock testing

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

lucene/CHANGES.txt

@@ -304,6 +304,9 @@ Bug fixes
   you may see write.lock hanging around from time to time: its harmless.  
   (Uwe Schindler, Mike McCandless, Robert Muir)
 
+* LUCENE-5624: Ensure NativeFSLockFactory does not leak file handles if it is unable
+  to obtain the lock. (Uwe Schindler, Robert Muir)
+
 Test Framework
 
 * LUCENE-5592: Incorrectly reported uncloseable files. (Dawid Weiss)
@@ -331,9 +334,7 @@ Build
   by adding a workaround for the Ant bug.  (Uwe Schindler)
 
 * LUCENE-5612: Add a new Ant target in lucene/core to test LockFactory
-  implementations: "ant test-lock-factory". By default it is ran
-  after the core testsuite on NativeFSLockFactory. To test another one,
-  pass -Dlock.factory.impl to Ant.  (Uwe Schindler, Mike McCandless,
+  implementations: "ant test-lock-factory".  (Uwe Schindler, Mike McCandless,
   Robert Muir)
 
 Documentation

lucene/core/build.xml

@@ -148,6 +148,8 @@
   
   <macrodef name="startLockStressTestClient">
     <attribute name="clientId"/>
+    <attribute name="lockFactoryImpl"/>
+    <attribute name="lockFactoryDir"/>
     <sequential>
       <local name="lockverifyserver.port"/>
       <groovy><![CDATA[
@@ -157,18 +159,42 @@
         }
         properties["lockverifyserver.port"] = port;
       ]]></groovy>
-      <java taskname="LockStressTest@{clientId}" fork="true" classpathref="test-lock.classpath" classname="org.apache.lucene.store.LockStressTest" failOnError="true"> 
+      <java taskname="lockStressTest@{clientId}" fork="true" classpathref="test-lock.classpath" classname="org.apache.lucene.store.LockStressTest" failOnError="true"> 
         <arg value="@{clientId}"/>
         <arg value="${lockverifyserver.host}"/>
         <arg value="${lockverifyserver.port}"/>
-        <arg value="${lock.factory.impl}"/>
-        <arg value="${lock.factory.dir}"/>
+        <arg value="@{lockFactoryImpl}"/>
+        <arg value="@{lockFactoryDir}"/>
         <arg value="${lockverify.delay}"/>
         <arg value="${lockverify.count}"/>
       </java>
     </sequential>
   </macrodef>
   
+  <macrodef name="testLockFactory">
+    <attribute name="lockFactoryImpl"/>
+    <attribute name="lockFactoryDir"/>
+    <sequential>
+      <echo taskname="testLockFactory" message="Testing @{lockFactoryImpl}..."/>
+      <mkdir dir="@{lockFactoryDir}"/>
+      <parallel threadCount="3" failonany="false">
+        <sequential>
+          <!-- the server runs in-process, so we can wait for the sysproperty -->
+          <java taskname="lockVerifyServer" fork="false" classpathref="test-lock.classpath" classname="org.apache.lucene.store.LockVerifyServer" failOnError="true">
+            <arg value="${lockverifyserver.host}"/>
+            <arg value="2"/>
+          </java>
+        </sequential>
+        <sequential>
+          <startLockStressTestClient clientId="1" lockFactoryImpl="@{lockFactoryImpl}" lockFactoryDir="@{lockFactoryDir}" />
+        </sequential>
+        <sequential>
+          <startLockStressTestClient clientId="2" lockFactoryImpl="@{lockFactoryImpl}" lockFactoryDir="@{lockFactoryDir}" />
+        </sequential>
+      </parallel>
+    </sequential>
+  </macrodef>
+  
   <!-- we ignore our ant-based lock factory test, if user applies test filtering: -->
   <condition property="-ignore-test-lock-factory">
     <or>
@@ -180,21 +206,19 @@
   <target name="test-lock-factory" depends="resolve-groovy,compile-core" unless="-ignore-test-lock-factory"
     description="Run LockStressTest with multiple JVMs">
     <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"/>
     <property name="lockverify.delay" value="1"/>
-    <groovy taskname="LockVerifySetup"><![CDATA[
+    <groovy taskname="lockVerifySetup"><![CDATA[
       System.clearProperty("lockverifyserver.port"); // make sure it is undefined
       
       if (!properties["lockverify.count"]) {
         int count = Boolean.parseBoolean(properties["tests.nightly"]) ?
-          20000 : 2000;
+          30000 : 2000;
         count *= Integer.parseInt(properties["tests.multiplier"]);
         properties["lockverify.count"] = count;
       }
       
       task.log("Configuration properties:");
-      ["lock.factory.impl", "lockverify.delay", "lockverify.count"].each {
+      ["lockverify.delay", "lockverify.count"].each {
         k -> task.log(" " + k + "=" + properties[k]);
       }
     ]]></groovy>
@@ -202,22 +226,8 @@
       <path refid="classpath"/>
       <pathelement location="${build.dir}/classes/java"/>
     </path>
-    <mkdir dir="${lock.factory.dir}"/>
-    <parallel threadCount="3" failonany="false">
-      <sequential>
-        <!-- the server runs in-process, so we can wait for the sysproperty -->
-        <java taskname="LockVerifyServer" fork="false" classpathref="test-lock.classpath" classname="org.apache.lucene.store.LockVerifyServer" failOnError="true">
-          <arg value="${lockverifyserver.host}"/>
-          <arg value="2"/>
-        </java>
-      </sequential>
-      <sequential>
-        <startLockStressTestClient clientId="1"/>
-      </sequential>
-      <sequential>
-        <startLockStressTestClient clientId="2"/>
-      </sequential>
-    </parallel>
+    <testLockFactory lockFactoryImpl="org.apache.lucene.store.NativeFSLockFactory" lockFactoryDir="${build.dir}/lockfactorytest/native" />
+    <testLockFactory lockFactoryImpl="org.apache.lucene.store.SimpleFSLockFactory" lockFactoryDir="${build.dir}/lockfactorytest/simple" />
   </target>
   
   <target name="test" depends="common.test, test-lock-factory"/>

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

@@ -123,14 +123,10 @@ class NativeFSLock extends Lock {
     path = new File(lockDir, lockFileName);
   }
 
-  private synchronized boolean lockExists() {
-    return lock != null;
-  }
-
   @Override
   public synchronized boolean obtain() throws IOException {
 
-    if (lockExists()) {
+    if (lock != null) {
       // Our instance is already locked:
       return false;
     }
@@ -150,7 +146,7 @@ class NativeFSLock extends Lock {
     boolean success = false;
     try {
       lock = channel.tryLock();
-      success = true;
+      success = lock != null;
     } catch (IOException | OverlappingFileLockException e) {
       // At least on OS X, we will sometimes get an
       // intermittent "Permission Denied" IOException,
@@ -171,39 +167,20 @@ class NativeFSLock extends Lock {
         }
       }
     }
-    return lockExists();
+    return lock != null;
   }
 
   @Override
   public synchronized void close() throws IOException {
-    if (lockExists()) {
-      try {
+    try {
+      if (lock != null) {
         lock.release();
-      } finally {
         lock = null;
-        try {
-          channel.close();
-        } finally {
-          channel = null;
-        }
       }
-    } 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
-      // to obtain the lock and release it. If the obtain fails, it means the
-      // lock cannot be released, and we should throw a proper exception rather
-      // than silently failing/not doing anything.
-      boolean obtained = false;
-      try {
-        if (!(obtained = obtain())) {
-          throw new LockReleaseFailedException(
-              "Cannot forcefully unlock a NativeFSLock which is held by another indexer component: "
-                  + path);
-        }
-      } finally {
-        if (obtained) {
-          close();
-        }
+    } finally {
+      if (channel != null) {
+        channel.close();
+        channel = null;
       }
     }
   }
@@ -213,7 +190,7 @@ class NativeFSLock extends Lock {
     // The test for is isLocked is not directly possible with native file locks:
     
     // First a shortcut, if a lock reference in this instance is available
-    if (lockExists()) return true;
+    if (lock != null) return true;
     
     // Look if lock file is present; if not, there can definitely be no lock!
     if (!path.exists()) return false;

lucene/core/src/test/org/apache/lucene/store/TestLockFactory.java

@@ -214,25 +214,6 @@ public class TestLockFactory extends LuceneTestCase {
       }
     }
 
-    public void testNativeFSLockReleaseByOtherLock() throws IOException {
-      NativeFSLockFactory f = new NativeFSLockFactory(createTempDir(LuceneTestCase.getTestClass().getSimpleName()));
-
-      f.setLockPrefix("test");
-      Lock l = f.makeLock("commit");
-      Lock l2 = f.makeLock("commit");
-
-      assertTrue("failed to obtain lock", l.obtain());
-      try {
-        assertTrue(l2.isLocked());
-        l2.close();
-        fail("should not have reached here. LockReleaseFailedException should have been thrown");
-      } catch (LockReleaseFailedException e) {
-        // expected
-      } finally {
-        l.close();
-      }
-    }
-
     // Verify: NativeFSLockFactory assigns null as lockPrefix if the lockDir is inside directory
     public void testNativeFSLockFactoryPrefix() throws IOException {