Kepler

This is an interesting issue. To pursue it would require a test case.

We've seen plenty of issues with cloning of actors in the past. What happens is that if an actor does not properly clone its fields, then we tend to see non-deterministic behavior around Actor-Oriented classes.

This issue seems to be a different level as it involves ptolemy/kernel/util/HierarchyListener.java and NamedObj.

I'm happy to provide a test case - what's the best format?

A simple model and a description of what to do would be sufficient.

I'm attaching a program that illustrates the leak. It creates a simple model containing Ramp, clones the model 100 times, then sleeps. A reference is kept to the original model, but not the clones, so they should be gc'd.

Once the program sleeps, you should see 101 Ramp instances in the heap: one in the original model and one for each clone. According to jvisualvm, the nearest gc root for all the Ramp instances is _hierarchyListeners in the same CompositeActor instance (the original model).

It seems like when we clone, it is not sufficient to set _hierarchyListeners to null in NamedObj, we also need to look at the container and remove the listeners. setContainer() should do this for us.

Ok, now I see Dan's text:

In the usecase I'm debugging, the toplevel Composites in both the original and new models are added as hierarchy listeners to PortParameters, but only the toplevel in the new model should be added. The original toplevel is added since clone() and setContainer() are called for the PortParameter before its container actor has been moved to the new toplevel.
I don't know the best approach to fix this. Making the listeners weak references would fix the memory leaks, but wrong listeners would still receive hierarchy change events.

I agree with the above analysis.

AbstractInitializableParameter.java has a setContainer() method that is invoking addHierarchyListener and removeHierarchyListener. PortParameter extends AbstractInitializabelParameter.

I'm not sure if this is right, but it solves the problem and passes tests.

I modified NamedObj.clone(Workspace) so that it detects the problem and removes listener. See below for the code.

I think we have a larger problem here in part because we don't have setContainer() in NamedObj. In this case, if the container of the container of the parameter is the same as the container of the original master NamedObj, then shouldn't we set the container of the container of the parameter to something else? Maybe this happens later.

Anyway, let's leave this bug open until the nightly build runs and we see if anything breaks.

                while (parameters.hasNext()) {
                    Attribute parameter = (Attribute) parameters.next();
                    Attribute newParameter = (Attribute) parameter
                            .clone(workspace);

                    try {
                        newParameter.setContainer(newObject);
                        // PortParameters are                                                                                                               
                        // AbstractInitializableParameters that end up                                                                                      
                        // adding HierarchyListeners to the                                                                                                 
                        // initializable container of the actor.  This                                                                                      
                        // results in a memory leak, so we remove                                                                                           
                        // parameters that are HierarchyListeners.                                                                                          
                        // See https://projects.ecoinformatics.org/ecoinfo/issues/7190                                                                      
                        if (newParameter instanceof HierarchyListener) {
                            if (newParameter.getContainer() != null) {
                                NamedObj newContainerContainer = newParameter.getContainer().getContainer();
                                NamedObj oldContainer = getContainer();
                                // FIXME: This is probably a serious                                                                                        
                                // problem that the container of the                                                                                        
                                // container of the parameter in the                                                                                        
                                // clone is the same as the container                                                                                       
                                // of original master NamedObj.  Part                                                                                       
                                // of the issue here is that NamedObj                                                                                       
                                // does not have setContainer().  What                                                                                      
                                // we would really like to do is to                                                                                         
                                // set the container to null during                                                                                         
                                // cloning.                                                                                                                 
                                if (newContainerContainer == oldContainer) {
                                    newContainerContainer.removeHierarchyListener((HierarchyListener)newParameter);
                                }
                            }
                        }
                    } catch (KernelException exception) {
                        throw new CloneNotSupportedException(
                                "Failed to clone attribute " 
                                        + parameter.getFullName() + ": " 
                                        + exception);
                    }
                }
            }

Thanks for the fix, Christopher. I verified the memory leak is going, so feel free to close the bug once the nightly tests run.

Oops, that should be: memory leak is gone, not going.

I don't seem to have permission to close bugs, but this bug can be closed now. The nightly build seems to be working just fine.