Fix Plotter and children for proper garbage collection (#3037)

* Change plotting parents to weakrefs

* Delete reference to __charts in Renderer.deep_clean() for gc

* Switch from weakref.proxy to ref in Charts

* Add regression test for collection of Plotter and children

* Cover Charts.__del__() (where this all started)

* Add _initialized flag for (Base)Plotter against errors in __del__

pyvista/plotting/charts.py

@@ -4370,7 +4370,16 @@ def __init__(self, renderer):
         # needed.
         self._scene = None
         self._actor = None
-        self._renderer = renderer
+
+        # a weakref.proxy would be nice here, but that doesn't play
+        # nicely with SetRenderer, so instead we'll use a weak reference
+        # plus a property to call it
+        self.__renderer = weakref.ref(renderer)
+
+    @property
+    def _renderer(self):
+        """Return the weakly dereferenced renderer, maybe None."""
+        return self.__renderer()
 
     def _setup_scene(self):
         """Set up a new context scene and actor for these charts."""
@@ -4459,6 +4468,6 @@ def __iter__(self):
         for chart in self._charts:
             yield chart
 
-    def __del__(self):  # pragma: no cover
+    def __del__(self):
         """Clean up before being destroyed."""
         self.deep_clean()

pyvista/plotting/plotting.py

@@ -201,6 +201,8 @@ def __init__(
         """Initialize base plotter."""
         super().__init__(**kwargs)  # cooperative multiple inheritance
         log.debug('BasePlotter init start')
+        self._initialized = False
+
         self._theme = pyvista.themes.DefaultTheme()
         if theme is None:
             # copy global theme to ensure local plot theme is fixed
@@ -281,6 +283,8 @@ def __init__(
         if self.theme.antialiasing:
             self.enable_anti_aliasing()
 
+        self._initialized = True
+
     @property
     def theme(self):
         """Return or set the theme used for this plotter.
@@ -4720,13 +4724,12 @@ def _datasets(self):
 
     def __del__(self):
         """Delete the plotter."""
-        # We have to check here if it has the closed attribute as it
-        # may not exist should the plotter have failed to initialize.
-        if hasattr(self, '_closed'):
+        # We have to check here if the plotter was only partially initialized
+        if self._initialized:
             if not self._closed:
                 self.close()
         self.deep_clean()
-        if hasattr(self, 'renderers'):
+        if self._initialized:
             del self.renderers
 
     def add_background_image(self, image_path, scale=1, auto_resize=True, as_global=True):
@@ -5163,6 +5166,8 @@ def __init__(
             lighting=lighting,
             theme=theme,
         )
+        # reset partial initialization flag
+        self._initialized = False
 
         log.debug('Plotter init start')
 
@@ -5256,6 +5261,9 @@ def on_timer(iren, event_id):
             if self.enable_depth_peeling():
                 for renderer in self.renderers:
                     renderer.enable_depth_peeling()
+
+        # some cleanup only necessary for fully initialized plotters
+        self._initialized = True
         log.debug('Plotter init stop')
 
     def show(

pyvista/plotting/renderer.py

@@ -203,7 +203,7 @@ def __init__(self, parent, border=True, border_color='w', border_width=2.0):
         """Initialize the renderer."""
         super().__init__()
         self._actors = {}
-        self.parent = parent  # the plotter
+        self.parent = parent  # weakref.proxy to the plotter from Renderers
         self._theme = parent.theme
         self.camera_set = False
         self.bounding_box_actor = None
@@ -2760,6 +2760,7 @@ def deep_clean(self, render=False):
             self.disable_shadows()
         if self.__charts is not None:
             self.__charts.deep_clean()
+            self.__charts = None
 
         self.remove_floors(render=render)
         self.remove_legend(render=render)

pyvista/plotting/renderers.py

@@ -1,5 +1,6 @@
 """Organize Renderers for ``pyvista.Plotter``."""
 import collections
+from weakref import proxy
 
 import numpy as np
 
@@ -26,7 +27,7 @@ def __init__(
     ):
         """Initialize renderers."""
         self._active_index = 0  # index of the active renderer
-        self._plotter = plotter
+        self._plotter = proxy(plotter)
         self._renderers = []
 
         # by default add border for multiple plots

tests/plotting/test_collection.py

@@ -1,4 +1,5 @@
 """This module contains any tests which cause memory leaks."""
+import gc
 import weakref
 
 import numpy as np
@@ -40,3 +41,23 @@ def test_add_array(sphere):
     """Ensure data added dynamically to a plotter is collected."""
     pl = pv.Plotter()
     pl.add_mesh(sphere, scalars=range(sphere.n_points))
+
+
+def test_plotting_collection():
+    """Ensure that we don't leak Plotter, Renderer and Charts instances."""
+    pl = pv.Plotter()
+    ref_plotter = weakref.ref(pl)
+    ref_renderers = weakref.ref(pl.renderers)
+    ref_renderer = weakref.ref(pl.renderer)
+    ref_charts = weakref.ref(pl.renderer._charts)  # instantiated on the fly
+
+    # delete known references to Plotter
+    del pv.plotting._ALL_PLOTTERS[pl._id_name]
+    del pl
+
+    # check that everything is eventually destroyed
+    gc.collect()  # small reference cycles allowed
+    assert ref_plotter() is None, gc.get_referrers(ref_plotter())
+    assert ref_renderers() is None
+    assert ref_renderer() is None
+    assert ref_charts() is None