Skip to content

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

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

E-Paine
Copy link
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

@E-Paine
Copy link
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

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

python-cla-bot bot commented Apr 18, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants