Skip to content

gh-83274: Stop deallocation of Tkapp causing Python interpreter crash#21532

Open
E-Paine wants to merge 14 commits into
python:mainfrom
E-Paine:tkapp-gc
Open

gh-83274: Stop deallocation of Tkapp causing Python interpreter crash#21532
E-Paine wants to merge 14 commits into
python:mainfrom
E-Paine:tkapp-gc

Conversation

@E-Paine
Copy link
Copy Markdown
Contributor

@E-Paine E-Paine commented Jul 18, 2020

I am pretty certain this is not the correct usage of PyErr_WriteUnraisable so please could someone pay particular attention to that (it was required to stop Python raising a SystemError). It appears to behave correctly during tests, but I am not yet confident in it and so have, for now, marked it as a draft.

https://bugs.python.org/issue39093

Comment thread Modules/_tkinter.c
@E-Paine
Copy link
Copy Markdown
Contributor Author

E-Paine commented Jul 19, 2020

@richardsheridan, do you mind please reviewing the new test. A particular note: can you think of a way for this test to not require a subprocess? (we need to test for the crash without crashing the main test interpreter)

@richardsheridan
Copy link
Copy Markdown

richardsheridan commented Jul 19, 2020

  1. I think you should use a threading.Event instead of a timer to make sure the thread reference outlives the main reference. This would save some time.
  2. Sadly, I too think you'll need a subprocess to avoid crashing the main test interpreter if we experience a regression. However, do you really need to avoid that crash? I used a subprocess for a test (fix FigureManagerTk close behavior if embedded in Tk App matplotlib/matplotlib#17802) because the regression failure lead to an infinite hang which made CI waste a lot of time (20 minute timeout...). The interpreter crash at least gives immediate feedback even if the test runner doesn't print out all the nice messages. You've already fixed the bug so the happy path should be as fast as possible, and the single leaked tcl interpreter is mostly harmless. I think this test could just be on the main MiscTest class, as long as you put a new self.root on when you're done.

@E-Paine
Copy link
Copy Markdown
Contributor Author

E-Paine commented Jul 19, 2020

  1. Thank you for your idea on using threading events: I have now used two of them to replace both time.sleeps (writing this, I remember I can now remove that import!)
  2. I feel we need to give the standard "Test: FAIL" rather than just crashing the main test interpreter. I have tested the subprocess on a build without the _tkinter changes and it did not cause the test to hang (I cannot think of a situation where it would). For the sake of conformity, I will stick by the fact that it should not crash the main test interpreter (though, when tested, it did give a non-zero return-code meaning the test would show as failed).

@E-Paine E-Paine marked this pull request as ready for review July 20, 2020 08:44
Comment thread Lib/tkinter/test/test_tkinter/test_misc.py Outdated
@erlend-aasland erlend-aasland changed the title bpo-39093: Stop deallocation of Tkapp causing Python interpreter crash gh-83274: Stop deallocation of Tkapp causing Python interpreter crash Jun 12, 2023
@python-cla-bot
Copy link
Copy Markdown

python-cla-bot Bot commented Apr 18, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 8, 2026

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review stale Stale PR or inactive for long period of time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants