You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ant.apache.org by gi...@apache.org on 2017/11/01 21:20:16 UTC
[2/2] ant-ivy git commit: No need to synchronise a concurrent map
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 <gi...@apache.org>
Authored: Wed Nov 1 22:19:31 2017 +0100
Committer: Gintas Grigelionis <gi...@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);
}
}
}
Re: [2/2] ant-ivy git commit: No need to synchronise a concurrent map
Posted by Jaikiran Pai <ja...@gmail.com>.
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, gintas@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 <gi...@apache.org>
> Authored: Wed Nov 1 22:19:31 2017 +0100
> Committer: Gintas Grigelionis <gi...@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-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org