NotificationsYou must be signed in to change notification settings
Fork33.4k
Star69.8k

gh-123504: Fix reference leak in initialization of`_tkinter`#123505

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

vstinner merged 9 commits intopython:mainfromZeroIntensity:tkinter-refleak

Sep 3, 2024

Merged

gh-123504: Fix reference leak in initialization of`_tkinter`#123505

vstinner merged 9 commits intopython:mainfromZeroIntensity:tkinter-refleak

Sep 3, 2024

Conversation

Copy link

Member

ZeroIntensity commentedAug 30, 2024•
edited by bedevere-appbot
Loading

Issue:_tkinter leaks type references on initialization #123504

ZeroIntensity added2 commits

August 30, 2024 08:54

Fix reference leak in initialization of _tkinter

cddd021

Add NEWS entry.

a9eb482

bedevere-appbot mentioned this pull request

Aug 30, 2024

_tkinter leaks type references on initialization#123504

Closed

bedevere-appbot added the awaiting review label

Aug 30, 2024

picnixz requested a review fromserhiy-storchaka

August 30, 2024 13:10

Copy link

Member

picnixz commentedAug 30, 2024

What is the output./python -Xshowrefcount -c "import tkinter" with this PR?

Copy link

MemberAuthor

ZeroIntensity commentedAug 30, 2024

What is the output./python -Xshowrefcount -c "import tkinter" with this PR?

[0 refs, 0 blocks]. Though, running with Valgrind is showing some (non-reference) leaks fromTcl_FindExecutable -- that's a separate issue and out of the scope of this PR, though.

serhiy-storchaka reviewed

Aug 30, 2024

View reviewed changes

