Tested this on the gevent (https://github.com/gevent/gevent) third-party dep. This change is backwards-compatible.

Repro steps:

git clone --single-branch -b heapstructseq-posix https://github.com/eduardo-elizondo/cpythoncd cpython./configure --prefix=$HOME/.localmake -jmake installgit clone https://github.com/pypa/virtualenvcd virtualenv~/.local/bin/python3.8 setup.py installgit clone https://github.com/gevent/geventcd geventvirtualenv env -p ~/.local/bin/python3.8source env/bin/activatepip install -r dev-requirements.txtcd src/greentestpython ./testrunner.py

@eduardo-elizondo Thanks for the PR. Is this PR still a WIP? If it is not, please edit the title. Thanks!

@willingc Hi Carol, sorry yes this is ready for review - totally forgot to update the title.

Looping in@encukou to look at this changes as well! Feel free to leave any feedback

Hello Eddie,
Thank for you for the patch. First, I think you should separate the changes to structseq.c from posixmodule.c. I.e. use multiple PRs. It makes the changes easier to review. That's assuming that it is not required to change both files at the same time. It doesn't look like it is because your change is backwards compatible. Making smaller more focused PRs is good, IMHO.

I will be adding some inline review comments to this PR. Your approach looks good to me. I.e. fix PyStructSequence_NewType() and then use it so that the types are heap allocated. In your commit message, NEWS entry or maybe in some code comments, I would try to incorporate the information in your post to python-dev, i.e.https://mail.python.org/pipermail/python-dev/2018-September/155069.html. That is good analysis and should get saved somewhere. Probably the commit message and NEWS would be most appropriate.

It looks like there are some issues with memory leaks and ref counting. See inline comments.

The BPO bug has multiple PR linked. I think the older one is obsolete by this one. You should close it in that case.

@nascheme

I think you should separate the changes to structseq.c from posixmodule.c. I.e. use multiple PRs. It makes the changes easier to review.
[...]

The BPO bug has multiple PR linked. I think the older one is obsolete by this one. You should close it in that case.

The older PR has the structseq.c changes; this one has the posixmodule.c changes. But one depends on the other, so this PR needs to include the structseq.c changes as well.

To me, it does make sense to review the new implementation together with an example of using it.

@encukou@nascheme

I finally updated the change to address all of your comments! I managed to fully clean up all the memory allocations without the need of an extra flag. Here's the rundown of the issues and the fixes:

Leak of PyMemberDef:

StructSequences have to dynamically create thePyMemberDefs that will be passed on to the type's tp_members.PyStructSequence_InitType2 just callsPyMem_New and never cleans up that memory. In my original implementation ofPyStructSequence_NewType, I was just replicating the same behavior.

This can be improved by looking at how new types get allocated. If we look into the implementation oftype_new, we'll find thattp_members is set by:PyHeapType_GET_MEMBERS. This macro basically looks past the size of thePyHeapTypeObject to pull thePyMemberDefs out. Looking back, we can see thattype_new allocatesPyHeapTypeObjects by calling(PyTypeObject *)metatype->tp_alloc(metatype, nslots);. This means that it counts the number ofPyMemberDefs that the type will need and uses that to allocate extra space past the size of the object. That's howPyHeapType_GET_MEMBERS is able to pull these members out. Finally, that extra space of memory is cleaned up with the whole object, meaning that it doesn't need any extra deallocation step.

Going back to StructSequences, we can easily reuse all of that machinery to clean up the dynamically allocatedPyMemberDefs. First, we start by modifyingPyType_FromSpecWithBases so that it allocates all the extra space it needs by counting the number of slots that the object will have and passing that toPyType_GenericAlloc. Then, when all the slots are being iterated over, we add a special case forPy_tp_members, just like there is one forPy_tp_doc. Using this, we should be able to copy the dynamically allocatedPyMemberDefs into the newly added space of memory at the end of thePyHeapTypeObject. This means that now the type owns it'sPyMemberDefs and has no need for the dynamically created ones. Finally, we add aPyMem_FREE after callingPyType_FromSpecWithBases withinPyStructSequence_NewType to deallocate the dynamically createdPyMemberDefs.

Leak of PyType_Slots:
It turns out that all the slots (moduloPy_tp_doc andPy_tp_members) are static pointers which only get copied over to the appropriate offset in thePyHeapTypeObject. ForPy_tp_doc, a new copy is made (as it points to a static literal), andPy_tp_members was addressed above. This means thatPyType_Slots is not really needed after runningPyType_FromSpecWithBases. Therefore, I moved this into a local variable which just gets cleaned up once it gets out of scope.

Leak of PyType_Spec:
Following the same approach, if we follow each of the uses ofPyType_Spec withinPyType_FromSpecWithBases we'll find that all the integer values (basicsize,itemsize, andflags) get copied over.slots, as we've already identified, can be easily deleted. Finally,name ismostly being used as a read-only variable during the execution ofPyType_FromSpecWithBases (i.e. for error messages). There are only two places where it requires to persist the string. The first, when setting thePyHeapTypeObject'sht_name but that already creates aPyUnicode so it's fine. The second, when setting the type'stp_name. In this case, this only copies the pointer and it works under the condition that the originalPyStructSequence_Descr remains alive during the lifetime of thePyHeapTypeObject. Given that all of ourPyStructSequence_Descr are static, this should just work. Otherwise, if a use case arrises for a non-staticPyStructSequence_Descr, we can just create a copy of the string for the type'stp_name - just likePy_tp_doc. Therefore, I moved this PyType_Spec into a local variable which just gets cleaned up once it gets out of scope.

Testing:
To prove that this doesn't actually leak, I followed Neil's advice and added a new test_capi test. This tracks the refcounts and the allocation blocks to prove that this implementation does not leak.

I like the approach!

There's a nice way to resolve the current confusion about what fromPyType_Spec is expected to be static and what can be malloc'd temporarily: do a deep copy, which works for both!
But we're not doing a full deep copy. Member defs fromPy_tp_members point to names and docstrings, and we still expect these are statically allocated (or at least that they will outlive the class). I don't think it's a problem – dynamic use cases are probably better served by__dict__. It would be good to document the expectation, to avoid confusion in the future.

Addressed all of@encukou comments:

• Added comments about the expectation of what will outlive what in PyStructSequence_NewType.
• Throw an error in PyStructSequence_NewType2 when calling with a PyTypeObject with a non-zero reference count.
• Renamed variables and updated all the if and for code blocks to now use braces.
• Removed the News link to the mailing list post in favor of having it in the bug itself.

This PR was never backported to Python 3.7 and below? I cannot use PyStructSequence_NewType in Python 3.7 and earlier, even latest patch release.

Is this only going to be fixed for Python 3.8 and forwards? My extension works in 3.8+

Correct, looks like this never got backported. 3.6 does not accept bug fixes anymore and 3.7 will only have one last bugfix release before it goes into security fixes only. Seems to me like it's not worth backporting this at this point since you wouldn't be able to use PyStructSequence_NewType in 3.7.0 - 3.7.7 anyway.

Alright, I can live with that. Good to know, I'll put a 3.8+ note on my project. Thanks.