From 8dc5e696d225ae3607b861063ca191e993c60898 Mon Sep 17 00:00:00 2001 From: Martin Stockhammer Date: Wed, 26 Oct 2016 23:25:50 +0200 Subject: [PATCH] Stabilised file lock implementation and tests --- .../filelock/DefaultFileLockManager.java | 31 ++++++++++---- .../apache/archiva/common/filelock/Lock.java | 2 +- .../filelock/DefaultFileLockManagerTest.java | 40 ++++++++++++------- 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 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() ); } -- 2.39.5