◐ Shell
reader mode source ↗
Skip to content

bpo-42972: _thread.RLock type implements tp_traverse#26734

Merged
vstinner merged 1 commit into
python:mainfrom
vstinner:rlock_traverse
Jun 15, 2021
Merged

bpo-42972: _thread.RLock type implements tp_traverse#26734
vstinner merged 1 commit into
python:mainfrom
vstinner:rlock_traverse

Conversation

@vstinner

@vstinner vstinner commented Jun 15, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

The _thread.RLock type now fully implement the GC protocol: add a
traverse function and the the Py_TPFLAGS_HAVE_GC flag.

https://bugs.python.org/issue42972

@vstinner vstinner added needs backport to 3.10 only security fixes and removed awaiting core review labels Jun 15, 2021
@vstinner vstinner mentioned this pull request Jun 15, 2021
Merged
@vstinner

vstinner commented Jun 15, 2021

Copy link
Copy Markdown
Member Author

Leak discovered by #26727

@vstinner
bpo-42972: _thread.RLock type implements tp_traverse …
f92a784 
The _thread.RLock type now fully implement the GC protocol: add a
traverse function and the Py_TPFLAGS_HAVE_GC flag.
@pitrou

pitrou commented Jun 15, 2021

Copy link
Copy Markdown
Member

Don't we have a warning for when heap types don't implement a tp_traverse?

@pablogsal

pablogsal commented Jun 15, 2021

Copy link
Copy Markdown
Member

Don't we have a warning for when heap types don't implement a tp_traverse?

No, the warning is when a heap type has the GC flags but don't have tp_traverse. in this case we were missing the GC flags so the check failed to raise the warning

@vstinner

vstinner commented Jun 15, 2021

Copy link
Copy Markdown
Member Author

Don't we have a warning for when heap types don't implement a tp_traverse?

I agree that a warning should be emitted at runtime when a heap type doesn't have the Py_TPFLAGS_HAVE_GC or doesn't implement tp_traverse. I suggest to discuss this idea in https://bugs.python.org/issue42972

It's super hard to discover such very tricky bug if you don't have a very deep understanding of GC internals and how Python works.

shihai1991
shihai1991 approved these changes Jun 15, 2021
View reviewed changes

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

Hide comment

LGTM.

@vstinner vstinner merged commit 1cd3d85 into python:main Jun 15, 2021
@miss-islington

miss-islington commented Jun 15, 2021

Copy link
Copy Markdown
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@vstinner vstinner deleted the rlock_traverse branch June 15, 2021 13:09
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 15, 2021
@vstinner @miss-islington
bpo-42972: _thread.RLock implements the GH protocol (pythonGH-26734) …
279caac 
The _thread.RLock type now fully implement the GC protocol: add a
traverse function and the Py_TPFLAGS_HAVE_GC flag.
(cherry picked from commit 1cd3d85)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-bot

bedevere-bot commented Jun 15, 2021

Copy link
Copy Markdown

GH-26735 is a backport of this pull request to the 3.10 branch.

miss-islington added a commit that referenced this pull request Jun 15, 2021
@miss-islington @vstinner
bpo-42972: _thread.RLock implements the GH protocol (GH-26734) …
e30fe27 
The _thread.RLock type now fully implement the GC protocol: add a
traverse function and the Py_TPFLAGS_HAVE_GC flag.
(cherry picked from commit 1cd3d85)

Co-authored-by: Victor Stinner <vstinner@python.org>
@vstinner

vstinner commented Jun 15, 2021

Copy link
Copy Markdown
Member Author

Thanks for your reviews @pablogsal and @shihai1991 ;-)

jdevries3133 pushed a commit to jdevries3133/cpython that referenced this pull request Jun 19, 2021
@vstinner @jdevries3133
bpo-42972: _thread.RLock implements the GH protocol (pythonGH-26734) …
c60a9c6 
The _thread.RLock type now fully implement the GC protocol: add a
traverse function and the Py_TPFLAGS_HAVE_GC flag.
@pablogsal pablogsal mentioned this pull request Jul 18, 2021
Closed
@vstinner vstinner mentioned this pull request Jun 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shihai1991 shihai1991 shihai1991 approved these changes

@pablogsal pablogsal pablogsal approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@vstinner @pitrou @pablogsal @miss-islington @bedevere-bot @shihai1991 @the-knights-who-say-ni