gh-116946: fully implement GC protocol for `_tkinter.Tk{app,tt}Object`#138331

picnixz · 2025-09-01T09:43:25Z

tkinter.Tcl_Obj is empty but not immutable so we make it immutable to avoid the needs of GC protocol.
_tkinter.tktimertoken and _tkinter.tkapp are not immutable but they are not empty either as they have a strong reference to a callable, so we should still make them support the full GC protocol.

Issue: Heap types implemented in C must have Py_TPFLAGS_HAVE_GC and implement tp_traverse #116946

Modules/_tkinter.c

serhiy-storchaka

No need to use tp_alloc and tp_free, because these types are not valid base types. Just make them immutable.

picnixz · 2025-09-06T11:00:13Z

No need to use tp_alloc and tp_free, because these types are not valid base types. Just make them immutable.

Doesn't it mean I would leave the possibility of introducing a cycle in bug fix versions and simply make them all immutable in main? unlike the select.[e]poll and _random.Random, the trace function can have a reference to the current app.

Now, you added tk.settrace as an experimental feature so it's internal only, but I believe that making it non-experimental would still require tp_clear/tp_traverse (and thus tp_alloc/tp_free as recommended by the docs; otherwise I would need GC_New/GC_Del + track).

serhiy-storchaka · 2025-09-06T12:09:21Z

Modules/_tkinter.c

+ type = (PyTypeObject *)Tktt_Type;
+ assert(type != NULL);
+ assert(type->tp_alloc != NULL);
+ v = (TkttObject *)type->tp_alloc(type, 0);


All these types cannot be subclassed. So we can simply replace PyObject_New/PyObject_Free with PyObject_GC_New/PyObject_GC_Del. I think that this would look clearer than with tp_alloc/tp_free.

Yes, but now the docs explicitly say "don't call those functions directly, use tp_alloc/tp_free". I agree that when possible, we could directly call them, but @ZeroIntensity suggested to follow the docs here.

I like using tp_alloc/tp_free more because it's less refactoring if we need to change the type flags. People follow CPython's source code for inspiration in their extensions, so we should follow what's documented. If we want to change the preference, let's update the docs.
Alternatively, we could have a function like this for limited API users:
PyObject*PyType_InvokeAlloc(PyTypeObject*tp){assert(tp!=NULL); assert(type->tp_alloc!=NULL); returntype->tp_alloc(type, 0)}

At this point, I would prefer having a macro to avoid the extra cast everytime... Something that unifies PyObject_[GC_]New(typename, type).

picnixz · 2025-09-06T12:12:22Z

Ok, now I'm utterly confused because I don't understand why I get a segfault. The segfault happens in PyObject_GC_UnTrack

vstinner

LGTM

vstinner · 2025-09-08T09:43:05Z

Modules/_tkinter.c

+ type = (PyTypeObject *)Tkapp_Type;
+ assert(type != NULL);
+ assert(type->tp_alloc != NULL);
+ v = (TkappObject *)type->tp_alloc(type, 0);


I would prefer to move the variable definition aside to their first assignment:
Suggested change
type= (PyTypeObject*)Tkapp_Type;
assert(type!=NULL);
assert(type->tp_alloc!=NULL);
v= (TkappObject*)type->tp_alloc(type, 0);
PyTypeObject*type= (PyTypeObject*)Tkapp_Type;
assert(type!=NULL);
assert(type->tp_alloc!=NULL);
TkappObject*v= (TkappObject*)type->tp_alloc(type, 0);
Same below.

I followed the existing style. Do you want me to move the char *arg as well?

Do you want me to move the char *arg as well?
As you want. You can leave argv0 as it is to keep the diff small.

I think I would be more comfortable not changing the style of this function. It's a long function, and if we were to use gotos instead for some refactoring, we could benefit for having everything declared at the top. Do you mind I leave it as is?

serhiy-storchaka

I am not comfortable with replacing a single line with 5 lines. Why not simply use public API?

picnixz · 2025-09-10T12:12:20Z

Well.. that's what I wanted to do but now the docs say to do things differently and considering people take inspiration for their extension modules with CPython code it'd be better to match our recommendations. Personally, I would prefer using the exact functions instead of tp_free/tp_alloc.

