Based on my cursory look at this code and this change, I think this
change isn't right. From what I see in the code, the synchronized block
is necessary, since what it's trying to achieve there is a mutual
exclusivity over a bunch of operations within that block. With this
synchronized block removed, it no longer guarantees the entire block to
be done serially by a single thread anymore.
-Jaikiran
On 02/11/17 2:50 AM, gin...@apache.org wrote:
No need to synchronise a concurrent map
Project: http://git-wip-us.apache.org/repos/asf/ant-ivy/repo
Commit: http://git-wip-us.apache.org/repos/asf/ant-ivy/commit/bac64754
Tree: http://git-wip-us.apache.org/repos/asf/ant-ivy/tree/bac64754
Diff: http://git-wip-us.apache.org/repos/asf/ant-ivy/diff/bac64754
Branch: refs/heads/master
Commit: bac647543497f0aa2a9f92fe726e1eb18631b4cc
Parents: fa21d6f
Author: Gintas Grigelionis <gin...@apache.org>
Authored: Wed Nov 1 22:19:31 2017 +0100
Committer: Gintas Grigelionis <gin...@apache.org>
Committed: Wed Nov 1 22:19:31 2017 +0100
----------------------------------------------------------------------
.../ivy/plugins/lock/FileBasedLockStrategy.java | 68 +++++++++-----------
1 file changed, 29 insertions(+), 39 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/ant-ivy/blob/bac64754/src/java/org/apache/ivy/plugins/lock/FileBasedLockStrategy.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/ivy/plugins/lock/FileBasedLockStrategy.java
b/src/java/org/apache/ivy/plugins/lock/FileBasedLockStrategy.java
index 2e31b2f..d59c544 100644
--- a/src/java/org/apache/ivy/plugins/lock/FileBasedLockStrategy.java
+++ b/src/java/org/apache/ivy/plugins/lock/FileBasedLockStrategy.java
@@ -64,39 +64,34 @@ public abstract class FileBasedLockStrategy extends
AbstractLockStrategy {
}
long start = System.currentTimeMillis();
do {
- synchronized (currentLockHolders) {
+ int lockCount = hasLock(file, currentThread);
+ if (isDebugLocking()) {
+ debugLocking("current status for " + file + " is " + lockCount
+ + " held locks: " + getCurrentLockHolderNames(file));
+ }
+ if (lockCount < 0) {
+ /* Another thread in this process holds the lock; we need to
wait */
if (isDebugLocking()) {
- debugLocking("entered synchronized area (locking)");
+ debugLocking("waiting for another thread to release the lock:
"
+ + getCurrentLockHolderNames(file));
}
- int lockCount = hasLock(file, currentThread);
+ } else if (lockCount > 0) {
+ int holdLocks = incrementLock(file, currentThread);
if (isDebugLocking()) {
- debugLocking("current status for " + file + " is " +
lockCount
- + " held locks: " +
getCurrentLockHolderNames(file));
+ debugLocking("reentrant lock acquired on " + file + " in "
+ + (System.currentTimeMillis() - start) + "ms" + " -
hold locks = "
+ + holdLocks);
}
- if (lockCount < 0) {
- /* Another thread in this process holds the lock; we need
to wait */
- if (isDebugLocking()) {
- debugLocking("waiting for another thread to release the
lock: "
- + getCurrentLockHolderNames(file));
- }
- } else if (lockCount > 0) {
- int holdLocks = incrementLock(file, currentThread);
+ return true;
+ } else {
+ /* No prior lock on this file is held at all */
+ if (locker.tryLock(file)) {
if (isDebugLocking()) {
- debugLocking("reentrant lock acquired on " + file + " in
"
- + (System.currentTimeMillis() - start) + "ms" + " -
hold locks = "
- + holdLocks);
+ debugLocking("lock acquired on " + file + " in "
+ + (System.currentTimeMillis() - start) + "ms");
}
+ incrementLock(file, currentThread);
return true;
- } else {
- /* No prior lock on this file is held at all */
- if (locker.tryLock(file)) {
- if (isDebugLocking()) {
- debugLocking("lock acquired on " + file + " in "
- + (System.currentTimeMillis() - start) +
"ms");
- }
- incrementLock(file, currentThread);
- return true;
- }
}
}
if (isDebugLocking()) {
@@ -112,21 +107,16 @@ public abstract class FileBasedLockStrategy extends
AbstractLockStrategy {
if (isDebugLocking()) {
debugLocking("releasing lock on " + file);
}
- synchronized (currentLockHolders) {
+ int holdLocks = decrementLock(file, currentThread);
+ if (holdLocks == 0) {
+ locker.unlock(file);
if (isDebugLocking()) {
- debugLocking("entered synchronized area (unlocking)");
+ debugLocking("lock released on " + file);
}
- int holdLocks = decrementLock(file, currentThread);
- if (holdLocks == 0) {
- locker.unlock(file);
- if (isDebugLocking()) {
- debugLocking("lock released on " + file);
- }
- } else {
- if (isDebugLocking()) {
- debugLocking("reentrant lock released on " + file + " - hold
locks = "
- + holdLocks);
- }
+ } else {
+ if (isDebugLocking()) {
+ debugLocking("reentrant lock released on " + file + " - hold locks
= "
+ + holdLocks);
}
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
