brooklyn-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From henev...@apache.org
Subject [10/16] git commit: remove finalizers from entity and location, because it causes objects to be kept around a lot longer than desired
Date Sun, 19 Oct 2014 00:59:57 GMT
remove finalizers from entity and location, because it causes objects to be kept around a lot
longer than desired


Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/a4e7741f
Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/a4e7741f
Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/a4e7741f

Branch: refs/heads/master
Commit: a4e7741f9658d1117329668cd1ff4581cb5bd32f
Parents: 31f430e
Author: Alex Heneveld <alex.heneveld@cloudsoftcorp.com>
Authored: Sat Oct 18 06:47:20 2014 +0100
Committer: Alex Heneveld <alex.heneveld@cloudsoftcorp.com>
Committed: Sat Oct 18 07:15:02 2014 +0100

----------------------------------------------------------------------
 .../brooklyn/entity/basic/AbstractEntity.java   |  9 ---------
 .../location/basic/AbstractLocation.java        | 14 ++++++++------
 .../location/basic/SshMachineLocation.java      | 18 ++++++++++--------
 .../internal/LocalLocationManager.java          | 20 +++++++++++++-------
 4 files changed, 31 insertions(+), 30 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/a4e7741f/core/src/main/java/brooklyn/entity/basic/AbstractEntity.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/entity/basic/AbstractEntity.java b/core/src/main/java/brooklyn/entity/basic/AbstractEntity.java
index 412ded0..fe55d5c 100644
--- a/core/src/main/java/brooklyn/entity/basic/AbstractEntity.java
+++ b/core/src/main/java/brooklyn/entity/basic/AbstractEntity.java
@@ -1450,13 +1450,4 @@ public abstract class AbstractEntity extends AbstractBrooklynObject
implements E
     public boolean containsTag(Object tag) {
         return getTagSupport().containsTag(tag);
     }    
-
-    // this is not recommended because -- especially in rebind-read-only mode, we create
a lot of large such entities
-    // and the phantom references that finalize uses are not GC'd very quickly, so it *looks*
like a memory leak
-    @Override
-    protected void finalize() throws Throwable {
-        super.finalize();
-        if (!getManagementSupport().wasDeployed() && !Boolean.TRUE.equals(getManagementSupport().isReadOnly()))
-            LOG.warn("Entity "+this+" ("+Integer.toHexString(System.identityHashCode(this))+")
was never deployed -- explicit call to manage(Entity) required.");
-    }
 }

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/a4e7741f/core/src/main/java/brooklyn/location/basic/AbstractLocation.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/basic/AbstractLocation.java b/core/src/main/java/brooklyn/location/basic/AbstractLocation.java
index e619c57..f652f8a 100644
--- a/core/src/main/java/brooklyn/location/basic/AbstractLocation.java
+++ b/core/src/main/java/brooklyn/location/basic/AbstractLocation.java
@@ -315,7 +315,7 @@ public abstract class AbstractLocation extends AbstractBrooklynObject
implements
         setParent(newParent, true);
     }
     
