Repository: archiva Updated Branches: refs/heads/master 0e73c44d8 -> 878287b7b
[MRM-1945] Fixing race condition Do not return used locks anymore. If the lock map contains an entry already, the retry loop continues. Project: http://git-wip-us.apache.org/repos/asf/archiva/repo Commit: http://git-wip-us.apache.org/repos/asf/archiva/commit/878287b7 Tree: http://git-wip-us.apache.org/repos/asf/archiva/tree/878287b7 Diff: http://git-wip-us.apache.org/repos/asf/archiva/diff/878287b7 Branch: refs/heads/master Commit: 878287b7b3a8c42a4a78028ca4d7b5204b4a5ab8 Parents: 2d23c4a Author: Martin Stockhammer <[email protected]> Authored: Sun May 28 22:32:29 2017 +0200 Committer: Martin Stockhammer <[email protected]> Committed: Sun May 28 22:40:07 2017 +0200 ---------------------------------------------------------------------- .../common/filelock/DefaultFileLockManager.java | 59 ++++++++++++++------ .../filelock/DefaultFileLockManagerTest.java | 20 +++++-- 2 files changed, 57 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/archiva/blob/878287b7/archiva-modules/archiva-base/archiva-filelock/src/main/java/org/apache/archiva/common/filelock/DefaultFileLockManager.java ---------------------------------------------------------------------- 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 ee4fb35..dcfeb04 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; http://git-wip-us.apache.org/repos/asf/archiva/blob/878287b7/archiva-modules/archiva-base/archiva-filelock/src/test/java/org/apache/archiva/common/filelock/DefaultFileLockManagerTest.java ---------------------------------------------------------------------- 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 7fa30b5..af189ba 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();
