You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2016/03/31 21:51:54 UTC
[GitHub] brooklyn-server pull request: BROOKLYN-245: improve synchronizatio...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/96
BROOKLYN-245: improve synchronization in entity
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server fix/BROOKLYN-245-deadlock
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/96.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #96
----
commit 24aa5dddd87e5b20ed7e43f817be9355d0b198bd
Author: Aled Sage <al...@gmail.com>
Date: 2016-03-31T19:46:58Z
BROOKLYN-245: improve synchronization in entity
----
---
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.
---
[GitHub] brooklyn-server pull request: BROOKLYN-245: improve synchronizatio...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/brooklyn-server/pull/96#issuecomment-204416974
i've merged this, with one further small tidy-up 9720743a23b816914e66fb37ed4d783837ecca36
this and #97 have been backported to 0.9.0
---
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.
---
[GitHub] brooklyn-server pull request: BROOKLYN-245: improve synchronizatio...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/96#discussion_r58174385
--- 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 --
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;
}
---
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.
---
[GitHub] brooklyn-server pull request: BROOKLYN-245: improve synchronizatio...
Posted by aledsage <gi...@git.apache.org>.
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.
---
[GitHub] brooklyn-server pull request: BROOKLYN-245: improve synchronizatio...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/brooklyn-server/pull/96#issuecomment-204302561
one comment to address
---
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.
---
[GitHub] brooklyn-server pull request: BROOKLYN-245: improve synchronizatio...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/96#discussion_r58174628
--- Diff: core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java ---
@@ -1484,9 +1484,11 @@ public void unsubscribeAll() {
}
protected SubscriptionContext getSubscriptionContext() {
- synchronized (AbstractEntity.this) {
- return getManagementSupport().getSubscriptionContext();
- }
--- End diff --
fix looks good, agree `EMS` handles synching is sufficient and means we should not try to do so here
---
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.
---
[GitHub] brooklyn-server pull request: BROOKLYN-245: improve synchronizatio...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/96
---
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.
---
[GitHub] brooklyn-server pull request: BROOKLYN-245: improve synchronizatio...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/96#discussion_r58194087
--- 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'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).
---
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.
---