-    public void setParent(Location newParent, boolean updateChildListInOldParent) {
+    public void setParent(Location newParent, boolean updateChildListParents) {
         if (newParent == this) {
             throw new IllegalArgumentException("Location cannot be its own parent: "+this);
         }
@@ -323,17 +323,19 @@ public abstract class AbstractLocation extends AbstractBrooklynObject
implements
             return; // no-op; already have desired parent
         }
         
-        // TODO Should we support a location changing parent? The resulting unmanage/manage
might cause problems.
         if (parent.get() != null) {
             Location oldParent = parent.get();
             parent.set(null);
-            if (updateChildListInOldParent)
+            if (updateChildListParents)
                 ((AbstractLocation)oldParent).removeChild(this);
         }
+        // TODO Should we support a location changing parent? The resulting unmanage/manage
might cause problems.
+        // The code above suggests we do, but maybe we should warn or throw error, or at
least test it!
+        
+        parent.set(newParent);
         if (newParent != null) {
-            parent.set(newParent);
-            if (updateChildListInOldParent)
-                ((AbstractLocation)parent.get()).addChild(this);
+            if (updateChildListParents)
+                ((AbstractLocation)newParent).addChild(this);
         }
         
         onChanged();

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/a4e7741f/core/src/main/java/brooklyn/location/basic/SshMachineLocation.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/location/basic/SshMachineLocation.java b/core/src/main/java/brooklyn/location/basic/SshMachineLocation.java
index a381e34..836b5b4 100644
--- a/core/src/main/java/brooklyn/location/basic/SshMachineLocation.java
+++ b/core/src/main/java/brooklyn/location/basic/SshMachineLocation.java
@@ -362,6 +362,7 @@ public class SshMachineLocation extends AbstractLocation implements MachineLocat
                         try {
                             if (sshPoolCacheOrNull != null) sshPoolCacheOrNull.cleanUp();
                             if (!SshMachineLocation.this.isManaged()) {
+                                if (sshPoolCacheOrNull != null) sshPoolCacheOrNull.invalidateAll();
                                 cleanupTask.cancel(false);
                                 sshPoolCacheOrNull = null;
                             }
@@ -401,14 +402,15 @@ public class SshMachineLocation extends AbstractLocation implements
MachineLocat
         }
     }
 
-    @Override
-    protected void finalize() throws Throwable {
-        try {
-            close();
-        } finally {
-            super.finalize();
-        }
-    }
+    // should not be necessary, and causes objects to be kept around a lot longer than desired
+//    @Override
+//    protected void finalize() throws Throwable {
+//        try {
+//            close();
+//        } finally {
+//            super.finalize();
+//        }
+//    }
 
     @Override
     public InetAddress getAddress() {

http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/a4e7741f/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java b/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java
index ede4c69..7ad7315 100644
--- a/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java
+++ b/core/src/main/java/brooklyn/management/internal/LocalLocationManager.java
@@ -51,7 +51,6 @@ import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Maps;
-import com.google.common.io.Closeables;
 
 public class LocalLocationManager implements LocationManagerInternal {
 
@@ -263,6 +262,7 @@ public class LocalLocationManager implements LocationManagerInternal {
 
         if (hasBeenReplaced) {
             // we are unmanaging an old instance after having replaced it
+            unmanageNonRecursiveOnlyClearItsFields(loc, mode);
             
             if (mode==ManagementTransitionMode.REBINDING_NO_LONGER_PRIMARY) {
                 // when migrating away, these all need to be called
@@ -368,18 +368,24 @@ public class LocalLocationManager implements LocationManagerInternal
{
         
         return true;
     }
-    
-    /**
-     * Should ensure that the location is no longer managed anywhere, remove from all lists.
-     * Returns true if the location has been removed from management; if it was not previously
managed (anything else throws exception) 
-     */
-    private synchronized boolean unmanageNonRecursive(Location loc, ManagementTransitionMode
mode) {
+
+    private synchronized void unmanageNonRecursiveOnlyClearItsFields(Location loc, ManagementTransitionMode
mode) {
         if (mode==ManagementTransitionMode.DESTROYING) {
             ((AbstractLocation)loc).setParent(null, true);
         } else {
             // if not destroying, don't change the parent's children list
             ((AbstractLocation)loc).setParent(null, false);
         }
+        // clear config to help with GC
+        ((AbstractLocation)loc).getLocalConfigBag().clear();
+    }
+    
+    /**
+     * Should ensure that the location is no longer managed anywhere, remove from all lists.
+     * Returns true if the location has been removed from management; if it was not previously
managed (anything else throws exception) 
+     */
+    private synchronized boolean unmanageNonRecursive(Location loc, ManagementTransitionMode
mode) {
+        unmanageNonRecursiveOnlyClearItsFields(loc, mode);
         
         Object old = locationsById.remove(loc.getId());
         locationTypes.remove(loc.getId());


Mime
View raw message