Fix Plotter and children for proper garbage collection

[adeak]

tl;dr We were leaking Plotters and some children. I think I've fixed it, even though I don't fully understand some related paranormal activity.

Part 1

Plotters don't get garbage collected. Consider this script:

import gc
import weakref
import sys

import pyvista as pv

pl = pv.Plotter()
wref = weakref.ref(pl)
print('initial: ', sys.getrefcount(wref()))
del pl
gc.collect()

assert wref() is not None
print('after del: ', sys.getrefcount(wref()))
pv.plotting._ALL_PLOTTERS.clear()
gc.collect()
assert wref() is not None
print('after _ALL_PLOTTERS clear: ', sys.getrefcount(wref()))

On main, this prints

initial:  10
after del:  9
after _ALL_PLOTTERS clear:  8

This means that even if we

1. delete the only direct reference to a plotter
2. clear the pv.plotting._ALL_PLOTTERS dict (which should be the only place where plotters are kept track of)
   the plotter doesn't get garbage collected.

Replacing strong references to the plotter in Renderers, Renderer and Charts (i.e. the first commit of this PR) changes the output to

initial:  7
after del:  6

and an assertion error, because in this case the plotter gets garbage collected once it's also cleared from _ALL_PLOTTERS!

Part 2

There's a different, lot more subtle issue too, which caused me to find this bug in the first place. The problem is that Charts objects don't get garbage collected at all!

Consider this script:

import gc
import weakref

import pyvista as pv

pl = pv.Plotter()
pwr = weakref.ref(pl)
rwr = weakref.ref(pl.renderer)
cwr = weakref.ref(pl.renderer._charts)

del pl
pv.plotting._ALL_PLOTTERS.clear()
gc.collect()

assert pwr() is None  # OK, plotter destroyed
assert rwr() is None  # OK, renderer destroyed
assert cwr() is None  # not OK, wat?!

The Plotter is collected (see Part 1), so is its only Renderer. However, the Charts object created when we accessed pl.renderer._charts sticks around!

Part 2.5

The reall weird part is how that Charts objects sticks around. I have no sane explanation (i.e. I suspect C-land shenanigans via VTK). Consider this script:

import gc
import weakref
import sys

import pyvista as pv

pl = pv.Plotter()
rwr = weakref.ref(pl.renderer)
renderer_dict = rwr().__dict__  # the __dict__/vars() of the Renderer
cwr = weakref.ref(pl.renderer._charts)

del pl
pv.plotting._ALL_PLOTTERS.clear()
gc.collect()

assert rwr() is None  # OK, renderer destroyed
assert cwr() is not None  # just left here for proof

print('Remaining refcount of the Charts:', sys.getrefcount(cwr()) - 1)  # one reference is held by sys.getrefcount() itself

assert sys.getrefcount(cwr()) - 1 == len(gc.get_referrers(cwr())) == 1  # one referrer left somewhere
referrer = gc.get_referrers(cwr())[0]  # the one referrer
assert referrer is renderer_dict  # the one referrer is the __dict__ of the collected Renderer, wat?!

This prints

Remaining refcount of the Charts: 1

with no (assertion) errors.

To recap:

1. we created a Plotter -> (Renderers ->) Renderer -> Charts object
2. the plotter and renderer instances are destroyed, the Charts instance sticks around because it has a remaining reference to it
3. that one reference is somehow the exact pl.renderer.__dict__, even though the instance this dict belongs to is dead!

Part 2.75 (Appendix)

