From 3fd9dc0e5c584702d25982172bb5ee44b6b57aa3 Mon Sep 17 00:00:00 2001 From: Jim Hahn Date: Thu, 12 Apr 2018 14:23:25 -0400 Subject: Sonar fixes for policy-core locks Made a number of changes to the locking code in policy-core, to address sonar issues. This entaileed changing some of the Lock API methods to return OperResult instead of Boolean. Updated distributed locking with the new API return types. Simplified Thread creation using functional methods. Change-Id: If32bf7a435d2aedb969de1b77c7e7e27e110ecb0 Issue-ID: POLICY-728 Signed-off-by: Jim Hahn --- .../policy/drools/core/lock/LockRequestFuture.java | 4 +- .../core/lock/PolicyResourceLockFeatureAPI.java | 87 ++++++++++------------ .../core/lock/PolicyResourceLockManager.java | 66 +++++++++++----- 3 files changed, 90 insertions(+), 67 deletions(-) (limited to 'policy-core/src/main') diff --git a/policy-core/src/main/java/org/onap/policy/drools/core/lock/LockRequestFuture.java b/policy-core/src/main/java/org/onap/policy/drools/core/lock/LockRequestFuture.java index 46d1ff2d..a2e9e62e 100644 --- a/policy-core/src/main/java/org/onap/policy/drools/core/lock/LockRequestFuture.java +++ b/policy-core/src/main/java/org/onap/policy/drools/core/lock/LockRequestFuture.java @@ -183,7 +183,7 @@ public class LockRequestFuture implements Future { * @return {@code true} if the lock was acquired, {@code false} if it was denied */ @Override - public Boolean get() throws CancellationException, InterruptedException { + public Boolean get() throws InterruptedException { waiter.await(); switch (state.get()) { @@ -202,7 +202,7 @@ public class LockRequestFuture implements Future { */ @Override public Boolean get(long timeout, TimeUnit unit) - throws CancellationException, InterruptedException, TimeoutException { + throws InterruptedException, TimeoutException { if (!waiter.await(timeout, unit)) { throw new TimeoutException("lock request did not complete in time"); diff --git a/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPI.java b/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPI.java index 718ed5e9..9f42936d 100644 --- a/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPI.java +++ b/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPI.java @@ -46,9 +46,9 @@ public interface PolicyResourceLockFeatureAPI extends OrderedService { new OrderedServiceImpl<>(PolicyResourceLockFeatureAPI.class); /** - * Callback that an implementer invokes when a lock is acquired (or denied), - * asynchronously. The implementer invokes the method to indicate that the lock was - * acquired (or denied). + * Callback that an implementer invokes, asynchronously, when a lock is acquired (or + * denied). The implementer invokes the method to indicate that the lock was acquired + * (or denied). */ @FunctionalInterface public static interface Callback { @@ -61,6 +61,31 @@ public interface PolicyResourceLockFeatureAPI extends OrderedService { public void set(boolean locked); } + /** + * Result of a requested operation. + */ + public static enum OperResult { + + /** + * The implementer accepted the request; no additional locking logic should be + * performed. + */ + OPER_ACCEPTED, + + /** + * The implementer denied the request; no additional locking logic should be + * performed. + */ + OPER_DENIED, + + + /** + * The implementer did not handle the request; additional locking logic should + * be performed. + */ + OPER_UNHANDLED + } + /** * This method is called before a lock is acquired on a resource. If a callback is * provided, and the implementer is unable to acquire the lock immediately, then the @@ -112,21 +137,11 @@ public interface PolicyResourceLockFeatureAPI extends OrderedService { * * @param resourceId * @param owner - *
true
- *
the implementer handled the request and found the resource to be locked - * by the given owner; the resource was unlocked and no additional locking - * logic should be performed
- *
false
- *
the implementer handled the request and found the resource was not - * locked by given the owner; no additional locking logic should be - * performed
- *
null
- *
the implementer did not handle the request; additional locking logic - * should be performed - * + * @return the result, where OPER_DENIED indicates that the lock is not + * currently held by the given owner */ - public default Boolean beforeUnlock(String resourceId, String owner) { - return null; + public default OperResult beforeUnlock(String resourceId, String owner) { + return OperResult.OPER_UNHANDLED; } /** @@ -147,21 +162,11 @@ public interface PolicyResourceLockFeatureAPI extends OrderedService { * This method is called before a check is made to determine if a resource is locked. * * @param resourceId - * @return - *
- *
true
- *
the implementer handled the request and found the resource to be - * locked; no additional locking logic should be performed
- *
false
- *
the implementer handled the request and found the resource was not - * locked; no additional locking logic should be performed
- *
null
- *
the implementer did not handle the request; additional locking logic - * should be performed - *
+ * @return the result, where OPER_ACCEPTED indicates that the resource is + * locked, while OPER_DENIED indicates that it is not */ - public default Boolean beforeIsLocked(String resourceId) { - return null; + public default OperResult beforeIsLocked(String resourceId) { + return OperResult.OPER_UNHANDLED; } /** @@ -170,21 +175,11 @@ public interface PolicyResourceLockFeatureAPI extends OrderedService { * * @param resourceId * @param owner - * @return - *
- *
true
- *
the implementer handled the request and found the resource to be locked - * by the given owner; no additional locking logic should be performed
- *
false
- *
the implementer handled the request and found the resource was not - * locked by given the owner; no additional locking logic should be - * performed
- *
null
- *
the implementer did not handle the request; additional locking logic - * should be performed - *
+ * @return the result, where OPER_ACCEPTED indicates that the resource is + * locked by the given owner, while OPER_DENIED indicates that it is + * not */ - public default Boolean beforeIsLockedBy(String resourceId, String owner) { - return null; + public default OperResult beforeIsLockedBy(String resourceId, String owner) { + return OperResult.OPER_UNHANDLED; } } diff --git a/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockManager.java b/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockManager.java index d51f2b91..a9305e51 100644 --- a/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockManager.java +++ b/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockManager.java @@ -26,7 +26,9 @@ import static org.onap.policy.drools.core.lock.LockRequestFuture.makeNullArgExce import java.util.List; import java.util.concurrent.Future; import java.util.function.Function; +import java.util.function.Supplier; import org.onap.policy.drools.core.lock.PolicyResourceLockFeatureAPI.Callback; +import org.onap.policy.drools.core.lock.PolicyResourceLockFeatureAPI.OperResult; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -40,7 +42,7 @@ public class PolicyResourceLockManager extends SimpleLockManager { /** * Used to access various objects. */ - public static Factory factory = new Factory(); + private static Factory factory = new Factory(); /** * Used by junit tests. @@ -105,17 +107,16 @@ public class PolicyResourceLockManager extends SimpleLockManager { throw makeNullArgException(MSG_NULL_OWNER); } - Boolean result = doIntercept(null, impl -> impl.beforeUnlock(resourceId, owner)); - if (result != null) { - return result; - } - // implementer didn't do the work - use superclass - boolean unlocked = super.unlock(resourceId, owner); + return doBoolIntercept(impl -> impl.beforeUnlock(resourceId, owner), () -> { + + // implementer didn't do the work - defer to the superclass + boolean unlocked = super.unlock(resourceId, owner); - doIntercept(false, impl -> impl.afterUnlock(resourceId, owner, unlocked)); + doIntercept(false, impl -> impl.afterUnlock(resourceId, owner, unlocked)); - return unlocked; + return unlocked; + }); } /** @@ -128,12 +129,12 @@ public class PolicyResourceLockManager extends SimpleLockManager { throw makeNullArgException(MSG_NULL_RESOURCE_ID); } - Boolean result = doIntercept(null, impl -> impl.beforeIsLocked(resourceId)); - if (result != null) { - return result; - } - return super.isLocked(resourceId); + return doBoolIntercept(impl -> impl.beforeIsLocked(resourceId), () -> { + + // implementer didn't do the work - defer to the superclass + return super.isLocked(resourceId); + }); } /** @@ -150,12 +151,31 @@ public class PolicyResourceLockManager extends SimpleLockManager { throw makeNullArgException(MSG_NULL_OWNER); } - Boolean result = doIntercept(null, impl -> impl.beforeIsLockedBy(resourceId, owner)); - if (result != null) { - return result; + return doBoolIntercept(impl -> impl.beforeIsLockedBy(resourceId, owner), () -> { + + // implementer didn't do the work - defer to the superclass + return super.isLockedBy(resourceId, owner); + }); + } + + /** + * Applies a function to each implementer of the lock feature. Returns as soon as one + * of them returns a result other than OPER_UNHANDLED. If they all return + * OPER_UNHANDLED, then it returns the result of applying the default function. + * + * @param interceptFunc + * @param defaultFunc + * @return {@code true} if success, {@code false} otherwise + */ + private boolean doBoolIntercept(Function interceptFunc, + Supplier defaultFunc) { + + OperResult result = doIntercept(OperResult.OPER_UNHANDLED, interceptFunc); + if (result != OperResult.OPER_UNHANDLED) { + return (result == OperResult.OPER_ACCEPTED); } - return super.isLockedBy(resourceId, owner); + return defaultFunc.get(); } /** @@ -168,7 +188,7 @@ public class PolicyResourceLockManager extends SimpleLockManager { * @return first non-null value returned by an implementer, continueValue if * they all returned continueValue */ - public static T doIntercept(T continueValue, Function func) { + private static T doIntercept(T continueValue, Function func) { for (PolicyResourceLockFeatureAPI impl : factory.getImplementers()) { try { @@ -189,6 +209,14 @@ public class PolicyResourceLockManager extends SimpleLockManager { * Initialization-on-demand holder idiom. */ private static class Singleton { + + /** + * Not invoked. + */ + private Singleton() { + super(); + } + private static final PolicyResourceLockManager instance = new PolicyResourceLockManager(); } -- cgit 1.2.3-korg