Closes#13293. (possibly?) Usetk.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 howTk_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 inpython/cpython#21299

Needs a test.

PR Checklist

Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (runflake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
Conforms to Matplotlib style conventions (installflake8-docstrings andpydocstyle<4 and runflake8 --docstring-convention=all).
New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

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

Copy link

ContributorAuthor

richardsheridan commentedSep 24, 2020

@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

Copy link

xzores commentedSep 24, 2020

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.

richardsheridan changed the title~~Tk blit thread safe~~Make Tkagg blit thread safe

Sep 24, 2020

Copy link

Member

tacaswell commentedSep 25, 2020

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

I assume thatmpl_tk_blit can go away now?

tacaswell added this to thev3.4.0 milestone

Sep 25, 2020

Copy link

Contributor

anntzer commentedSep 25, 2020

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 :))?

Copy link

ContributorAuthor

richardsheridan commentedSep 25, 2020•
edited
Loading

It was a bit tricky to ensure that the right arguments connect with the right call to_tkagg.blit. UnfortunatelyPhotoImage 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

richardsheridan marked this pull request as ready for review

September 25, 2020 20:35

Copy link

ContributorAuthor

richardsheridan commentedSep 25, 2020

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.

Copy link

ContributorAuthor

richardsheridan commentedSep 27, 2020

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 anupdate_idletasks as per#9956 but I will try a bit more to see if we can avoid that route.

Copy link

ContributorAuthor

richardsheridan commentedSep 28, 2020

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'?

richardsheridan added13 commits

October 24, 2020 07:59

WIP: Try to make tkagg blitting threadsafe

0dcdd21

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

make tkagg blitting threadsafe

2ad422a

current status: Prone to races and hard-to-diagnose timeouts(seepython/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 thetk_blit_thread_safe branch fromef900b6 tofd97233Compare

October 24, 2020 13:15

richardsheridan added2 commits

October 24, 2020 10:59

ignore -> xfail for wx and macosx

bbb82ac

increase event loop time for CI, cleanup imports

59d5c06

Copy link

ContributorAuthor

richardsheridan commentedOct 26, 2020

@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.

anntzer reviewed

Oct 27, 2020

View reviewed changes

lib/matplotlib/backends/backend_tkagg.py OutdatedShow resolvedHide resolved

anntzer reviewed

Oct 27, 2020

View reviewed changes

lib/matplotlib/backends/_backend_tk.py Outdated

		@@ -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

anntzerOct 27, 2020

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

ContributorAuthor

richardsheridanOct 27, 2020

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

WeatherGodOct 27, 2020

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

ContributorAuthor

richardsheridanOct 27, 2020

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...

anntzer reviewed

Oct 27, 2020

View reviewed changes

lib/matplotlib/backends/_backend_tk.py OutdatedShow resolvedHide resolved

Copy link

Contributor

anntzer commentedOct 27, 2020

Just some minor points, but mostly looks good.

anntzer approved these changes

Oct 27, 2020

View reviewed changes

Copy link

Contributor

anntzer left a comment•
edited
Loading

Choose a reason for hiding this comment

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

conditional on handling the comments above.

richardsheridan added2 commits

October 27, 2020 08:44

respond to comments

9255fb6

remove lingering threading import

d1984c5

timhoffm reviewed

Oct 27, 2020

View reviewed changes

lib/matplotlib/backends/_backend_tk.py OutdatedShow resolvedHide resolved

richardsheridanand others added2 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 commit59bfcfa intomatplotlib: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

richardsheridan mentioned this pull request

Aug 24, 2021

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

Merged

7 tasks

Labels

GUI: tk

7 participants

Movatterモバイル変換

Uh oh!

Make Tkagg blit thread safe#18565

Make Tkagg blit thread safe#18565

Uh oh!

Conversation

richardsheridan commentedSep 24, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

richardsheridan commentedSep 24, 2020

Uh oh!

xzores commentedSep 24, 2020

Uh oh!

tacaswell commentedSep 25, 2020

Uh oh!

anntzer commentedSep 25, 2020

Uh oh!

richardsheridan commentedSep 25, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

richardsheridan commentedSep 25, 2020

Uh oh!

richardsheridan commentedSep 27, 2020

Uh oh!

richardsheridan commentedSep 28, 2020

Uh oh!

richardsheridan commentedOct 26, 2020

Uh oh!

Uh oh!

anntzerOct 27, 2020

Choose a reason for hiding this comment

Uh oh!

richardsheridanOct 27, 2020

Choose a reason for hiding this comment

Uh oh!

WeatherGodOct 27, 2020

Choose a reason for hiding this comment

Uh oh!

richardsheridanOct 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anntzer commentedOct 27, 2020

Uh oh!

anntzer left a comment• editedLoading 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!

Uh oh!

richardsheridan commentedSep 24, 2020•
edited
Loading

richardsheridan commentedSep 25, 2020•
edited
Loading

anntzer left a comment•
edited
Loading