Hopefully adding a test dependency in a patch release will not be too troublesome for the packagers. Assume the escape hatch to patch out the test.

@richardsheridan Thank you for tracking this down and 👍🏻 on adding that test!

@tacaswell should I XFAIL the test for now or just let it fail entirely?

Can you usespytest.importorskiphttps://docs.pytest.org/en/6.2.x/reference.html#pytest-importorskip ? We already use it in the tests to skip the tests that require pandas.

This PR is blocked by the hidpi leak that maybe only@QuLogic can debug. Otherwise we could adjust the memory leak threshold higher so it passes for now. That way the hidpi leak can be fixed later and th threshold can be lowered again at that time. Thoughts?

I don't have Windows to check the HiDPI code right now. But I'm not sure how it could be leaking? The only HiDPI variable is attached to the window (the figure manager doeswindow_dpi = tk.IntVar(master=window, ...) andself._window_dpi = window_dpi), so it should go away with it.

I suppose there's also the additionalFont object on the toolbar as well, but adding an explicitdel self.toolbar._label_font todelayed_destroy doesn't do anything.

If I runmemleak.py with the following patch:

$ git diffdiff --git a/tools/memleak.py b/tools/memleak.pyindex 9b9da912b7..d61d0b57c2 100755--- a/tools/memleak.py+++ b/tools/memleak.py@@ -79,10 +79,10 @@ def run_memleak_test(bench, iterations, report):     ax3.plot(open_files_arr)     ax3.set_ylabel('open file handles')-    if not report.endswith('.pdf'):-        report = report + '.pdf'+    if not report.endswith('.png'):+        report = report + '.png'     fig.tight_layout()-    fig.savefig(report, format='pdf')+    fig.savefig(report, format='png')   class MemleakTest:@@ -115,7 +115,7 @@ class MemleakTest:             ax.pcolor(10 * np.random.rand(50, 50))          fig.savefig(BytesIO(), dpi=75)-        fig.canvas.flush_events()+        # fig.canvas.flush_events()         plt.close(1)

Main branch

this branch

This clearly shows that this branch fixes something, but that the warm up time is in the hundreds of figures which is not practical for the test suite.

I took the test and put it in a standalone script, and ran it through scalene. Unfortunately, it seems to break Tk threading with explicitgc.collect calls. With garbage collection, it seemed to think thatFigureCanvasTk._tkphoto andFigureCanvasTk._tkcanvas.create_image were the biggest leak. However, I don't know how much we can trust that without garbage collection.

That being said,@tacaswell noticed that we explicitly deleted the image created from the photo on the canvas during resize:

So I wonder if we're somehow missing deletion of that image when destroying the figure?

I made a bit of progress on this thanks to learning aboutmemleak.py. The ever-increasing not-garbage objects before this PR was due to tkinter hanging on to various callbacks. I identified them usingobjgraph.show_growth and followingobjgraph.show_backrefs from them. Confusingly they lead to nowhere, and they also show up withobjgraph.get_leaking_objects. They aren't actually leaked, though... they are being legitimatelytracked by the tkinter c module. these aremostly cleared via running the event loop in the patch.

I say mostly, but actually all were gone but one; theCallWrapper associated withFigureManagerTk._update_window_dpi. ThisCallWrapper is created by thetrace_add command which registers the callback, and, much to my surprise, does not die with theIntVar either ondestroy. It might be cleaned up on normal python GC, but the entire web of figures and backend python objects is being pinned alive by that callback, in a sort of un-collectable reference cycle.

I manually unregister the trace ina3016af, and accellerate the garbage collection ine6e4cad. This gave me flat graphs for memory and object behavior inmemleak.py. Yay! But tkagg still fails my new test. Boo!

I updated the test to flush events after both creating and closing the figure and adjusted the thresholds. It passes on my machine and we'll see if CI accepts it.

If I changememleak.py a bit more:

$  git diffdiff --git a/tools/memleak.py b/tools/memleak.pyindex 9b9da912b7..6dbc9dc505 100755--- a/tools/memleak.py+++ b/tools/memleak.py@@ -115,8 +121,9 @@ class MemleakTest:             ax.pcolor(10 * np.random.rand(50, 50))         fig.savefig(BytesIO(), dpi=75)-        fig.canvas.flush_events()+       plt.pause(.1)         plt.close(1) if __name__ == '__main__':

I see continued growth in RSS in the tkagg backend, but constant object count which is disturbing.

Nevertheless, the growth is muted compared to before the patches.

I cannot reproduce the error in CI that has to do with a missingupdate command locally, but it must have something to do with the difference betweenflush_events andpause after the figure is destroyed. Pause seems more leaky than flush_events so I reverted my earlier tweak, but the test will still fail on tkagg at the moment.

I pushed a couple of updates to the tests that should hopefully fix them. At least, they appear fine locally.

thanks@QuLogic I'm happy with your changes fwiw.

this one is a definite squash merge when the time comes

For reference, using the example from#20490 (comment) (+ agc.collect), I managed to run memray on it, and it went from:

to:

There's a bit of a spinup, which I haven't quite investigated (i.e., how many figures, or if it's just initial imports and such). The peaking behaviour doesn't exhibit in the RSS when you run the script by itself, so I assume that's just memray periodically writing out stats (or maybe it's slowed down the gc a bit.)

Thank you@richardsheridan and@QuLogic !

@meeseeksdev backport to v3.5.x

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

git checkout v3.5.xgit pull

2. Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 1a016f0395c3a8d87b632a47d813db2491863164

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #22002: Fix TkAgg memory leaks and test for memory growth regressions'

4. Push to a named branch:

git push YOURFORK v3.5.x:auto-backport-of-pr-22002-on-v3.5.x

5. Create a PR against branch v3.5.x, I would have named this PR:

"Backport PR#22002 on branch v3.5.x (Fix TkAgg memory leaks and test for memory growth regressions)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove theStill Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free tosuggest an improvement.