Repository: matplotlib/matplotlib
Pull request: Make Tkagg blit thread safe
## PR Summary
Closes #13293. (possibly?)  Use `tk.call` to invoke tkinter's cross-thread signalling when necessary to safely blit from threads. A small amount of architectural changes were made in the hopes of attaining actual CPU parallelism with threads, but that was unsuccessful because of how `Tk_PhotoPutBlock` is written upstream. 

Nevertheless this will prevent some silent crashes.

Downsides: I haven't done any work to prevent data races, and users that misuse the Tcl event system may run into issues discussed in https://github.com/python/cpython/pull/21299


Needs a test.
## PR Checklist

<!-- Please mark any checkboxes that do not apply to this PR as [N/A]. -->

- [x] Has pytest style unit tests (and `pytest` passes).
- [x] Is [Flake 8](https://flake8.pycqa.org/en/latest/) compliant (run `flake8` on changed files to check).
- [x] New features are documented, with examples if plot related.
- [ ] Documentation is sphinx and numpydoc compliant (the docs should [build](https://matplotlib.org/devel/documenting_mpl.html#building-the-docs) without error).
- [x] Conforms to Matplotlib style conventions (install `flake8-docstrings` and `pydocstyle<4` and run `flake8 --docstring-convention=all`).
- [ ] New features have an entry in `doc/users/next_whats_new/` (follow instructions in README.rst there).
- [ ] API changes documented in `doc/api/next_api_changes/` (follow instructions in README.rst there).

<!--
Thank you so much for your PR!  To help us review your contribution, please
consider the following points:

- A development guide is available at https://matplotlib.org/devdocs/devel/index.html.

- Help with git and github is available at
  https://matplotlib.org/devel/gitwash/development_workflow.html.

- Do not create the PR out of master, but out of a separate branch.

- The PR title should summarize the changes, for example "Raise ValueError on
  non-numeric input to set_xlim".  Avoid non-descriptive titles such as
  "Addresses issue #8576".

- The summary should provide at least 1-2 sentences describing the pull request
  in detail (Why is this change required?  What problem does it solve?) and
  link to any relevant issues.

- If you are contributing fixes to docstrings, please pay attention to
  http://matplotlib.org/devel/documenting_mpl.html#formatting.  In particular,
  note the difference between using single backquotes, double backquotes, and
  asterisks in the markup.

We understand that PRs can sometimes be overwhelming, especially as the
reviews start coming in.  Please let us know if the reviews are unclear or
the recommended next step seems overly demanding, if you would like help in
addressing a reviewer's comments, or if you have been waiting too long to hear
back on your PR.
-->


Problem statement:
FigureCanvasTkAgg.draw() silently crashes in threaded application
<!--To help us understand and resolve your issue, please fill out the form to the best of your ability.-->
<!--You can feel free to delete the sections that do not apply.-->

### Bug report

**Bug summary**

Matplotlib crashes python without rising any exceptions (python simply closes), when draw method is called on FigureCanvasTkAgg object in threaded application from thread other than main thread.

**Code for reproduction**

```python
from matplotlib.backends.backend_tkagg import FigureCanvasTkAgg
from matplotlib.figure import Figure
import tkinter as tk
from threading import Thread

def draw(canvas):
    t = Thread(target=canvas.draw)
    t.start()

root = tk.Tk()
fig = Figure()
canvas = FigureCanvasTkAgg(fig, master=root)
canvas.get_tk_widget().pack()
tk.Button(root, text='Do it!', command=lambda: draw(canvas)).pack()
root.mainloop()
```

**Actual outcome**

When button from example shown above is clicked, application window freezes for a moment and than closes along with python, no exception or error is displayed.

**Expected outcome**

Preferably draw method should run normally, or at least an exception should be rised.
In Matplotlib 2.2.2 same code snipped runs flawlessly.

**Matplotlib version**
  * Operating system: Windows 10 Home x64
  * Matplotlib version: 3.0.2
  * Matplotlib backend (`print(matplotlib.get_backend())`): TkAgg
  * Python version: 3.6.7
  * Other libraries: tkinter, threading

Matplotlib installed with 'pip install matplotlib'.

Hints from issue discussion:
Matplotlib makes no guarantees of thread safety. You'll need to use other methods to ensure that only one thread accesses it (conditions, mutexes, GUI-specific signals (if available), etc.)
```
19:59 $ python -X faulthandler  /tmp/tk_thread.py 
Fatal Python error: Segmentation fault

Current thread 0x00007f7029ab2700 (most recent call first):
  File "/home/tcaswell/mc3/envs/dd36/lib/python3.6/site-packages/matplotlib/backends/_backend_tk.py", line 91 in blit
  File "/home/tcaswell/mc3/envs/dd36/lib/python3.6/site-packages/matplotlib/backends/backend_tkagg.py", line 10 in draw
  File "/home/tcaswell/mc3/envs/dd36/lib/python3.6/threading.py", line 864 in run
  File "/home/tcaswell/mc3/envs/dd36/lib/python3.6/threading.py", line 916 in _bootstrap_inner
  File "/home/tcaswell/mc3/envs/dd36/lib/python3.6/threading.py", line 884 in _bootstrap

Thread 0x00007f7040fff740 (most recent call first):
  File "/home/tcaswell/mc3/envs/dd36/lib/python3.6/tkinter/__init__.py", line 1277 in mainloop
  File "/tmp/tk_thread.py", line 15 in <module>
Segmentation fault (core dumped)
```

Even though we do not claim that Matplotlib is threadsafe, there is no obvious race here and the sefault is coming from inside https://github.com/matplotlib/matplotlib/blob/5a53f5ef5aa848887f918067e38d9f314ea38131/src/_tkagg.cpp#L212-L250 so I am a bit worried that this is exposing a latent bug.  

We may also be hitting non-thread safe operations in tk?
You can get a backtrace via `catchsegv python ../mpl-tests/issue13293.py`:
```
.../conda/envs/mpl36/lib/python3.6/lib-dynload/../../libtk8.6.so(Tk_GetImageMasterData+0x11)[0x7f4d018b9821]
.../conda/envs/mpl36/lib/python3.6/lib-dynload/../../libtk8.6.so(Tk_FindPhoto+0xe)[0x7f4d018c4bee]
.../matplotlib/lib/matplotlib/backends/_tkagg.cpython-36m-x86_64-linux-gnu.so(+0x279e)[0x7f4d11ef379e]
```

Looking upstream at [this comment](https://bugs.python.org/issue11077#msg184521), there does appear to be some queuing involved and using the interpreter across threads is risky. Our `_tkagg` C code probably does not do that queuing.

Also according to a [later comment](https://bugs.python.org/issue11077#msg316449), Tk may or may not be compiled with thread support.
I also have this problem, calling everything from the same thread.
Newest matlablib and python, really tiered of it
AFAICT this is simply not supported by Tcl.  Per http://www.tcl.tk/doc/howto/thread_model.html:
> each interpreter is tightly bound to its OS thread and errors will occur if you let more than one thread call into the same interpreter (e.g., with Tcl_Eval).

We could start peppering the codebase with `if threading.current_thread() == threading.main_thread()` (in this specific case I guess in `_backend_tk.blit`, but I'm not sure it's so cheap?  And we are explicitly not threadsafe anyways (there are other places which I *know* have race conditions, e.g. likely most things involving _setattr_cm).

So I'll close as wontfix, but as usual feel free to disagree and reopen...
@xzores Is that thread the main thread?  In general GUI toolkits do not like having any part of them be run in a background thread (unless they manage it).
No it is in an other thread, but it is the same thread
I can't do "if threading.current_thread() == threading.main_thread()", because the main thread is used for tk.mainloop().
So I will never call matlablib from the main thread.
I just want to update a plot. The only way I found is to remove the plot entirely and then remake it, but that is not optimal, for many reasons
and I agree with the "can't fix" label, getting into thread safetly is starting to get into application logic and I suspect that any attempts on our part would invariably conflict with user's attempts to do the same in their application.
Use Tk's signaling system to ask the main thread to schedule a draw.

[edit to fix spelling]

Commit message: remove lingering threading import

Patch to review:
diff --git a/lib/matplotlib/backends/_backend_tk.py b/lib/matplotlib/backends/_backend_tk.py
index 69b57fc15d2a..326df873214e 100644
--- a/lib/matplotlib/backends/_backend_tk.py
+++ b/lib/matplotlib/backends/_backend_tk.py
@@ -1,3 +1,4 @@
+import uuid
 from contextlib import contextmanager
 import logging
 import math
@@ -44,6 +45,28 @@ def _restore_foreground_window_at_end():
             _c_internal_utils.Win32_SetForegroundWindow(foreground)
 
 
+_blit_args = {}
+# Initialize to a non-empty string that is not a Tcl command
+_blit_tcl_name = "mpl_blit_" + uuid.uuid4().hex
+
+
+def _blit(argsid):
+    """
+    Thin wrapper to blit via tkapp.call
+
+    *argsid* is a unique string identifier to fetch the correct arguments from
+    the _blit_args dict, since args cannot be passed directly
+
+    photoimage blanking must occur in the same event and thread as blitting
+    to avoid flickering.
+    """
+    photoimage, dataptr, offsets, bboxptr, blank = _blit_args.pop(argsid)
+    if blank:
+        photoimage.blank()
+    _tkagg.blit(
+        photoimage.tk.interpaddr(), str(photoimage), dataptr, offsets, bboxptr)
+
+
 def blit(photoimage, aggimage, offsets, bbox=None):
     """
     Blit *aggimage* to *photoimage*.
@@ -53,7 +76,10 @@ def blit(photoimage, aggimage, offsets, bbox=None):
     (2, 1, 0, 3) for little-endian ARBG32 (i.e. GBRA8888) data and (1, 2, 3, 0)
     for big-endian ARGB32 (i.e. ARGB8888) data.
 
-    If *bbox* is passed, it defines the region that gets blitted.
+    If *bbox* is passed, it defines the region that gets blitted. That region
+    will NOT be blanked before blitting.
+
+    Tcl events must be dispatched to trigger a blit from a non-Tcl thread.
     """
     data = np.asarray(aggimage)
     height, width = data.shape[:2]
@@ -65,11 +91,30 @@ def blit(photoimage, aggimage, offsets, bbox=None):
         y1 = max(math.floor(y1), 0)
         y2 = min(math.ceil(y2), height)
         bboxptr = (x1, x2, y1, y2)
+        blank = False
     else:
-        photoimage.blank()
         bboxptr = (0, width, 0, height)
-    _tkagg.blit(
-        photoimage.tk.interpaddr(), str(photoimage), dataptr, offsets, bboxptr)
+        blank = True
+
+    # NOTE: _tkagg.blit is thread unsafe and will crash the process if called
+    # from a thread (GH#13293). Instead of blanking and blitting here,
+    # use tkapp.call to post a cross-thread event if this function is called
+    # from a non-Tcl thread.
+
+    # tkapp.call coerces all arguments to strings, so to avoid string parsing
+    # within _blit, pack up the arguments into a global data structure.
+    args = photoimage, dataptr, offsets, bboxptr, blank
+    # Need a unique key to avoid thread races.
+    # Again, make the key a string to avoid string parsing in _blit.
+    argsid = repr(id(args))
+    _blit_args[argsid] = args
+
+    try:
+        photoimage.tk.call(_blit_tcl_name, argsid)
+    except tk.TclError:
+        # register _blit with code copied from tkinter.Misc._register
+        photoimage.tk.createcommand(_blit_tcl_name, _blit)
+        photoimage.tk.call(_blit_tcl_name, argsid)
 
 
 class TimerTk(TimerBase):
@@ -402,10 +447,18 @@ def destroy(self, *args):
         if self.canvas._idle_callback:
             self.canvas._tkcanvas.after_cancel(self.canvas._idle_callback)
 
-        self.window.destroy()
+        # NOTE: events need to be flushed before issuing destroy (GH #9956),
+        # however, self.window.update() can break user code. This is the
+        # safest way to achieve a complete draining of the event queue,
+        # but it may require users to update() on their own to execute the
+        # completion in obscure corner cases.
+        def delayed_destroy():
+            self.window.destroy()
+
+            if self._owns_mainloop and not Gcf.get_num_fig_managers():
+                self.window.quit()
 
-        if self._owns_mainloop and not Gcf.get_num_fig_managers():
-            self.window.quit()
+        self.window.after_idle(delayed_destroy)
 
     def get_window_title(self):
         return self.window.wm_title()
diff --git a/lib/matplotlib/backends/backend_tkagg.py b/lib/matplotlib/backends/backend_tkagg.py
index 484d5a40edbe..ca15bf818142 100644
--- a/lib/matplotlib/backends/backend_tkagg.py
+++ b/lib/matplotlib/backends/backend_tkagg.py
@@ -7,7 +7,7 @@
 class FigureCanvasTkAgg(FigureCanvasAgg, FigureCanvasTk):
     def draw(self):
         super().draw()
-        _backend_tk.blit(self._tkphoto, self.renderer._renderer, (0, 1, 2, 3))
+        self.blit()
 
     def blit(self, bbox=None):
         _backend_tk.blit(
diff --git a/lib/matplotlib/tests/test_backends_interactive.py b/lib/matplotlib/tests/test_backends_interactive.py
index 000ff971bc97..ecf9c513bd1b 100644
--- a/lib/matplotlib/tests/test_backends_interactive.py
+++ b/lib/matplotlib/tests/test_backends_interactive.py
@@ -67,7 +67,6 @@ def _get_testable_interactive_backends():
 # early.  Also, gtk3 redefines key_press_event with a different signature, so
 # we directly invoke it from the superclass instead.
 _test_script = """\
-import importlib
 import importlib.util
 import io
 import json
@@ -173,6 +172,90 @@ def test_interactive_backend(backend, toolbar):
     assert proc.stdout.count("CloseEvent") == 1
 
 
+_thread_test_script = """\
+import json
+import sys
+import threading
+
+from matplotlib import pyplot as plt, rcParams
+rcParams.update({
+    "webagg.open_in_browser": False,
+    "webagg.port_retries": 1,
+})
+if len(sys.argv) >= 2:  # Second argument is json-encoded rcParams.
+    rcParams.update(json.loads(sys.argv[1]))
+
+# Test artist creation and drawing does not crash from thread
+# No other guarantees!
+fig, ax = plt.subplots()
+# plt.pause needed vs plt.show(block=False) at least on toolbar2-tkagg
+plt.pause(0.5)
+
+exc_info = None
+
+def thread_artist_work():
+    try:
+        ax.plot([1,3,6])
+    except:
+        # Propagate error to main thread
+        import sys
+        global exc_info
+        exc_info = sys.exc_info()
+
+def thread_draw_work():
+    try:
+        fig.canvas.draw()
+    except:
+        # Propagate error to main thread
+        import sys
+        global exc_info
+        exc_info = sys.exc_info()
+
+t = threading.Thread(target=thread_artist_work)
+t.start()
+# artists never wait for the event loop to run, so just join
+t.join()
+
+if exc_info:  # Raise thread error
+    raise exc_info[1].with_traceback(exc_info[2])
+
+t = threading.Thread(target=thread_draw_work)
+fig.canvas.mpl_connect("close_event", print)
+t.start()
+plt.pause(0.5)  # flush_events fails here on at least Tkagg (bpo-41176)
+t.join()
+plt.close()
+fig.canvas.flush_events()  # pause doesn't process events after close
+
+if exc_info:  # Raise thread error
+    raise exc_info[1].with_traceback(exc_info[2])
+"""
+
+_thread_safe_backends = _get_testable_interactive_backends()
+# Known unsafe backends. Remove the xfails if they start to pass!
+if "wx" in _thread_safe_backends:
+    _thread_safe_backends.remove("wx")
+    _thread_safe_backends.append(
+        pytest.param("wx", marks=pytest.mark.xfail(
+            raises=subprocess.CalledProcessError, strict=True)))
+if "macosx" in _thread_safe_backends:
+    _thread_safe_backends.remove("macosx")
+    _thread_safe_backends.append(
+        pytest.param("macosx", marks=pytest.mark.xfail(
+            raises=subprocess.TimeoutExpired, strict=True)))
+
+
+@pytest.mark.parametrize("backend", _thread_safe_backends)
+@pytest.mark.flaky(reruns=3)
+def test_interactive_thread_safety(backend):
+    proc = subprocess.run(
+        [sys.executable, "-c", _thread_test_script],
+        env={**os.environ, "MPLBACKEND": backend, "SOURCE_DATE_EPOCH": "0"},
+        timeout=_test_timeout, check=True,
+        stdout=subprocess.PIPE, universal_newlines=True)
+    assert proc.stdout.count("CloseEvent") == 1
+
+
 @pytest.mark.skipif('TF_BUILD' in os.environ,
                     reason="this test fails an azure for unknown reasons")
 @pytest.mark.skipif(os.name == "nt", reason="Cannot send SIGINT on Windows.")


Code review comments:
diff_hunk: @@ -65,11 +91,30 @@ def blit(photoimage, aggimage, offsets, bbox=None):
         y1 = max(math.floor(y1), 0)
         y2 = min(math.ceil(y2), height)
         bboxptr = (x1, x2, y1, y2)
+        blank = False
     else:
-        photoimage.blank()
         bboxptr = (0, width, 0, height)
-    _tkagg.blit(
-        photoimage.tk.interpaddr(), str(photoimage), dataptr, offsets, bboxptr)
+        blank = True
+
+    # NOTE: _tkagg.blit is thread unsafe and will crash the process if called
+    # from a thread (GH#13293). Instead of blanking and blitting here,
+    # use tkapp.call to post a cross-thread event if this function is called
+    # from a non-Tcl thread.
+
+    # tkapp.call coerces all arguments to strings, so to avoid string parsing
+    # within _blit, pack up the arguments into a global data structure.
+    args = photoimage, dataptr, offsets, bboxptr, blank
+    # Need a unique key to avoid thread races.
+    # Again, make the key a string to avoid string parsing in _blit.
+    argsid = repr(id(args))
path: lib/matplotlib/backends/_backend_tk.py
text: @user1:
```suggestion
    argsid = str(id(args))
```
Technically the same, but str may be slightly clearer.