Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/835#discussion_r142912453
--- Diff:
core/src/main/java/org/apache/brooklyn/core/sensor/AttributeMap.java ---
@@ -139,9 +139,18 @@ private void checkPath(Collection<String> path) {
}
public <T> T update(AttributeSensor<T> attribute, T newValue) {
- T oldValue = updateWithoutPublishing(attribute, newValue);
- entity.emitInternal(attribute, newValue);
- return oldValue;
+ // 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) {
--- End diff --
TL;DR: I agree this is fine. Below are some notes from digging around in
the synchronization code.
Looking at the other synchronization blocks, `emitInternal` will
synchronize on `EntityManagementSupport` instance (when it calls
`getSubscriptionContext()`. It will then call `publish` which will synchronize
on `LocalSubscriptionManager` instance (in `getSubscriptionsForEntitySensor`).
However, I think we can trust both the `EntityManagementSupport` and the
`LocalSubscriptionManager` to not call out to alien code while holding a lock
on itself. Therefore we should be safe in that respect.
The code in `AbstractEntity` and `AbstractGroupImpl` looks a bit scary,
where it first gets the lock on this `values` object and then on either
`abstractGroup.members` or `abstractEntity.children` (but it's kind-of
understandable why it does that and how that pattern avoids it; it would just
be nicer if instead we didn't call out to alient code while holding the lock on
`values` - but that's a bigger discussion than for this PR):
```
synchronized (getAttributesSynchObjectInternal()) {
synchronized (members) {
```
---
