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 uses pytest.importorskip https://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 does window_dpi = tk.IntVar(master=window, ...) and self._window_dpi = window_dpi), so it should go away with it.

I suppose there's also the additional Font object on the toolbar as well, but adding an explicit del self.toolbar._label_font to delayed_destroy doesn't do anything.

If I run memleak.py with the following patch:

$ git diff
diff --git a/tools/memleak.py b/tools/memleak.py
index 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 explicit gc.collect calls. With garbage collection, it seemed to think that FigureCanvasTk._tkphoto and FigureCanvasTk._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 about memleak.py. The ever-increasing not-garbage objects before this PR was due to tkinter hanging on to various callbacks. I identified them using objgraph.show_growth and following objgraph.show_backrefs from them. Confusingly they lead to nowhere, and they also show up with objgraph.get_leaking_objects. They aren't actually leaked, though... they are being legitimately tracked by the tkinter c module. these are mostly cleared via running the event loop in the patch.

I say mostly, but actually all were gone but one; the CallWrapper associated with FigureManagerTk._update_window_dpi. This CallWrapper is created by the trace_add command which registers the callback, and, much to my surprise, does not die with the IntVar either on destroy. 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 in a3016af, and accellerate the garbage collection in e6e4cad. This gave me flat graphs for memory and object behavior in memleak.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 change memleak.py a bit more:

$  git diff
diff --git a/tools/memleak.py b/tools/memleak.py
index 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 missing update command locally, but it must have something to do with the difference between flush_events and pause 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.

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) (+ a gc.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.x
git 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 the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.