FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-116946: fully implement GC protocol for `_tkinter.Tk{app,tt}Object` by picnixz · Pull Request #138331 · python/cpython · GitHub

/ cpython Public

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

Merged
picnixz merged 11 commits into
python:mainfrom
picnixz:fix/gc/tkinter-heap-types-116946
Sep 11, 2025
Merged

gh-116946: fully implement GC protocol for _tkinter.Tk{app,tt}Object#138331
picnixz merged 11 commits into
python:mainfrom
picnixz:fix/gc/tkinter-heap-types-116946

Conversation

picnixz commented Sep 1, 2025
edited
Loading

Copy link
Copy Markdown
Member
  • 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.

Comment thread Modules/_tkinter.c Outdated
picnixz marked this pull request as draft September 1, 2025 12:08
picnixz marked this pull request as ready for review September 1, 2025 12:26
Comment thread Modules/_tkinter.c
picnixz marked this pull request as draft September 1, 2025 18:55
picnixz force-pushed the fix/gc/tkinter-heap-types-116946 branch from 5f3fc4b to a9c01ef Compare September 2, 2025 09:08
picnixz marked this pull request as ready for review September 3, 2025 09:09
Comment thread Modules/_tkinter.c

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

picnixz commented Sep 6, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

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

Comment thread Modules/_tkinter.c Outdated
Comment on lines +2748 to +2751
type = (PyTypeObject *)Tktt_Type;
assert(type != NULL);
assert(type->tp_alloc != NULL);
v = (TkttObject *)type->tp_alloc(type, 0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

picnixz Sep 6, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ZeroIntensity Sep 6, 2025
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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);
    return type->tp_alloc(type, 0);
}

picnixz Sep 6, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

picnixz commented Sep 6, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

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

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

This comment was marked as resolved.

picnixz changed the title gh-116946: fully implement GC protocol for _tkinter objects gh-116946: fully implement GC protocol for _tkinter.Tk{app,tt}Object Sep 6, 2025
picnixz marked this pull request as ready for review September 6, 2025 12:23

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Comment thread Modules/_tkinter.c Outdated
Comment on lines +596 to +599
type = (PyTypeObject *)Tkapp_Type;
assert(type != NULL);
assert(type->tp_alloc != NULL);
v = (TkappObject *)type->tp_alloc(type, 0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

picnixz Sep 10, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

picnixz commented Sep 10, 2025

Copy link
Copy Markdown
Member Author

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 commented Sep 10, 2025
edited
Loading

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

Comment thread Modules/_tkinter.c Outdated
Comment on lines +2752 to +2755
type = (PyTypeObject *)Tktt_Type;
assert(type != NULL);
assert(type->tp_alloc != NULL);
v = (TkttObject *)type->tp_alloc(type, 0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
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);

picnixz Sep 11, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Modules/_tkinter.c Outdated
Comment thread Modules/_tkinter.c
PyTypeObject *tp = Py_TYPE(op);
PyObject_GC_UnTrack(op);
(void)Tktt_Clear(op);
tp->tp_free(op);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
tp->tp_free(op);
Py_TYPE(op)->tp_free(op);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Modules/_tkinter.c

picnixz commented Sep 10, 2025

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

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

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

picnixz commented Sep 11, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

(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 merged commit 283380a into python:main Sep 11, 2025
43 checks passed
picnixz deleted the fix/gc/tkinter-heap-types-116946 branch September 11, 2025 10:59

picnixz commented Sep 11, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

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

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

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

import tkinter
w = tkinter.Tk()
func = lambda *_, **__: None
func.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.

Copy link
Copy Markdown

⚠️⚠️⚠️ 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:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. 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.
  4. Check if the failure is related to this commit (f96f7c9) or if it is a false positive.
  5. 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 module

make: *** [Makefile:2486: buildbottest] Error 2

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL