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: BROOKLYN-245: improve synchronizatio...
Date Fri, 01 Apr 2016 11:12:04 GMT
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/96#discussion_r58191767
  
    --- Diff: core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java ---
    @@ -1548,7 +1550,7 @@ public boolean unsubscribe(Entity producer, SubscriptionHandle handle)
{
          * @deprecated since 0.9.0; for internal use only
          */
         @Deprecated
    -    protected synchronized SubscriptionTracker getSubscriptionTracker() {
    +    protected SubscriptionTracker getSubscriptionTracker() {
    --- End diff --
    
    I think this existing code is ok, in terms of the deadlock encountered in BROOKLYN-245.
The `AttributeMap` should never call into `getSubscriptionTracker()`, so there will never
be a call to it while holding a lock on `AttributeMap.values`.
    
    In terms of calling `getSubscriptionContext()` while holding other locks (e.g. on `AbstractEntity.this`)
we should be ok: it will synchronize on `EntityManagementSupport.this` and will then call
into the management context's subscription manager, but that code should *never* call out
to alien code.
    
    It would be good to remove the `synchronized (AbstractEntity.this)` entirely, to have
simpler concurrency code. But we must be aware that the caller might already have synchronized
on the entity instance when calling `subscribe()`. So removing this synchronization doesn't
eliminate any deadlocks - instead we must be sure that synchronizing on the entity instance
won't risk deadlocks.
    
    As an aside, note that double-checked locking is broken if the field is not declared volatile.
    
    Given that, are you ok leaving the code as-is in this PR?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message