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`
---
