A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have made the requested changes; please review again

cc@vstinner@ericsnowcurrently@serhiy-storchaka

Hi Serihy. Thanks for pointing out the bug, I've updated the change with the appropriate fix.

I was able to come up with a generic solution which can solve many of the bugs that have come up from converting a PyType_Ready to a PyType_FromSpec. I used that to solve the specific case that is presented in posixmodule.c

Problem

Most of the issues in converting aPyType_Ready to aPyType_FromSpec come from implicit assumptions about how these types should behave. This results in missing signals which cause incorrect behavior. For example, uninstantiable types becoming instantiable, the runtime crashing, or refcnt leaks. To understand these issues I have broken down the scenarios that might arise:

tp_new is defined and tp_dealloc is defined.

Result: Refcnt Leak or Crash

Instances are most likely instantiated in Python-land and it usesPyType_GenericAlloc to allocate the instance, increfing the type. However, under the assumption that the type is static, it is likely thattp_dealloc is not decrefing the type. This results in a net refcnt of +1 per object instantiation.
Another sub-scenario is whentp_dealloc actually decrefs the type. This results in correct behavior.
However, if the user creates the instance throughPyObject_New{Var} it won't incref its type. This results in a net refcnt of -1 per object instantiation, causing a a segfault.

tp_new is defined and tp_dealloc is undefined.

Result: Correct Behavior

Instances are most likely instantiated in Python-land and it usesPyType_GenericAlloc to allocate the instance, increfing the type.tp_dealloc is undefined and will inheritsubtype_dealloc which will decref the type. This results in correct behavior.
However, if the user creates the instance throughPyObject_New{Var} it won't incref its type. This results in a net refcnt of -1 per object instantiation, causing a a segfault.

tp_new is undefined and tp_dealloc is defined. Result:

Incorrect Behavior

This is an uninstantiable type from Python-land. Therefore, instantiation comes directly fromPyObject_New{Var}, which won't incref the type. Therefore, the defined tp_dealloc won't decref the type either. However, the side effect is that the signal produced bytp_new beingNULL will be lost given that the type will inherit its base object'stp_new. This will now make the type both instantiable and pickable in Python-land.

tp_new is undefined and tp_dealloc is unedefined.

Result: Crash

This is an uninstantiable type from Python-land. Therefore, instantiation comes directly fromPyObject_New{Var}, which won't incref the type. However, the undefinedtp_dealloc will inheritsubtype_dealloc which will decref the type. This results in a net refcnt of -1 per object instantiation, causing a a segfault.

Solution

In order to solve these issues, we need to think of these types as participating in normal refcnting just like any other instance.

The following provides a generic solution for all these different scenarios:

• MakePyObject_INIT incref heap-allocated instance's type.
• Have the customtp_dealloc decref the instance's type
• Explicitly define atp_new,__reduce__, and__reduce_ex__ that raise an exception and returnNULL to make it uninstantiable and unpickable respectively.

Closing Bugs

The generic solution proposed here should be able to address all the issues and problems in the following bpos:

https://bugs.python.org/issue14936
https://bugs.python.org/issue15142
https://bugs.python.org/issue15721
https://bugs.python.org/issue16690
https://bugs.python.org/issue23815
https://bugs.python.org/issue26979

Thanks for making the requested changes!

@ericsnowcurrently,@serhiy-storchaka: please review the changes made to this pull request.

Thanks for the review@serhiy-storchaka ! I answered each of the points that you raised and marked the comments as resolved. Please feel free to raise it again if you still feel that I did not fully address either of your comments.

I have made the requested changes; please review again

Thanks for making the requested changes!

@serhiy-storchaka,@ericsnowcurrently: please review the changes made to this pull request.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes,you will be poked with soft cushions!

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed thePSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username onbugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please followthe steps outlined in the CPython devguide to rectify this issue.

You cancheck yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

Woops, this change got messed up in a rebase