Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BROOKLYN-245: improve synchronization in entity #96

Merged
merged 2 commits into from
Apr 1, 2016
Merged

BROOKLYN-245: improve synchronization in entity #96

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24aa5dd
BROOKLYN-245: improve synchronization in entity
aledsage Mar 31, 2016
01c67b6
Entity.getSubscriptionTracker synchronization
aledsage Apr 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1484,15 +1484,21 @@ public void unsubscribeAll() {
}

protected SubscriptionContext getSubscriptionContext() {
synchronized (AbstractEntity.this) {
return getManagementSupport().getSubscriptionContext();
}
// Rely on synchronization in EntityManagementSupport; synchronizing on AbstractEntity.this
// is dangerous because user's entity code might synchronize on that and call getAttribute.
// Given that getSubscriptionContext is called by AttributeMap.update (via emitInternal),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix looks good, agree EMS handles synching is sufficient and means we should not try to do so here

// that risks deadlock!
return getManagementSupport().getSubscriptionContext();
}

protected SubscriptionTracker getSubscriptionTracker() {
// TODO Would be nice to simplify concurrent model, and not synchronize on
// AbstractEntity.this; perhaps could get rid of lazy-initialisation, but then
// would need to first ensure `managementSupport` is definitely initialised.
SubscriptionContext subscriptionContext = getSubscriptionContext();
synchronized (AbstractEntity.this) {
if (_subscriptionTracker == null) {
_subscriptionTracker = new SubscriptionTracker(getSubscriptionContext());
_subscriptionTracker = new SubscriptionTracker(subscriptionContext);
}
return _subscriptionTracker;
}
Expand Down Expand Up @@ -1548,7 +1554,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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the BasicSubscriptionSupport.getSubscriptionTracker() method this calls also synchronizes on AbstractEntity.this (line 1494 in new money) -- so no impact whether this method is synchronized or not

in contrast to the change to unsynchronize getSubscriptionContext() here, synchronizing on something when getting the tracker looks right as it is setting a local field. however the underlying method (line 1498 new money) calls to getSubscriptionContext() while synchronized on this so i think we haven't entirely eliminated the deadlock prospect.

feels like line 1498 could double-check the tracker, and load the getSubscriptionContext() between the checks eg

if (tracker!=null) return tracker;
subs = getSubscriptionContext();
synchronized (AbstractEntity.this) {
    if (tracker!=null) return tracker;
    tracker = new Tracker(subs);
    return tracker;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added another commit that does the getSubscriptionContext() call outside of the synchronized block - just in case there are any problems I haven't thought of there!

I didn't bother with double-checked locking and volatile field. If we care hugely about performance here, then we should instantiate it on entity initialisation (before this method is called), and get rid of all the synchronization in the method. That would be a simpler concurrency model. But it's a bit fiddly because of the use EntityManagementSupport (and its lazy initialisation, and use of NonDeploymentManagementContext on construction).

return subscriptions().getSubscriptionTracker();
}

Expand Down