◐ Shell
reader mode source ↗
Skip to content

bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types#19414

Merged
vstinner merged 1 commit into
python:masterfrom
pablogsal:bpo-40217
Apr 27, 2020
Merged

bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types#19414
vstinner merged 1 commit into
python:masterfrom
pablogsal:bpo-40217

Conversation

@pablogsal

@pablogsal pablogsal commented Apr 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

https://bugs.python.org/issue40217

Types created by PyType_FromSpec own a strong reference to their type, but this was added in Python 3.8. The tp_traverse function needs to call Py_VISIT on the type but all existing traverse functions cannot be updated (especially the ones from existing user functions) so we need to provide a tp_traverse that manually calls Py_VISIT(Py_TYPE(self)) and then call the provided tp_traverse. In this way, user functions do not need to be updated, preserve backwards compatibility.

To do this, we store the user-provided traverse function at the end of the type (we need to allocate space for it) so we can call it from our PyType_FromSpec_tp_traverse wrapper.

@pablogsal pablogsal changed the title bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType… Apr 7, 2020
@pablogsal pablogsal force-pushed the bpo-40217 branch 4 times, most recently from ea96f0b to 88f807c Compare April 7, 2020 20:25
@pablogsal

pablogsal commented Apr 7, 2020

Copy link
Copy Markdown
Member Author

Before this PR:

❯ ./python -m test -R 3:3 test_threading -m test.test_threading.SubinterpThreadingTests.test_threads_join_2
0:00:00 load avg: 2.55 Run tests sequentially                                                  
0:00:00 load avg: 2.55 [1/1] test_threading                                                    
beginning 6 repetitions                                                                        
123456                                                                                         
......                                                                                                                                                                                                                                                                                                                                                                                         test_threading leaked [19, 19, 19] references, sum=57                                                                                                                                                                                                                                                                                                                                          test_threading leaked [12, 12, 12] memory blocks, sum=36                                                                                                                                                                                                                                                                                                                                       test_threading failed                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                         == Tests result: FAILURE ==                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                   1 test failed:                                                                                                                                                                                                                                                                                                                                                                                 
    test_threading                                                                                                                                                                             
                                                                                                                                                                                               
Total duration: 385 ms                                                                         
Tests result: FAILURE

With this PR:

❯ ./python -m test -R 3:3 test_threading -m test.test_threading.SubinterpThreadingTests.test_threads_join_2
0:00:00 load avg: 2.37 Run tests sequentially
0:00:00 load avg: 2.37 [1/1] test_threading
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 367 ms
Tests result: SUCCESS

@pablogsal pablogsal marked this pull request as ready for review April 7, 2020 21:27
@pablogsal pablogsal force-pushed the bpo-40217 branch 4 times, most recently from 353f682 to ab1ca32 Compare April 7, 2020 21:58
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 8, 2020
@bedevere-bot

bedevere-bot commented Apr 8, 2020

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit ab1ca32 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 8, 2020
vstinner
vstinner approved these changes Apr 23, 2020
View reviewed changes

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

Hide comment

LGTM, but I have a few minor remarks.

@vstinner vstinner merged commit 0169d30 into python:master Apr 27, 2020
@vstinner

vstinner commented Apr 27, 2020

Copy link
Copy Markdown
Member

@pablogsal: I added "skip news" to be able to merge your PR, but IMHO it's worth it to add a NEWS entry for it (I suggest the C API category). Would you mind to add one?

@pablogsal

pablogsal commented Apr 27, 2020

Copy link
Copy Markdown
Member Author

Would you mind to add one?

Will do but I didn't had time to add your suggestions for this PR before you merged it. Should I add this in the future PR?

@vstinner

vstinner commented Apr 27, 2020

Copy link
Copy Markdown
Member

Will do but I didn't had time to add your suggestions for this PR before you merged it. Should I add this in the future PR?

Yeah.

@pablogsal pablogsal mentioned this pull request Apr 27, 2020
Merged
pablogsal added a commit to pablogsal/cpython that referenced this pull request May 23, 2020
@pablogsal
Revert "bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called fo… …
3bc77ee 
…r PyType_FromSpec types (pythonGH-19414)"

This reverts commit 0169d30.
miss-islington pushed a commit that referenced this pull request May 27, 2020
@pablogsal
bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType… …
1cf15af 
…_FromSpec types (reverts GH-19414) (GH-20264)

Heap types now always visit the type in tp_traverse. See added docs for details.

This reverts commit 0169d30.

Automerge-Triggered-By: @encukou
miss-islington added a commit that referenced this pull request May 28, 2020
@miss-islington @pablogsal
bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType… …
bcbe5c5 
…_FromSpec types (reverts GH-19414) (GH-20264)

Heap types now always visit the type in tp_traverse. See added docs for details.

This reverts commit 0169d30.

Automerge-Triggered-By: @encukou
(cherry picked from commit 1cf15af)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
@tjstum tjstum mentioned this pull request Feb 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tim-one tim-one tim-one left review comments

@vstinner vstinner vstinner approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

@encukou encukou Awaiting requested review from encukou

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pablogsal @bedevere-bot @vstinner @tim-one @the-knights-who-say-ni