[MRM-1945] Fixing race condition

Do not return used locks anymore. If the lock map contains
an entry already, the retry loop continues.

2 files changed, 57 insertions, 22 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..dcfeb04b3 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
@@ -71,6 +71,8 @@ public class DefaultFileLockManager
 
         while ( !acquired )
         {
+            // Make sure that not a bad lock is returned, if a exception was thrown.
+            lock = null;
 
             if ( timeout > 0 )
             {
@@ -88,7 +90,7 @@ public class DefaultFileLockManager
 
             if ( current != null )
             {
-                log.debug( "read lock file exist continue wait" );
+                log.trace( "read lock file exist continue wait" );
                 continue;
             }
 
@@ -97,7 +99,22 @@ public class DefaultFileLockManager
                 lock = new Lock( file, false );
                 createNewFileQuietly( file );
                 lock.openLock( false, timeout > 0 );
-                acquired = true;
+                // We are not returning an existing lock. If the lock is not
+                // exclusive, another thread may release the lock and the client
+                // knows nothing about it.
+                // The only atomic operation is the putIfAbsent operation, so if
+                // this returns null everything is OK, otherwise we should start at
+                // the beginning.
+                current = lockFiles.putIfAbsent( file, lock );
+                if ( current == null )
+                {
+                    // Success
+                    acquired = true;
+                } else {
+                    // We try again
+                    lock.close();
+                    lock=null;
+                }
             }
             catch ( FileNotFoundException e )
             {
@@ -116,14 +133,10 @@ public class DefaultFileLockManager
             }
             catch ( IllegalStateException e )
             {
-                log.debug( "openLock {}:{}", e.getClass(), e.getMessage() );
+                log.trace( "openLock {}:{}", e.getClass(), e.getMessage() );
             }
         }
-        Lock current = lockFiles.putIfAbsent( file, lock );
-        if ( current != null )
-        {
-            lock = current;
-        }
+
         return lock;
 
     }
@@ -149,7 +162,8 @@ public class DefaultFileLockManager
 
         while ( !acquired )
         {
-
+            // Make sure that not a bad lock is returned, if a exception was thrown.
+            lock = null;
             if ( timeout > 0 )
             {
                 long delta = stopWatch.getTime();
@@ -169,14 +183,29 @@ public class DefaultFileLockManager
 
                 if ( current != null )
                 {
-                    log.debug( "write lock file exist continue wait" );
+                    log.trace( "write lock file exist continue wait" );
 
                     continue;
                 }
                 lock = new Lock( file, true );
                 createNewFileQuietly( file );
                 lock.openLock( true, timeout > 0 );
-                acquired = true;
+                // We are not returning an existing lock. If the lock is not
+                // exclusive, another thread may release the lock and the client
+                // knows nothing about it.
+                // The only atomic operation is the putIfAbsent operation, so if
+                // this returns null everything is OK, otherwise we should start at
+                // the beginning.
+                current = lockFiles.putIfAbsent( file, lock );
+                if ( current == null )
+                {
+                    // Success
+                    acquired = true;
+                } else {
+                    // We try again
+                    lock.close();
+                    lock=null;
+                }
             }
             catch ( FileNotFoundException e )
             {
@@ -195,16 +224,10 @@ public class DefaultFileLockManager
             }
             catch ( IllegalStateException e )
             {
-                log.debug( "openLock {}:{}", e.getClass(), e.getMessage() );
+                log.trace( "openLock {}:{}", e.getClass(), e.getMessage() );
             }
         }
 
-        Lock current = lockFiles.putIfAbsent( file, lock );
-        if ( current != null )
-        {
-            lock = current;
-        }
-
         return lock;
 
 
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 7fa30b566..af189ba49 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
@@ -144,11 +144,14 @@ public class DefaultFileLockManagerTest {
             try {
                 logger.info("thread3");
                 Lock lock = fileLockManager.readFileLock(this.file);
+                Path outFile = null;
                 try {
+                    outFile = Files.createTempFile("foo", ".jar");
                     Files.copy(Paths.get(lock.getFile().getPath()),
-                            new FileOutputStream(File.createTempFile("foo", ".jar")));
+                            Files.newOutputStream(outFile));
                 } finally {
                     fileLockManager.release(lock);
+                    if (outFile!=null) Files.delete( outFile );
                 }
                 logger.info("thread3 ok");
                 success.incrementAndGet();
@@ -207,10 +210,13 @@ public class DefaultFileLockManagerTest {
             try {
                 logger.info("thread6");
                 Lock lock = fileLockManager.readFileLock(this.file);
+                Path outFile = null;
                 try {
-                    Files.copy(lock.getFile().toPath(), new FileOutputStream(File.createTempFile("foo", ".jar")));
+                    outFile = Files.createTempFile("foo", ".jar");
+                    Files.copy(lock.getFile().toPath(), Files.newOutputStream( outFile ));
                 } finally {
                     fileLockManager.release(lock);
+                    if (outFile!=null) Files.delete( outFile );
                 }
                 logger.info("thread6 ok");
                 success.incrementAndGet();
@@ -248,10 +254,13 @@ public class DefaultFileLockManagerTest {
             try {
                 logger.info("thread8");
                 Lock lock = fileLockManager.readFileLock(this.file);
+                Path outFile = null;
                 try {
-                    Files.copy(lock.getFile().toPath(), new FileOutputStream(File.createTempFile("foo", ".jar")));
+                    outFile = Files.createTempFile("foo", ".jar");
+                    Files.copy(lock.getFile().toPath(), Files.newOutputStream( outFile ));
                 } finally {
                     fileLockManager.release(lock);
+                    if (outFile!=null) Files.delete( outFile );
                 }
                 logger.info("thread8 ok");
                 success.incrementAndGet();
@@ -288,10 +297,13 @@ public class DefaultFileLockManagerTest {
             try {
                 logger.info("thread10");
                 Lock lock = fileLockManager.readFileLock(this.file);
+                Path outFile = null;
                 try {
-                    Files.copy(lock.getFile().toPath(), new FileOutputStream(File.createTempFile("foo", ".jar")));
+                    outFile = Files.createTempFile("foo", ".jar");
+                    Files.copy(lock.getFile().toPath(), Files.newOutputStream( outFile ));
                 } finally {
                     fileLockManager.release(lock);
+                    if (outFile!=null) Files.delete(outFile);
                 }
                 logger.info("thread10 ok");
                 success.incrementAndGet();