You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@archiva.apache.org by ma...@apache.org on 2017/06/10 11:26:20 UTC
[35/50] archiva git commit: [MRM-1945] Fixing race condition
[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/citest
Commit: 878287b7b3a8c42a4a78028ca4d7b5204b4a5ab8
Parents: 2d23c4a
Author: Martin Stockhammer <ma...@apache.org>
Authored: Sun May 28 22:32:29 2017 +0200
Committer: Martin Stockhammer <ma...@apache.org>
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();