Modules/_tkinter.c Outdated


		Tkinter_TclError=PyErr_NewException("_tkinter.TclError",NULL,NULL);
		if (PyModule_AddObjectRef(m,"TclError",Tkinter_TclError)) {
		if (PyModule_AddObject(m,"TclError",Tkinter_TclError)) {

Copy link

Member

Choose a reason for hiding this comment

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

Tkinter_TclError should be a strong reference to_tkinter.TclError. Even if you remove "TclError" from the the module dict, the use ofTkinter_TclError should not crash.

BTW, do not usePyModule_AddObject(), it is broken beyond repair. UsePyModule_AddObjectRef() orPyModule_Add(), what is more convenient.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Ok, where should the strong reference toTkinter_TclError be decref'd? Should we add a module clear function?

Switch toPyModule_AddObjectRef

36c65af

serhiy-storchaka reviewed

Aug 30, 2024

View reviewed changes

Modules/_tkinter.c Outdated

		Py_DECREF(m);
		returnNULL;
		}
		Py_DECREF(Tkapp_Type);

Copy link

Member

Choose a reason for hiding this comment

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

It is the same for all other global references (otherwise why would they be global references?).

Yes, if you want to release these references, you need a module clearing function. This is not a big deal, because usually when you unload the module, you quit the program, so who cares about few dangling references?

Switch to a module clear function.

cc291e4

ZeroIntensity requested a review fromserhiy-storchaka

August 30, 2024 15:46

Add back missing newline.

df9265f

Copy link

MemberAuthor

ZeroIntensity commentedAug 30, 2024

FWIW, it might be worth trying to switch_tkinter over toPEP-489 style initialization in the future.

Fix formatting.

4f1f4e4

Copy link

Member

picnixz commentedAug 30, 2024

FWIW, it might be worth trying to switch _tkinter

I had those same concerns when dealing with curses in#123291 but since the module is a bit of a mess, I just focused on getting the correct DECREF when needed.

picnixz reviewed

Aug 30, 2024

View reviewed changes

Modules/_tkinter.c OutdatedShow resolvedHide resolved

Switch toPy_CLEAR

a77abba

vstinner approved these changes

Sep 3, 2024

View reviewed changes

Copy link

Member

vstinner left a comment

Choose a reason for hiding this comment

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

LGTM, it works as expected.

Without this PR:

$ ./python -Xshowrefcount -c "import tkinter"[130 refs, 78 blocks]

With this PR:

$ ./python -Xshowrefcount -c "import tkinter"[0 refs, 0 blocks]

Misc/NEWS.d/next/Library/2024-08-30-09-01-35.gh-issue-123504.lJ9_BB.rst OutdatedShow resolvedHide resolved

bedevere-appbot added awaiting merge and removed awaiting review labels

Sep 3, 2024

Update Misc/NEWS.d/next/Library/2024-08-30-09-01-35.gh-issue-123504.l…

f35f2a0

…J9_BB.rstCo-authored-by: Victor Stinner <vstinner@python.org>

serhiy-storchaka reviewed

Sep 3, 2024

View reviewed changes

Modules/_tkinter.c OutdatedShow resolvedHide resolved

Use dot notation for module initializer.

fc34562

ZeroIntensity requested a review fromserhiy-storchaka

September 3, 2024 20:10

vstinner approved these changes

Sep 3, 2024

View reviewed changes

Copy link

Member

vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

Modules/_tkinter.c

		PyModuleDef_HEAD_INIT,
		"_tkinter",
		NULL,
		-1,

Copy link

Member

vstinnerSep 3, 2024

Choose a reason for hiding this comment

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

You removed.m_size = -1, but it's ok,m_size=0 also means "no module state".

Copy link

Member

serhiy-storchakaSep 4, 2024

Choose a reason for hiding this comment

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

There is a difference between 0 and -1 related to subinterpreters.

Copy link

Member

vstinnerSep 4, 2024

Choose a reason for hiding this comment

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

What is the difference?

Copy link

Member

serhiy-storchakaSep 4, 2024

Choose a reason for hiding this comment

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

From the documentation:

      Setting ``m_size`` to ``-1`` means that the module does not support      sub-interpreters, because it has global state.

Copy link

Member

vstinnerSep 4, 2024

Choose a reason for hiding this comment

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

Oh I see, I forgot about this difference :-(

vstinnerenabled auto-merge (squash)

September 3, 2024 20:26

Copy link

Member

vstinner commentedSep 3, 2024

I enabled auto-merge. Thanks for the PR updates.

I don't think that such "leak" at Python exit is important enough to justify a backport.

vstinner merged commita8bc036 intopython:main

Sep 3, 2024

bedevere-appbot removed the awaiting merge label

Sep 3, 2024

ZeroIntensity deleted the tkinter-refleak branch

September 3, 2024 23:09

serhiy-storchaka reviewed

Sep 4, 2024

View reviewed changes

Modules/_tkinter.c

		PyModuleDef_HEAD_INIT,
		"_tkinter",
		NULL,
		-1,

Copy link

Member

serhiy-storchakaSep 4, 2024

Choose a reason for hiding this comment

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

There is a difference between 0 and -1 related to subinterpreters.

Modules/_tkinter.c

		NULL
		.m_name="_tkinter",
		.m_methods=moduleMethods,
		.m_clear=module_clear,

Copy link

Member

serhiy-storchakaSep 4, 2024

Choose a reason for hiding this comment

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

If you define "clear", you should always define "travel".

Copy link

Member

vstinnerSep 4, 2024

Choose a reason for hiding this comment

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

Would it make sense to emit a warning or even raise an error at runtime?

For example in Objects/typeobject.c, I added an assertion in _PyType_CheckConsistency():

if (type->tp_flags&Py_TPFLAGS_HAVE_GC) {// bpo-44263: tp_traverse is required if Py_TPFLAGS_HAVE_GC is set.// Note: tp_clear is optional.CHECK(type->tp_traverse!=NULL);    }

Copy link

Member

serhiy-storchakaSep 4, 2024

Choose a reason for hiding this comment

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

Looks like a good idea. For both types and modules.

Labels

None yet

Movatterモバイル変換

Uh oh!

gh-123504: Fix reference leak in initialization of_tkinter#123505

gh-123504: Fix reference leak in initialization of_tkinter#123505

Uh oh!

Conversation

ZeroIntensity commentedAug 30, 2024• edited by bedevere-appbotLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

picnixz commentedAug 30, 2024

Uh oh!

ZeroIntensity commentedAug 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity commentedAug 30, 2024

Uh oh!

picnixz commentedAug 30, 2024

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commentedSep 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gh-123504: Fix reference leak in initialization of`_tkinter`#123505

gh-123504: Fix reference leak in initialization of`_tkinter`#123505

ZeroIntensity commentedAug 30, 2024•
edited by bedevere-appbot
Loading