brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aledsage <...@git.apache.org>
Subject [GitHub] brooklyn-server pull request #860: improve locking for sensor values
Date Thu, 12 Oct 2017 09:01:03 GMT
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/860#discussion_r144226175
  
    --- Diff: core/src/main/java/org/apache/brooklyn/core/sensor/AttributeMap.java ---
    @@ -138,62 +165,91 @@ private void checkPath(Collection<String> path) {
             Preconditions.checkArgument(!path.isEmpty(), "path can't be empty");
         }
     
    +    /**
    +     * Sets a value and published it.
    +     * <p>
    +     * Constraints of {@link #getLockInternal()} apply, with lock-holder being the supplied
{@link Function}.
    +     */
         public <T> T update(AttributeSensor<T> attribute, T newValue) {
             // 2017-10 this was unsynched which meant if two threads updated
             // the last publication would not correspond to the last value.
    -        // could introduce deadlock but emit internal and publish should
    -        // not seek any locks. _subscribe_ and _delivery_ might, but they
    -        // won't be in this block. an issue with _subscribe-and-get-initial_
    -        // should be resolved by initial subscription queueing the publication
    -        // to a context where locks are not held.
    -        synchronized (values) {
    -            T oldValue = updateWithoutPublishing(attribute, newValue);
    +        // a crude `synchronized (values)` could cause deadlock as
    +        // this does publish (doing `synch(LSM)`) whereas _subscribe_ 
    +        // would have the LSM lock and try to `synch(values)`
    +        // as described at https://issues.apache.org/jira/browse/BROOKLYN-544.
    +        // the addition of getLockInternal should make this much cleaner,
    +        // as no one holding `synch(LSM)` should be updating here, 
    +        // ands gets here won't call any other code at all
    +        return withLock(() -> {
    +            T oldValue = updateInternalWithoutLockOrPublish(attribute, newValue);
                 entity.emitInternal(attribute, newValue);
                 return oldValue;
    -        }
    +        });
         }
         
    +    /** @deprecated since 0.13.0 this is becoming an internal method, {@link #updateInternalWithoutLockOrPublish(AttributeSensor,
Object)} */
    +    @Deprecated
         public <T> T updateWithoutPublishing(AttributeSensor<T> attribute, T
newValue) {
    -        if (log.isTraceEnabled()) {
    -            Object oldValue = getValue(attribute);
    -            if (!Objects.equal(oldValue, newValue != null)) {
    -                log.trace("setting attribute {} to {} (was {}) on {}", new Object[] {attribute.getName(),
newValue, oldValue, entity});
    -            } else {
    -                log.trace("setting attribute {} to {} (unchanged) on {}", new Object[]
{attribute.getName(), newValue, this});
    +        return updateInternalWithoutLockOrPublish(attribute, newValue);
    +    }
    +    
    +    @Beta
    +    public <T> T updateInternalWithoutLockOrPublish(AttributeSensor<T> attribute,
T newValue) {
    +        synchronized (values) {
    +            if (log.isTraceEnabled()) {
    +                Object oldValue = getValue(attribute);
    +                if (!Objects.equal(oldValue, newValue != null)) {
    +                    log.trace("setting attribute {} to {} (was {}) on {}", new Object[]
{attribute.getName(), newValue, oldValue, entity});
    +                } else {
    +                    log.trace("setting attribute {} to {} (unchanged) on {}", new Object[]
{attribute.getName(), newValue, this});
    +                }
                 }
    +    
    +            T oldValue = update(attribute.getNameParts(), newValue);
    +            return (isNull(oldValue)) ? null : oldValue;
             }
    -
    -        T oldValue = update(attribute.getNameParts(), newValue);
    -        
    -        return (isNull(oldValue)) ? null : oldValue;
         }
     
    +    private <T> T withLock(Callable<T> body) { return Locks.withLock(getLockInternal(),
body); }
    +    private void withLock(Runnable body) { Locks.withLock(getLockInternal(), body); }
    +    
         /**
    -     * Where atomicity is desired, the methods in this class synchronize on the {@link
#values} map.
    +     * Where atomicity is desired, this method wraps an acquisition of {@link #getLockInternal()}
    +     * while applying the given {@link Function}.
    +     * <p>
    +     * Constraints of {@link #getLockInternal()} apply, with lock-holder being the supplied
{@link Function}.
          */
         public <T> T modify(AttributeSensor<T> attribute, Function<? super
T, Maybe<T>> modifier) {
    -        synchronized (values) {
    +        return withLock(() -> {
                 T oldValue = getValue(attribute);
                 Maybe<? extends T> newValue = modifier.apply(oldValue);
    -
    +            
                 if (newValue.isPresent()) {
                     if (log.isTraceEnabled()) log.trace("modified attribute {} to {} (was
{}) on {}", new Object[] {attribute.getName(), newValue, oldValue, entity});
                     return update(attribute, newValue.get());
                 } else {
                     if (log.isTraceEnabled()) log.trace("modified attribute {} unchanged;
not emitting on {}", new Object[] {attribute.getName(), newValue, this});
                     return oldValue;
                 }
    -        }
    +        });
         }
     
    +    /** Removes a sensor.  The {@link #getLockInternal()} is acquired,
    +     * and constraints on the caller described there apply to callers of this method.
    +     * <p>
    +     * The caller is responsible for any subsequent actions needed, including publishing
    +     * the removal of the sensor and triggering persistence. Caller may wish to 
    +     * have the {@link #getLockInternal()} while doing that. */
         public void remove(AttributeSensor<?> attribute) {
             BrooklynLogging.log(log, BrooklynLogging.levelDebugOrTraceIfReadOnly(entity),
                 "removing attribute {} on {}", attribute.getName(), entity);
    -
    -        remove(attribute.getNameParts());
    +        withLock(() -> remove(attribute.getNameParts()) );
         }
     
         // TODO path must be ordered(and legal to contain duplicates like "a.b.a"; list would
be better
    +    /** @deprecated since 0.13.0 becoming private; callers should only ever use {@link
#remove(AttributeSensor)}
    --- End diff --
    
    `since 1.0.0`


---

Mime
View raw message