fix FigureManagerTk close behavior if embedded in Tk App

[richardsheridan]

PR Summary

Fixes #13470. I test to see if the Tk mainloop was running before FigureManagerTk was created and if that was the case, then I don't call the window.quit method.

The solution relies on implementation details of the tkinter c module and a disgusting hack using threads, so I have made this PR a draft. Really, the functionality of my is_tk_mainloop_running function should be provided as a method on _tkinter.tkapp AKA TkappObject to access the dispatching struct member from Python, but that's a job for cPython upstream.

I suppose there could be an alternative method involving traversing the children of the _default_root, or parents of the figure window but that feels just as much like leaning on implementation details and more error prone.

If there is any interest in maintaining the "fix" within matplotlib, I'll work on this PR further, otherwise this and #13470 should be closed pending upstream changes.

PR Checklist

• Has Pytest style unit tests
• Code is Flake 8 compliant
• New features are documented, with examples if plot related
• Documentation is sphinx and numpydoc compliant
• Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
• Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

[anntzer]

how does this compare with https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/cbook/__init__.py#L65 (possibly combined with checking that the thread matches threading.main_thread())?

[richardsheridan]

In the contexts that I call is_tk_mainloop_running, _get_running_interactive_framework returns None, not "tk". This is because _get_running_interactive_framework is incorrectly implemented. There are several mainloop functions in various classes in tkinter and it only checks for one of them. Importantly one of them is a built-in method that has no __code__ attribute to check ("built-in method mainloop of _tkinter.tkapp object") so i'm not sure how to check for it. It would be a rare case to call that method directly, however, so maybe it would be ok to overlook?

[anntzer]

I see your point. Adding a check for Misc.mainloop as you did certainly seems better. I don't have a good solution for the general case, though.

[tacaswell]

Thanks for working on this @richardsheridan !

I am not sure that this will work, consider the sequence of:

• user creates a window
• calls plt.show(block=True) which starts the tk event loop
• a callback triggered from inside the first figure creates a second
• the user closes the first figure (which does not try to quit the tk event loop because there are still other of our figures open)
• the user then closes the second figure (which does not quit the event loop because it detected the loop was running when it was created)
• the user now has a hung python session

I think this is going the right direction, but instead of detecting on a per-figure-manager basis if it should quit the tk event loop we should do the check in mainloop or show and then "arm" the Figure managers with some global state (probably stash it on FigureManagerTk as a class level attribute?) that they should quit the tkmain loop when the last of them exits (and then flip the state back to "not armed") thus we will only try to stop the tk event loop if we are sure that we started it.

That is more global state (which I'm not wild about), but GUI frameworks seem to be an endless source of global state so one more bit won't do any harm....

[richardsheridan]

For what it's worth, I'm not sure your sequence demonstrates an actual issue. Yes, the REPL hangs if you open a tkinter window and then show(block=True) and close a figure:

import tkinter
import matplotlib.pyplot as plt
root= tkinter.Tk()
plt.plot([1,2,3],[1,2,5])
plt.show(block=True)

However, it is not necessary to call plt.show(block=True) from the REPL due to the EventHook functionality of tkinter and the behavior of plt.ion(). The expected behavior occurs when run as a script (you have to press the close button of all the windows to exit the script)

I'll try arming quit upon mainloop on my next pass at this PR.

[tacaswell]

However, it is not necessary to call plt.show(block=True) from the REPL due to the EventHook functionality of tkinter and the behavior of plt.ion().

While true, we definitely have users that want to work in a terminal and do not use plt.ion().

That aside, the same scenario in a script is a much better story (user wants to stop the script, open a plot (that spawns more figures) and if the windows get closed in the wrong order the script hangs forever).

[richardsheridan]

This idea came out very elegantly, as long as there are no unexpected wider behavior changes. I added a test that is more minimal than the MVE shown in #13470. For the record, this test also passes when performed on the REPL as well, except the root window hangs around due to EventHook. This can be fixed with an additional destroy in the legitimate_quit but I'm not sure how to automatically test REPL behavior??

This does not fix the plt.show(block=True) hang but IMO that is a separate issue. Should we make it literally a separate issue on github or fully fix it on this PR?

The fix to _get_running_interactive_framework remains even though now totally unrelated.

[tacaswell]

@richardsheridan I took the liberty of fixing the style issues in the test, I hope you don't mind.

[tacaswell]

This can be fixed with an additional destroy in the legitimate_quit but I'm not sure how to automatically test REPL behavior??

While testing is good, there is a lot of code in the GUI / interactive / repl space that we do not have tests on because the trade off of the effort to write the test vs the benefit is not worth it. I think that this is in that category.

This does not fix the plt.show(block=True) hang but IMO that is a separate issue.

I'm not sure how to get the hang any more though, the following:

import matplotlib.pyplot as plt
plt.ion()
fig, ax = plt.subplots()
fig.canvas.mpl_connect('button_press_event', lambda *args, **kwargs: plt.figure())
plt.show(block=True)

sets it up so if you click anywhere on the first figure it spawns more. You can close them in any order and it seems to work (which is the case I was worried about).

If you call plt.show(block=True) from of a running tk mainloop than you are trying to start a tk mainloop inside of a tk mainloop which I am not convinced has well defined behavior (I know Qt has the ability to run a second event loop inside of the first, but that is not what we are doing here?) If that is the only problem we have left, I am happy to defer it to a later issue.

[richardsheridan]

I discovered two bugs in the PR, which I believe the additional commits address. Managers were not quitting loops that they had responsibility for. I cherry picked a commit from my other PR #17789 that corrects FigureManager destruction behavior to fix that.

This appears to also fix the plt.show(block=True) hang in REPL issue.

[tacaswell]

One thing we have found with CI is that you need to use rather extreme timeouts, things take longer than "reasonable" because they are running on very over-subscribed machines at low-priority (the service is free so we can not complain too much).

How were you getting the repl to hang?

[richardsheridan]

It seems there is some kind of deeper race condition going on. Before the fix, there a spurious green zone around 0.1 seconds of waiting that will make the test pass and more or less than that everything hangs unless Tcl recieves some user events (i.e. mouseover the remaining window). I tried a dozen different things to iron this out and nothing was working. Now I finally understand why that test in test_backends_interactive.py was running in a subprocess...

[richardsheridan]

How were you getting the repl to hang?

See my comment above, run those on the REPL and simply close the figure window but not the Tk root window. On 3860b21 this hangs but on bea3a3f you can enter commands again.

[tacaswell]

We used to drop the window, presumably to avoid calling destroy() on it twice (?). Is that a concern we should have?

[richardsheridan]

Calling destroy twice was an artifact of mistakenly binding instead of using protocol. It should be fine now.

[tacaswell]

One small question, but still approve!

[tacaswell]

Ah, thanks! Sorry I was being dense.

Thank you for working on this @richardsheridan ! I will try to take a look at your other PR tomorrow.

[QuLogic]

Thanks @richardsheridan! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.