-
Notifications
You must be signed in to change notification settings - Fork 55
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.
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
BROOKLYN-245: improve synchronization in entity #96
Conversation
@@ -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() { |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
one comment to address |
Avoid calling getSubscriptionContext() while holding lock on AbstractEntity.this. See discussion in apache#96
Avoid calling getSubscriptionContext() while holding lock on AbstractEntity.this. See discussion in #96
No description provided.