Normally, the __dict__ of an instance should die when the instance dies. Consider this example (unfortunately we can't create weakrefs to dicts):

import gc
import weakref
import sys

class Foo:  # pretend Renderer
    pass
class Bar:  # pretend Plotter
    def __init__(self):
        self.renderer = Foo()
pl = Bar()
pwr = weakref.ref(pl)
rwr = weakref.ref(pl.renderer)
renderer_dict_ref = pl.renderer.__dict__
print('Before "plotter" destruction:', sys.getrefcount(renderer_dict_ref) - 1)  # one reference held by sys.getrefcount() itself

del pl
gc.collect()
assert pwr() is None  # "plotter" destroyed
assert rwr() is None  # "renderer" destroyed
print('After "plotter" destruction:', sys.getrefcount(renderer_dict_ref) - 1)  # one reference held by sys.getrefcount() itself

This prints

Before "plotter" destruction: 2
After "plotter" destruction: 1

Which makes sense:

1. destroying the "renderer" decreases the refcount of "renderer"'s __dict__ by 1
2. the one reference left after all this is the strong reference in renderer_dict_ref.

So I'm thinking that we can't end up with the weird situation we have with charts vs plotter.renderer.__dict__ unless there's some C-world reference keeping this __dict__ alive, which in turn keeps the Charts instance alive.

Part 3 (Prologue)

Even though I don't understand what's going on, I finally realised how I can prevent this weird situation. If we reassign the __charts private attribute referencing the Charts while the Renderer is alive, the Charts instance dies!

import gc
import weakref

import pyvista as pv

pl = pv.Plotter()
rwr = weakref.ref(pl.renderer)
cwr = weakref.ref(pl.renderer._charts)

# way to let Charts die: replace them while the Renderer is still alive
rwr()._Renderer__charts = None  # have to undo name mangling
assert cwr() is None  # now it's instantly dead

So if we just make sure to reassign None to renderer.__charts in renderer.__del__() -> renderer.deep_clean(), the problem goes away. I'd love to understand what's going on here, but at least this removes the whole issue.

---

Labelling this as [review-critical] because weakrefs might have unintended side-effects, e.g. I originally used a weakref.proxy in Charts to refer to the renderer, but this broke the SetRenderer() calls. I ended up replacing that with a weakref.ref, which is called in a property so that we don't have to write self.renderer() anywhere. (I think I may have even seen precedent for this in the codebase.) I might even have to replace the other weakref.proxys I've added here to use a similar dereferencing trick, to ensure that the corresponding attributes are bone fide instances of the referenced type.

[adeak]

This is how this all started. I naively added a __del__ when fixing #2991, only to realise that this is never called. Had to skip this for coverage then, since you can't call __del__ on an object that refuses to die.

[codecov]

Codecov Report

Merging #3037 (dd476af) into main (8ff8f63) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3037   +/-   ##
=======================================
  Coverage   94.37%   94.38%           
=======================================
  Files          76       76           
  Lines       16541    16552   +11     
=======================================
+ Hits        15611    15623   +12     
+ Misses        930      929    -1     

[adeak]

We were getting errors inside self.close() due to missing attributes, turned into warnings because they were raised in __del__() (I guess more plotters are dying now during the test suite).

Since errors can appear in many places during __init__(), plotters can have many levels of partial initialization. I've added an _initialized flag that is only True when (Base)Plotter.__init__() is done, and only do self.deep_clean() unless the plotter was fully initialized. This leaves some potential attributes that aren't cleaned (anything set before the error was raised), but this is probably fairly safe inside __del__: those resources should be released soon, hopefully.

Again I'm not 100% sure this is the right call, or the right implementation (e.g. set self._initialized to True at the end of BasePlotter.__init__(), but then to False again at the start of Plotter.__init__()). Test suite doesn't report VTK leaks at least, and the errors-turned-warnings-during-__del__() are now gone.

[akaszynski]

If we reassign the __charts private attribute referencing the Charts

Using delattr works as well:

delattr(rwr(), '_Renderer__charts')

I suspect the issue is that there's a reference Renderer that's sticking around that's causing it's __dict__ to not be collected since the following works just fine:

import gc
import weakref


class FakeCharts:

    def __init__(self):
        pass


class FakeRenderer:

    def __init__(self):
        self._charts = FakeCharts()


class FakePlotter:

    def __init__(self):
        self.renderer = FakeRenderer()

pl = FakePlotter()

rwr = weakref.ref(pl.renderer)
cwr = weakref.ref(pl.renderer._charts)

del pl
assert cwr() is None

---

Many of your changes here match the work done in #2964, and I bet you've cleaned up a few remaining references as I didn't pay attention to Charts.