8000 Make Tkagg blit thread safe by richardsheridan · Pull Request #18565 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Make Tkagg blit thread safe #18565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Nov 2, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 61 additions & 7 deletions lib/matplotlib/backends/_backend_tk.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,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 = "29345091836409813"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not directly initialize this to the final name here? e.g. "mpl_blit_" + uuid.uuid4().hex

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this idea, I always thought the initial name was tacky but for some reason I never came up with this fix!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick question, didn't we have a bug report come by recently because we were using one of the uuid functions, which was using a "weak" shasum algorithm, so their security-restricted system couldn't compute it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a pinch we could hardcode a uuid, they are universally unique after all...



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*.
Expand All @@ -53,7 +75,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]
Expand All @@ -65,11 +90,32 @@ 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

global _blit_tcl_name
try:
photoimage.tk.call(_blit_tcl_name, argsid)
except tk.TclError:
# register _blit with code copied from tkinter.Misc._register
_blit_tcl_name = repr(id(_blit)) + _blit.__name__
photoimage.tk.createcommand(_blit_tcl_name, _blit)
photoimage.tk.call(_blit_tcl_name, argsid)


class TimerTk(TimerBase):
Expand Down Expand Up @@ -402,10 +448,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()
Expand Down
8 changes: 3 additions & 5 deletions lib/matplotlib/backends/backend_tkagg.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
from . import _backend_tk
from .backend_agg import FigureCanvasAgg
from ._backend_tk import (
_BackendTk, FigureCanvasTk, FigureManagerTk, NavigationToolbar2Tk)
_BackendTk, FigureCanvasTk, FigureManagerTk, NavigationToolbar2Tk, blit)


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(
self._tkphoto, self.renderer._renderer, (0, 1, 2, 3), bbox=bbox)
blit(self._tkphoto, self.renderer._renderer, (0, 1, 2, 3), bbox=bbox)


@_BackendTk.export
Expand Down
86 changes: 85 additions & 1 deletion lib/matplotlib/tests/test_backends_interactive.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ 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
import sys
import threading
from unittest import TestCase

import matplotlib as mpl
Expand Down Expand Up @@ -173,6 +173,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.")
Expand Down
0