fix proxy leak on extra promotion to master
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/35142c80 Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/35142c80 Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/35142c80 Branch: refs/heads/master Commit: 35142c8049d3068775f448d6c48edeb365f53bfa Parents: 71c82da Author: Alex Heneveld <[email protected]> Authored: Mon Feb 9 10:23:20 2015 +0000 Committer: Alex Heneveld <[email protected]> Committed: Mon Feb 9 10:23:20 2015 +0000 ---------------------------------------------------------------------- .../entity/proxying/InternalEntityFactory.java | 12 ++++++++- .../rebind/InitialFullRebindIteration.java | 8 ++++++ .../internal/EntityManagementSupport.java | 4 ++- .../entity/rebind/ActivePartialRebindTest.java | 26 +++++++++++++++++++- .../ha/HighAvailabilityManagerInMemoryTest.java | 21 +++++++++++++++- 5 files changed, 67 insertions(+), 4 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/35142c80/core/src/main/java/brooklyn/entity/proxying/InternalEntityFactory.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/proxying/InternalEntityFactory.java b/core/src/main/java/brooklyn/entity/proxying/InternalEntityFactory.java index 40885b4..880d07a 100644 --- a/core/src/main/java/brooklyn/entity/proxying/InternalEntityFactory.java +++ b/core/src/main/java/brooklyn/entity/proxying/InternalEntityFactory.java @@ -358,7 +358,17 @@ public class InternalEntityFactory extends InternalFactory { checkState(interfaces != null && !Iterables.isEmpty(interfaces), "must have at least one interface for entity %s:%s", clazz, entityId); T entity = constructEntityImpl(clazz, ImmutableMap.<String, Object>of(), entityId); - if (((AbstractEntity)entity).getProxy() == null) ((AbstractEntity)entity).setProxy(createEntityProxy(interfaces, entity)); + if (((AbstractEntity)entity).getProxy() == null) { + Entity proxy = managementContext.getEntityManager().getEntity(entity.getId()); + if (proxy==null) { + // normal case, proxy does not exist + proxy = createEntityProxy(interfaces, entity); + } else { + // only if rebinding to existing; don't create a new proxy, then we have proxy explosion + // but callers must be careful that the entity's proxy does not yet point to it + } + ((AbstractEntity)entity).setProxy(proxy); + } return entity; } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/35142c80/core/src/main/java/brooklyn/entity/rebind/InitialFullRebindIteration.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/entity/rebind/InitialFullRebindIteration.java b/core/src/main/java/brooklyn/entity/rebind/InitialFullRebindIteration.java index 9906f30..44ef922 100644 --- a/core/src/main/java/brooklyn/entity/rebind/InitialFullRebindIteration.java +++ b/core/src/main/java/brooklyn/entity/rebind/InitialFullRebindIteration.java @@ -94,6 +94,14 @@ public class InitialFullRebindIteration extends RebindIteration { // this is discouraged if we were already master Entity anEntity = Iterables.getFirst(managementContext.getEntityManager().getEntities(), null); if (anEntity!=null && !((EntityInternal)anEntity).getManagementSupport().isReadOnly()) { + // NB: there is some complexity which can happen in this situation; "initial-full" rebind really expected everything is being + // initially bound from persisted state, and completely so; "active-partial" is much more forgiving. + // one big difference is in behaviour of management: it is recursive for most situations, and initial-full assumes recursive, + // but primary-primary is *not* recursive; + // as a result some "new" entities created during rebind might be left unmanaged; they should get GC'd, + // but it's possible the new entity impl or even a new proxy could leak. + // see HighAvailabilityManagerInMemoryTest.testLocationsStillManagedCorrectlyAfterDoublePromotion + // (i've plugged the known such leaks, but still something to watch out for); -Alex 2015-02 overwritingMaster = true; LOG.warn("Rebind requested for "+mode+" node "+managementContext.getManagementNodeId()+" " + "when it already has active state; discouraged, " http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/35142c80/core/src/main/java/brooklyn/management/internal/EntityManagementSupport.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/management/internal/EntityManagementSupport.java b/core/src/main/java/brooklyn/management/internal/EntityManagementSupport.java index 4bea56e..eb9a27f 100644 --- a/core/src/main/java/brooklyn/management/internal/EntityManagementSupport.java +++ b/core/src/main/java/brooklyn/management/internal/EntityManagementSupport.java @@ -268,7 +268,9 @@ public class EntityManagementSupport { public void onManagementStopping(ManagementTransitionInfo info) { synchronized (this) { if (managementContext != info.getManagementContext()) { - throw new IllegalStateException("Has different management context: "+managementContext+"; expected "+info.getManagementContext()); + throw new IllegalStateException("onManagementStopping encountered different management context for "+entity+ + (!wasDeployed() ? " (wasn't deployed)" : !isDeployed() ? " (no longer deployed)" : "")+ + ": "+managementContext+"; expected "+info.getManagementContext()); } Stopwatch startTime = Stopwatch.createStarted(); while (!managementFailed.get() && nonDeploymentManagementContext!=null && http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/35142c80/core/src/test/java/brooklyn/entity/rebind/ActivePartialRebindTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/brooklyn/entity/rebind/ActivePartialRebindTest.java b/core/src/test/java/brooklyn/entity/rebind/ActivePartialRebindTest.java index 80e13b6..414ca25 100644 --- a/core/src/test/java/brooklyn/entity/rebind/ActivePartialRebindTest.java +++ b/core/src/test/java/brooklyn/entity/rebind/ActivePartialRebindTest.java @@ -27,6 +27,7 @@ import brooklyn.basic.BrooklynObject; import brooklyn.entity.Entity; import brooklyn.entity.basic.AbstractEntity; import brooklyn.entity.basic.Entities; +import brooklyn.entity.basic.EntityInternal; import brooklyn.entity.proxying.EntitySpec; import brooklyn.test.entity.TestEntity; import brooklyn.util.text.Strings; @@ -41,7 +42,7 @@ public class ActivePartialRebindTest extends RebindTestFixtureWithApp { } @Test - public void testRebindOneSimple() throws Exception { + public void testRebindChildSimple() throws Exception { TestEntity c1 = origApp.addChild(EntitySpec.create(TestEntity.class)); Entities.manage(c1); AbstractEntity c1r = Entities.deproxy(c1); @@ -55,6 +56,29 @@ public class ActivePartialRebindTest extends RebindTestFixtureWithApp { Assert.assertFalse(c2r == c1r, "Real instance should NOT be the same: "+c1r+" / "+c2r); } + @Test + public void testRebindParentSimple() throws Exception { + TestEntity c1 = origApp.addChild(EntitySpec.create(TestEntity.class)); + Entities.manage(c1); + + AbstractEntity origAppr = Entities.deproxy(origApp); + + doPartialRebindOfIds(origApp.getId()); + + BrooklynObject app2 = origManagementContext.lookup(origApp.getId()); + AbstractEntity app2r = Entities.deproxy((Entity)app2); + + Assert.assertTrue(app2 == origApp, "Proxy instance should be the same: "+app2+" / "+origApp); + Assert.assertFalse(app2r == origAppr, "Real instance should NOT be the same: "+app2r+" / "+origAppr); + + Assert.assertTrue(c1.getManagementSupport().isDeployed()); + + // check that child of parent is not a new unmanaged entity + Entity c1b = origApp.getChildren().iterator().next(); + Assert.assertTrue(c1.getManagementSupport().isDeployed()); + Assert.assertTrue( ((EntityInternal)c1b).getManagementSupport().isDeployed(), "Not deployed: "+c1b ); + } + @Test(groups="Integration") public void testRebindCheckingMemoryLeak() throws Exception { TestEntity c1 = origApp.addChild(EntitySpec.create(TestEntity.class)); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/35142c80/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerInMemoryTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerInMemoryTest.java b/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerInMemoryTest.java index 7c9f213..c647f19 100644 --- a/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerInMemoryTest.java +++ b/core/src/test/java/brooklyn/management/ha/HighAvailabilityManagerInMemoryTest.java @@ -27,6 +27,7 @@ import org.testng.Assert; import org.testng.annotations.Test; import brooklyn.entity.Entity; +import brooklyn.entity.basic.EntityInternal; import brooklyn.entity.proxying.EntitySpec; import brooklyn.entity.rebind.persister.InMemoryObjectStore; import brooklyn.entity.rebind.persister.PersistenceObjectStore; @@ -34,6 +35,7 @@ import brooklyn.location.Location; import brooklyn.location.NoMachinesAvailableException; import brooklyn.location.basic.LocalhostMachineProvisioningLocation; import brooklyn.location.basic.SshMachineLocation; +import brooklyn.management.internal.LocalManagementContext; import brooklyn.test.entity.TestApplication; import brooklyn.test.entity.TestEntity; import brooklyn.util.collections.MutableList; @@ -76,15 +78,22 @@ public class HighAvailabilityManagerInMemoryTest extends HighAvailabilityManager Collection<Location> lm = managementContext.getLocationManager().getLocations(); log.info("Locs managed are: "+lm); log.info(" objs: "+identities(lm)); + Assert.assertNotNull(entity.getManagementSupport().getManagementContext()); + Assert.assertNotNull( ((EntityInternal)app.getChildren().iterator().next()).getManagementSupport().getManagementContext()); + Assert.assertTrue( ((EntityInternal)app.getChildren().iterator().next()).getManagementSupport().isDeployed()); + checkEntitiesHealthy(app, entity); managementContext.getRebindManager().forcePersistNow(true, null); - log.info("DODGY extra promoteToMaster"); + log.info("Test deliberately doing unnecessary extra promoteToMaster"); ha.promoteToMaster(); log.info("Entities managed are: "+managementContext.getEntityManager().getEntities()); Collection<Location> lle = entity.getLocations(); log.info("Locs at entity(old) are: "+lle); log.info(" objs: "+identities(lle)); + // check entities -- the initial-full promotion previously re-created items, + // and plugged them in as children, but only managed the roots + checkEntitiesHealthy(app, entity); // assert what's in the location manager is accurate Collection<Location> llmm = managementContext.getLocationManager().getLocations(); @@ -108,6 +117,16 @@ public class HighAvailabilityManagerInMemoryTest extends HighAvailabilityManager } + private void checkEntitiesHealthy(TestApplication app, TestEntity entity) { + Assert.assertNotNull(app.getManagementSupport().getManagementContext()); + Assert.assertTrue( app.getManagementSupport().getManagementContext().isRunning() ); + + Assert.assertNotNull(entity.getManagementSupport().getManagementContext()); + Assert.assertNotNull( ((EntityInternal)app.getChildren().iterator().next()).getManagementSupport().getManagementContext() ); + Assert.assertTrue( ((EntityInternal)app.getChildren().iterator().next()).getManagementSupport().isDeployed()); + Assert.assertTrue( ((EntityInternal)app.getChildren().iterator().next()).getManagementSupport().getManagementContext() instanceof LocalManagementContext ); + } + @Test(groups="Integration", invocationCount=50) public void testLocationsStillManagedCorrectlyAfterDoublePromotionManyTimes() throws NoMachinesAvailableException { testLocationsStillManagedCorrectlyAfterDoublePromotion();