ZeroIntensity · 2025-09-10T12:14:42Z

Please create a DPO post to gauge feedback on whether to use tp_alloc/tp_free vs manual allocation functions.

serhiy-storchaka · 2025-09-10T12:17:49Z

Well, if you want to use tp_alloc/tp_free, can you just use them, without adding unnecessary lines?

serhiy-storchaka · 2025-09-10T12:23:27Z

Modules/_tkinter.c

+ type = (PyTypeObject *)Tktt_Type;
+ assert(type != NULL);
+ assert(type->tp_alloc != NULL);
+ v = (TkttObject *)type->tp_alloc(type, 0);


Suggested change
type= (PyTypeObject*)Tktt_Type;
assert(type!=NULL);
assert(type->tp_alloc!=NULL);
v= (TkttObject*)type->tp_alloc(type, 0);
v= (TkttObject*)((PyTypeObject*)Tktt_Type)->tp_alloc(type, 0);

Unfortunately, we would need
(TkttObject*)((PyTypeObject*)Tktt_Type)->tp_alloc((PyTypeObject*)Tktt_Type, 0);
which is a bit unreadable IMO. So I'll keep the temporary type variable. I would prefer having a macro for calling tp_alloc because it becomes quite annoying if we need to cast to PyTypeObject* everytime twice.
#define_PyObject_New(T, type) \ ((T *)((PyTypeObject *)type)->tp_alloc((PyTypeObject *)type, 0))

Modules/_tkinter.c

serhiy-storchaka · 2025-09-10T12:25:47Z

Modules/_tkinter.c

+ PyTypeObject *tp = Py_TYPE(op);
+ PyObject_GC_UnTrack(op);
+ (void)Tktt_Clear(op);
+ tp->tp_free(op);


Suggested change
tp->tp_free(op);
Py_TYPE(op)->tp_free(op);

Unfortunately, this temporary variable is needed as we need Py_DECREF(Py_TYPE(op)) otherwise. The rest of the code base also does this.

Modules/_tkinter.c

picnixz · 2025-09-10T12:26:46Z

