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

pablogsal · 2020-04-07T19:16:30Z

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 · 2020-04-07T20:26:42Z

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

Objects/typeobject.c

bedevere-bot · 2020-04-08T00:20:48Z

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

…_FromSpec types

vstinner

LGTM, but I have a few minor remarks.

vstinner · 2020-04-23T14:48:37Z

Objects/typeobject.c

+    if (obj == NULL)
+        return PyErr_NoMemory();
+
+    obj = obj;


what is the purpose of that?

vstinner · 2020-04-23T14:48:45Z

Objects/typeobject.c

+    /* note that we need to add one, for the sentinel and space for the
+       provided tp-traverse: See bpo-40217 for more details */
+
+    if (PyType_IS_GC(type))


PEP 7: please add { ... }. Same remark for other if.

vstinner · 2020-04-23T14:51:24Z

Objects/typeobject.c

        }
+        else if (slot->slot == Py_tp_traverse) {
+
+           /* Types created by PyType_FromSpec own a strong reference to their


Please put bpo-40217 somewhere in the comment.

vstinner · 2020-04-27T12:22:59Z

@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 · 2020-04-27T12:30:46Z

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 · 2020-04-27T12:34:19Z

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.

…r PyType_FromSpec types (pythonGH-19414)" This reverts commit 0169d30.

@encukou

…_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

@encukou

…_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>

the-knights-who-say-ni added the CLA signed label Apr 7, 2020

bedevere-bot added the awaiting core review label Apr 7, 2020

pablogsal changed the title ~~bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType…~~ bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types Apr 7, 2020

pablogsal force-pushed the bpo-40217 branch 4 times, most recently from ea96f0b to 88f807c Compare April 7, 2020 20:25

tim-one reviewed Apr 7, 2020

View reviewed changes

Objects/typeobject.c Outdated Show resolved Hide resolved

pablogsal force-pushed the bpo-40217 branch from 0b95e68 to f9a9508 Compare April 7, 2020 21:26

pablogsal marked this pull request as ready for review April 7, 2020 21:27

pablogsal requested review from encukou, serhiy-storchaka and vstinner April 7, 2020 21:28

pablogsal force-pushed the bpo-40217 branch 4 times, most recently from 353f682 to ab1ca32 Compare April 7, 2020 21:58

pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 8, 2020

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 8, 2020

bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType…

47ffc90

…_FromSpec types

pablogsal force-pushed the bpo-40217 branch from ab1ca32 to 47ffc90 Compare April 10, 2020 23:32

vstinner approved these changes Apr 23, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Apr 23, 2020

vstinner added the skip news label Apr 27, 2020

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

bedevere-bot removed the awaiting merge label Apr 27, 2020

pablogsal mentioned this pull request Apr 27, 2020

bpo-40217: Clean code in PyType_FromSpec_Alloc and add NEWS entry #19733

Merged

pablogsal added a commit to pablogsal/cpython that referenced this pull request May 23, 2020

Revert "bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called fo…

3bc77ee

…r PyType_FromSpec types (pythonGH-19414)" This reverts commit 0169d30.

tjstum mentioned this pull request Feb 2, 2023

Discussion - All nanobind types should have a tp_traverse? wjakob/nanobind#121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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