Make Tkagg blit thread safe by richardsheridan · Pull Request #18565 · matplotlib/matplotlib · GitHub

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 python/cpython#21299

Needs a test.

PR Checklist

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
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).

@xzores can you install matplotlib from this PR and see if it prevents your crashes?

pip install git+https://github.com/richardsheridan/matplotlib.git@tk_blit_thread_safe

pip install git+https://github.com/richardsheridan/matplotlib.git@tk_blit_thread_safe

I found another way. I don't really want to undo my code, so I will have no way to test.

I'm always happy to be wrong about this sort of thing 😄

I assume that mpl_tk_blit can go away now?

Indeed, this appears to work. But I think the logic should go into _backend_tk.blit instead, so that it also works for tkcairo (and mplcairo.tk :))?

It was a bit tricky to ensure that the right arguments connect with the right call to _tkagg.blit. Unfortunately PhotoImage is a kind of incomplete widget that doesn't have a register method or parent widget attribute, so it is necessary to pass in some other, unrelated widget.If anyone can think of a smarter way to get to the root widget, I'm open to it! nm i thought of one

My test creates some stderr spam from a ttk widget being destroyed late, but I can't for the life of me figure out how to fix it. It would also be nice to get coverage on the code where bbox is not none, but I don't think it should block this PR.

The stderr spam will probably look familiar to some:

can't invoke "event" command: application has been destroyed
    while executing
"event generate $w <<ThemeChanged>>"
    (procedure "ttk::ThemeChanged" line 6)
    invoked from within
"ttk::ThemeChanged"

It can be silenced by adding an update_idletasks as per #9956 but I will try a bit more to see if we can avoid that route.

Some interesting failures on travis. I'm not sure what webagg has to do with anything, but some segfaults happen on py38 that don't happen on py37. Possibly I can guard that section of the test from running on the deprecated backend = 'wx'?

current status: doesn't crash but also doesn't draw when called from a thread

current status: Prone to races and hard-to-diagnose timeouts (see python/cpython#21299)

current status: Relies on tkinter arcana, untested

drawing from a thread produces an intolerable flicker unless blank and blit are called from the same event in the main thread

ignore wx

@tacaswell @anntzer This PR is consistently green on CI now, if you want to have another look.

I left it up to some other enterprising hacker to fix wx or macosx if they need it and make a separate PR.

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

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

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?

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

Just some minor points, but mostly looks good.

conditional on handling the comments above.

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

richardsheridan mentioned this pull request Sep 24, 2020

FigureCanvasTkAgg.draw() silently crashes in threaded application #13293

Closed

QuLogic added the GUI: tk label Sep 24, 2020

richardsheridan changed the title Tk blit thread safe Make Tkagg blit thread safe Sep 24, 2020

tacaswell added this to the v3.4.0 milestone Sep 25, 2020

richardsheridan marked this pull request as ready for review September 25, 2020 20:35

richardsheridan added 13 commits October 24, 2020 07:59

WIP: Try to make tkagg blitting threadsafe

0dcdd21

current status: doesn't crash but also doesn't draw when called from a thread

make tkagg blitting threadsafe

2ad422a

current status: Prone to races and hard-to-diagnose timeouts (see python/cpython#21299)

revert to unified blit code and prevent some races

a4a920e

current status: Relies on tkinter arcana, untested

register _blit with Tcl without an extra widget

321b7a0

current status: Relies on tkinter arcana, untested

docstring

24d2545

missing id argument

7196d37

make blank occur in same event as blit

b63950e

drawing from a thread produces an intolerable flicker unless blank and blit are called from the same event in the main thread

test for thread safety

9180607

improve thread safety test

e692b62

fix tcl errors by delaying the widget destruction

e201189

blit docs & comments

8c979cc

appease CI

3463148

Split thread tests from other interactive tests

fd97233

ignore wx

richardsheridan force-pushed the tk_blit_thread_safe branch from ef900b6 to fd97233 Compare October 24, 2020 13:15

richardsheridan added 2 commits October 24, 2020 10:59

ignore -> xfail for wx and macosx

bbb82ac

increase event loop time for CI, cleanup imports

59d5c06

anntzer reviewed Oct 27, 2020

View reviewed changes

Comment thread lib/matplotlib/backends/backend_tkagg.py

anntzer reviewed Oct 27, 2020

View reviewed changes

Comment thread lib/matplotlib/backends/_backend_tk.py Outdated

anntzer approved these changes Oct 27, 2020

View reviewed changes

richardsheridan added 2 commits October 27, 2020 08:44

respond to comments

9255fb6

remove lingering threading import

d1984c5

timhoffm reviewed Oct 27, 2020

View reviewed changes

Comment thread lib/matplotlib/backends/_backend_tk.py Outdated

Comment thread lib/matplotlib/backends/_backend_tk.py Outdated

Comment thread lib/matplotlib/backends/_backend_tk.py Outdated

Comment thread lib/matplotlib/backends/_backend_tk.py Outdated

richardsheridan and others added 2 commits October 27, 2020 09:55

Apply suggestions from code review

78a1e43

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

narrow TclError handling

0b1660b

timhoffm approved these changes Nov 2, 2020

View reviewed changes

timhoffm merged commit 59bfcfa into matplotlib:master Nov 2, 2020

richardsheridan deleted the tk_blit_thread_safe branch November 2, 2020 16:17

richardsheridan mentioned this pull request Aug 15, 2021

Speed up Tkagg blit with Tk_PhotoPutBlock #20840

Merged

3 tasks done

richardsheridan mentioned this pull request Aug 24, 2021

Implement a consistent behavior in TkAgg backend for bad blit bbox #20887

Merged

7 tasks

+              _blit_args = {}
+              # Initialize to a non-empty string that is not a Tcl command
+              _blit_tcl_name = "29345091836409813"

Make Tkagg blit thread safe by richardsheridan · Pull Request #18565 · matplotlib/matplotlib · GitHub

Repository navigation

Make Tkagg blit thread safe - #18565

Make Tkagg blit thread safe#18565timhoffm merged 19 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom richardsheridan:tk_blit_thread_saferichardsheridan/matplotlib:tk_blit_thread_safeCopy head branch name to clipboard

Conversation

richardsheridan commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

richardsheridan commented Sep 24, 2020

Uh oh!

xzores commented Sep 24, 2020

Uh oh!

tacaswell commented Sep 25, 2020

Uh oh!

anntzer commented Sep 25, 2020

Uh oh!

richardsheridan commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardsheridan commented Sep 25, 2020

Uh oh!

richardsheridan commented Sep 27, 2020

Uh oh!

richardsheridan commented Sep 28, 2020

Uh oh!

richardsheridan commented Oct 26, 2020

Uh oh!

Uh oh!

anntzer Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

richardsheridan Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

WeatherGod Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

richardsheridan Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anntzer commented Oct 27, 2020

Uh oh!

anntzer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Make Tkagg blit thread safe#18565
timhoffm merged 19 commits into
matplotlib:masterfrom
richardsheridan:tk_blit_thread_safe

richardsheridan commented Sep 24, 2020 •
edited
Loading

richardsheridan commented Sep 25, 2020 •
edited
Loading

anntzer left a comment •
edited
Loading