Stabilised file lock implementation and tests

3 files changed, 50 insertions, 23 deletions

diff --git a/archiva-modules/archiva-base/archiva-filelock/src/main/java/org/apache/archiva/common/filelock/DefaultFileLockManager.java b/archiva-modules/archiva-base/archiva-filelock/src/main/java/org/apache/archiva/common/filelock/DefaultFileLockManager.java
index ee4fb35be..2ce2f188c 100644
--- a/archiva-modules/archiva-base/archiva-filelock/src/main/java/org/apache/archiva/common/filelock/DefaultFileLockManager.java
+++ b/archiva-modules/archiva-base/archiva-filelock/src/main/java/org/apache/archiva/common/filelock/DefaultFileLockManager.java
@@ -31,6 +31,7 @@ import java.io.RandomAccessFile;
 import java.nio.channels.ClosedChannelException;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 /**
  * @author Olivier Lamy
@@ -51,7 +52,6 @@ public class DefaultFileLockManager
 
     private int timeout = 0;
 
-
     @Override
     public Lock readFileLock( File file )
         throws FileLockException, FileLockTimeoutException
@@ -162,20 +162,18 @@ public class DefaultFileLockManager
                 }
             }
 
-            Lock current = lockFiles.get( file );
-
             try
             {
-
+                Lock current = lockFiles.get( file );
                 if ( current != null )
                 {
                     log.debug( "write lock file exist continue wait" );
 
                     continue;
                 }
-                lock = new Lock( file, true );
-                createNewFileQuietly( file );
-                lock.openLock( true, timeout > 0 );
+                lock = new Lock(file, true);
+                createNewFileQuietly(file);
+                lock.openLock(true, timeout > 0);
                 acquired = true;
             }
             catch ( FileNotFoundException e )
@@ -191,12 +189,29 @@ public class DefaultFileLockManager
             }
             catch ( IOException e )
             {
+                if (lock!=null && lock.isValid()) {
+                    try {
+                        lock.close();
+                    } catch (IOException ex) {
+                        // Ignore
+                    }
+                }
                 throw new FileLockException( e.getMessage(), e );
             }
-            catch ( IllegalStateException e )
+            catch ( Throwable e )
             {
+                if (lock!=null && lock.isValid()) {
+                    try {
+                        lock.close();
+                    } catch (IOException ex) {
+                        // Ignore
+                    } finally {
+                        lock = null;
+                    }
+                }
                 log.debug( "openLock {}:{}", e.getClass(), e.getMessage() );
             }
+
         }
 
         Lock current = lockFiles.putIfAbsent( file, lock );
diff --git a/archiva-modules/archiva-base/archiva-filelock/src/main/java/org/apache/archiva/common/filelock/Lock.java b/archiva-modules/archiva-base/archiva-filelock/src/main/java/org/apache/archiva/common/filelock/Lock.java
index bd4afcb61..17b19c0b0 100644
--- a/archiva-modules/archiva-base/archiva-filelock/src/main/java/org/apache/archiva/common/filelock/Lock.java
+++ b/archiva-modules/archiva-base/archiva-filelock/src/main/java/org/apache/archiva/common/filelock/Lock.java
@@ -90,7 +90,7 @@ public class Lock
 
     public boolean isValid()
     {
-        return this.fileLock.isValid();
+        return this.fileLock!=null && this.fileLock.isValid();
     }
 
     public Map<Thread, AtomicInteger> getFileClients()
diff --git a/archiva-modules/archiva-base/archiva-filelock/src/test/java/org/apache/archiva/common/filelock/DefaultFileLockManagerTest.java b/archiva-modules/archiva-base/archiva-filelock/src/test/java/org/apache/archiva/common/filelock/DefaultFileLockManagerTest.java
index cc63e0c3a..96186612e 100644
--- a/archiva-modules/archiva-base/archiva-filelock/src/test/java/org/apache/archiva/common/filelock/DefaultFileLockManagerTest.java
+++ b/archiva-modules/archiva-base/archiva-filelock/src/test/java/org/apache/archiva/common/filelock/DefaultFileLockManagerTest.java
@@ -35,7 +35,9 @@ import javax.inject.Named;
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
+import java.nio.file.FileAlreadyExistsException;
 import java.nio.file.Files;
+import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.StandardCopyOption;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -83,6 +85,22 @@ public class DefaultFileLockManagerTest
 
         }
 
+        // Files.copy is not atomic so have to try several times in
+        // a multithreaded test
+        private void copyFile(Path source, Path destination) {
+            int attempts=10;
+            boolean finished = false;
+            while(!finished && attempts-->0) {
+                try {
+                    Files.copy(source, destination, StandardCopyOption.REPLACE_EXISTING,
+                            StandardCopyOption.COPY_ATTRIBUTES);
+                    finished=true;
+                } catch (IOException ex) {
+                    //
+                }
+            }
+        }
+
         public void thread1()
             throws FileLockException, FileLockTimeoutException, IOException
         {
@@ -91,8 +109,7 @@ public class DefaultFileLockManagerTest
             try
             {
                 lock.getFile().delete();
-                Files.copy( largeJar.toPath(), lock.getFile().toPath(), StandardCopyOption.REPLACE_EXISTING,
-                            StandardCopyOption.COPY_ATTRIBUTES );
+                copyFile( largeJar.toPath(), lock.getFile().toPath());
             }
             finally
             {
@@ -110,8 +127,7 @@ public class DefaultFileLockManagerTest
             try
             {
                 lock.getFile().delete();
-                Files.copy( largeJar.toPath(), lock.getFile().toPath(), StandardCopyOption.REPLACE_EXISTING,
-                            StandardCopyOption.COPY_ATTRIBUTES );
+                copyFile( largeJar.toPath(), lock.getFile().toPath());
             }
             finally
             {
@@ -147,8 +163,7 @@ public class DefaultFileLockManagerTest
             try
             {
                 lock.getFile().delete();
-                Files.copy( largeJar.toPath(), lock.getFile().toPath(), StandardCopyOption.REPLACE_EXISTING,
-                            StandardCopyOption.COPY_ATTRIBUTES );
+                copyFile( largeJar.toPath(), lock.getFile().toPath());
             }
             finally
             {
@@ -166,8 +181,7 @@ public class DefaultFileLockManagerTest
             try
             {
                 lock.getFile().delete();
-                Files.copy( largeJar.toPath(), lock.getFile().toPath(), StandardCopyOption.REPLACE_EXISTING,
-                            StandardCopyOption.COPY_ATTRIBUTES );
+                copyFile( largeJar.toPath(), lock.getFile().toPath());
             }
             finally
             {
@@ -202,8 +216,7 @@ public class DefaultFileLockManagerTest
             try
             {
                 lock.getFile().delete();
-                Files.copy( largeJar.toPath(), lock.getFile().toPath(), StandardCopyOption.REPLACE_EXISTING,
-                            StandardCopyOption.COPY_ATTRIBUTES );
+                copyFile( largeJar.toPath(), lock.getFile().toPath());
             }
             finally
             {
@@ -238,8 +251,7 @@ public class DefaultFileLockManagerTest
             try
             {
                 lock.getFile().delete();
-                Files.copy( largeJar.toPath(), lock.getFile().toPath(), StandardCopyOption.REPLACE_EXISTING,
-                            StandardCopyOption.COPY_ATTRIBUTES );
+                copyFile( largeJar.toPath(), lock.getFile().toPath());
             }
             finally
             {
@@ -283,9 +295,9 @@ public class DefaultFileLockManagerTest
     {
         ConcurrentFileWrite concurrentFileWrite = new ConcurrentFileWrite( fileLockManager );
         //concurrentFileWrite.setTrace( true );
-        TestFramework.runOnce( concurrentFileWrite );
+        TestFramework.runManyTimes( concurrentFileWrite, 10);
         logger.info( "success: {}", concurrentFileWrite.success );
-        Assert.assertEquals( 10, concurrentFileWrite.success.intValue() );
+        Assert.assertEquals( 100, concurrentFileWrite.success.intValue() );
     }