Oh you mean, removing the asserts. Well, I can do it though tracking segfaults would be a bit more annoying (at least with asserts, we don't have this issue). I'll just shorten the diff then and let it segfault normally.

ZeroIntensity · 2025-09-10T12:29:10Z

Debuggers are typically pretty good at catching null pointer dereferences anyway.

serhiy-storchaka

LGTM.

…-types-116946

picnixz · 2025-09-11T09:14:55Z

(I merged main because of conflicts; for the backports, I will need to do them manually as those types are not immutable on 3.13 and 3.14).

picnixz · 2025-09-11T11:32:53Z

Mmh, something is actually wrong now. Previously, those leaked:

importtkinterw=tkinter.Tk() trace=lambda*_, **__: Nonetrace.evil=type(w.tk) w.tk.settrace(trace) w.mainloop()

For the timer, it's also possible to make something bad as follows:

importtkinterw=tkinter.Tk() func=lambda*_, **__: Nonefunc.evil=type(w.tk.createtimerhandler(1234567, print)) w.tk.createtimerhandler(1234567, func) w.mainloop()

But now I have the following crash for the timer reproducer:

python: ./Modules/_tkinter.c:2784: int Tktt_Traverse(PyObject *, visitproc, void *): Assertion `TkttObject_Check(op)' failed.

The first reproducer is fixed however. I think it's because of the extra reference created by the timer handler that is not properly visited or decrefed. I'll first patch this crash and move to backports afterwards.

…{app,tt}Object` (python#138331)" This reverts commit 283380a.

…t}Object` (#138331)" (#138807) This reverts commit 283380a.

bedevere-bot · 2025-09-11T22:58:15Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 CentOS9 NoGIL Refleaks 3.x (tier-1) has failed when building commit f96f7c9.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1610/builds/2057) and take a look at the build logs.
Check if the failure is related to this commit (f96f7c9) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1610/builds/2057

Failed tests:

test_free_threading

Test leaking resources:

test_free_threading: file descriptors

Summary of the results of the build (if available):

==

Click to see traceback logs

remote: Enumerating objects: 3, done. remote: Counting objects: 50% (1/2) remote: Counting objects: 100% (2/2) remote: Counting objects: 100% (2/2), done. remote: Total 3 (delta 1), reused 1 (delta 1), pack-reused 1 (from 1)  From https://github.com/python/cpython * branch main -> FETCH_HEAD Note: switching to 'f96f7c9f5b5db35e8a22f29b4e20f386cdde64f5'. You are in 'detached HEAD' state. You can look around, make experimental changes and commit them, and you can discard any commits you make in this state without impacting any branches by switching back to a branch. If you want to create a new branch to retain commits you create, you may do so (now or later) by using -c with the switch command. Example: git switch -c <new-branch-name> Or undo this operation with: git switch - Turn off this advice by setting config variable advice.detachedHead to false HEAD is now at f96f7c9f5b5 Revert "gh-116946: fully implement GC protocol for `_tkinter.Tk{app,tt}Object` (#138331)" (#138807) Switched to and reset branch 'main' configure: WARNING: no system libmpdec found; falling back to pure-Python version for the decimal modulemake: *** [Makefile:2486: buildbottest] Error 2

fully implement GC protocol for _tkinter objects
84f832f

picnixz added the skip news label Sep 1, 2025

bedevere-appbot added the awaiting core review label Sep 1, 2025

bedevere-appbot mentioned this pull request Sep 1, 2025
Heap types implemented in C must have Py_TPFLAGS_HAVE_GC and implement tp_traverse #116946
Open

picnixz commented Sep 1, 2025
View reviewed changes

Modules/_tkinter.c Outdated Show resolvedHide resolved

picnixz marked this pull request as draft September 1, 2025 12:08

bedevere-appbot removed the awaiting core review label Sep 1, 2025

picnixz marked this pull request as ready for review September 1, 2025 12:26

bedevere-appbot added the awaiting core review label Sep 1, 2025

picnixz commented Sep 1, 2025
View reviewed changes

Modules/_tkinter.cShow resolvedHide resolved

picnixz marked this pull request as draft September 1, 2025 18:55

bedevere-appbot removed the awaiting core review label Sep 1, 2025

make _tkinter.Tcl_Obj immutable instead of supporting full GC
a9c01ef

picnixz force-pushed the fix/gc/tkinter-heap-types-116946 branch from 5f3fc4b to a9c01efCompare September 2, 2025 09:08

picnixz marked this pull request as ready for review September 3, 2025 09:09

bedevere-appbot added the awaiting core review label Sep 3, 2025

picnixz commented Sep 3, 2025
View reviewed changes

Modules/_tkinter.cShow resolvedHide resolved

serhiy-storchaka reviewed Sep 3, 2025
View reviewed changes

revert changes to _tkinter.Tcl_Obj; they will be made later
dbb343f

picnixz requested a review from serhiy-storchaka September 6, 2025 11:04

picnixz added 3 commits September 6, 2025 13:22

Merge branch 'main' into fix/gc/tkinter-heap-types-116946
9eb9f47

add tp_clear for visited objects
8e80641

use the correct slot...
833a85e

serhiy-storchaka reviewed Sep 6, 2025
View reviewed changes

picnixz marked this pull request as draft September 6, 2025 12:14

bedevere-appbot removed the awaiting core review label Sep 6, 2025

revert the revert
022d71e

This comment was marked as resolved.
Sign in to view

keep the necessary bits
168fe43

picnixz changed the title ~~gh-116946: fully implement GC protocol for _tkinter objects~~gh-116946: fully implement GC protocol for _tkinter.Tk{app,tt}ObjectSep 6, 2025

picnixz marked this pull request as ready for review September 6, 2025 12:23

bedevere-appbot added the awaiting core review label Sep 6, 2025

remove warnings
5e5f698

vstinner approved these changes Sep 8, 2025
View reviewed changes

bedevere-appbot added awaiting merge and removed awaiting core review labels Sep 8, 2025

serhiy-storchaka reviewed Sep 10, 2025
View reviewed changes

reduce the diff a bit
35915b0

serhiy-storchaka approved these changes Sep 11, 2025
View reviewed changes

Merge remote-tracking branch 'upstream/main' into fix/gc/tkinter-heap…
ba9c809
…-types-116946

picnixz merged commit 283380a into python:mainSep 11, 2025
43 checks passed

picnixz deleted the fix/gc/tkinter-heap-types-116946 branch September 11, 2025 10:59

bedevere-appbot removed the awaiting merge label Sep 11, 2025

picnixz mentioned this pull request Sep 11, 2025
Crash with non-consumed Tk timer handler #138791
Closed

serhiy-storchaka mentioned this pull request Sep 11, 2025
gh-138791: fix crash when visiting a non-consumed timer handler #138789
Closed

ZeroIntensity added a commit to ZeroIntensity/cpython that referenced this pull request Sep 11, 2025
Revert "pythongh-116946: fully implement GC protocol for `_tkinter.Tk…
41c00af
…{app,tt}Object` (python#138331)" This reverts commit 283380a.

ZeroIntensity mentioned this pull request Sep 11, 2025
Revert "gh-116946: fully implement GC protocol for _tkinter.Tk{app,tt}Object (#138331)" #138807
Merged

ZeroIntensity added a commit that referenced this pull request Sep 11, 2025
Revert "gh-116946: fully implement GC protocol for `_tkinter.Tk{app,t…
f96f7c9
…t}Object` (#138331)" (#138807) This reverts commit 283380a.

-type= (PyTypeObject*)Tkapp_Type;
-assert(type!=NULL);
-assert(type->tp_alloc!=NULL);
-v= (TkappObject*)type->tp_alloc(type, 0);
+PyTypeObject*type= (PyTypeObject*)Tkapp_Type;
+assert(type!=NULL);
+assert(type->tp_alloc!=NULL);
+TkappObject*v= (TkappObject*)type->tp_alloc(type, 0);

-type= (PyTypeObject*)Tktt_Type;
-assert(type!=NULL);
-assert(type->tp_alloc!=NULL);
-v= (TkttObject*)type->tp_alloc(type, 0);
+v= (TkttObject*)((PyTypeObject*)Tktt_Type)->tp_alloc(type, 0);

Uh oh!

gh-116946: fully implement GC protocol for _tkinter.Tk{app,tt}Object#138331

gh-116946: fully implement GC protocol for _tkinter.Tk{app,tt}Object#138331

Uh oh!

Conversation

picnixz commented Sep 1, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

picnixz commented Sep 6, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchakaSep 6, 2025

Choose a reason for hiding this comment

Uh oh!

picnixzSep 6, 2025• 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!

ZeroIntensitySep 6, 2025• 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!

picnixzSep 6, 2025• 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!

picnixz commented Sep 6, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinnerSep 8, 2025

Choose a reason for hiding this comment

Uh oh!

picnixzSep 8, 2025

Choose a reason for hiding this comment

Uh oh!

vstinnerSep 8, 2025

Choose a reason for hiding this comment

Uh oh!

picnixzSep 10, 2025• 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!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

picnixz commented Sep 10, 2025

Uh oh!

ZeroIntensity commented Sep 10, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Sep 10, 2025

Uh oh!

serhiy-storchakaSep 10, 2025

Choose a reason for hiding this comment

Uh oh!

picnixzSep 11, 2025• 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!

serhiy-storchakaSep 10, 2025

Choose a reason for hiding this comment

Uh oh!

picnixzSep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

picnixz commented Sep 10, 2025

Uh oh!

gh-116946: fully implement GC protocol for `_tkinter.Tk{app,tt}Object`#138331

gh-116946: fully implement GC protocol for `_tkinter.Tk{app,tt}Object`#138331

picnixz commented Sep 1, 2025•
edited
Loading

picnixz commented Sep 6, 2025•
edited
Loading

picnixzSep 6, 2025•
edited
Loading

ZeroIntensitySep 6, 2025•
edited
Loading

picnixzSep 6, 2025•
edited
Loading

picnixz commented Sep 6, 2025•
edited
Loading

picnixzSep 10, 2025•
edited
Loading

ZeroIntensity commented Sep 10, 2025•
edited
Loading

picnixzSep 11, 2025•
edited
Loading

picnixz commented Sep 11, 2025•
edited
Loading

picnixz commented Sep 11, 2025•
edited
Loading