ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jaikiran Pai <jai.forums2...@gmail.com>
Subject Re: [2/2] ant-ivy git commit: No need to synchronise a concurrent map
Date Thu, 02 Nov 2017 03:17:25 GMT
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 <gintas@apache.org>
> Authored: Wed Nov 1 22:19:31 2017 +0100
> Committer: Gintas Grigelionis <gintas@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


Mime
View raw message