Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-37879: Suppress subtype_dealloc decref when base type is a C heap type #15323

Merged
merged 11 commits into from Sep 11, 2019

Conversation

@eduardo-elizondo
Copy link
Contributor

commented Aug 17, 2019

The instance destructor for a type is responsible for preparing
an instance for deallocation by decrementing the reference counts
of its referents.

If an instance belongs to a heap type, the type object of an instance
has its reference count decremented while for static types, which
are permanently allocated, the type object is unaffected by the
instance destructor.

Previously, the default instance destructor searched the class
hierarchy for an inherited instance destructor and, if present,
would invoke it.

Then, if the instance type is a heap type, it would decrement the
reference count of that heap type. However, this could result in the
premature destruction of a type because the inherited instance
destructor should have already decremented the reference count
of the type object.

This change avoids the premature destruction of the type object
by suppressing the decrement of its reference count when an
inherited, non-default instance destructor has been invoked.

Finally, an assertion on the Py_SIZE of a type was deleted. Heap
types have a non zero size, making this into an incorrect assertion.

https://bugs.python.org/issue37879

Suppress subtype_dealloc decref when base type is a C heap type
The instance destructor for a type is responsible for preparing
an instance for deallocation by decrementing the reference counts
of its referents.

If an instance belongs to a heap type, the type object of an instance
has its reference count decremented while for static types, which
are permanently allocated, the type object is unaffected by the
instance destructor.

Previously, the default instance destructor searched the class
hierarchy for an inherited instance destructor and, if present,
would invoke it.

Then, if the instance type is a heap type, it would decrement the
reference count of that heap type.  However, this could result in the
premature destruction of a type because the inherited instance
destructor should have already decremented the reference count
of the type object.

This change avoids the premature destruction of the type object
by suppressing the decrement of its reference count when an
inherited, non-default instance destructor has been invoked.

Finally, an assertion on the Py_SIZE of a type was deleted.  Heap
types have a non zero size, making this into an incorrect assertion.
@jdemeyer
Copy link
Contributor

left a comment

Apart from the one comment, the changes to typeobject.c look good. I haven't yet looked at the testcases.

Objects/typeobject.c Show resolved Hide resolved
PyTypeObject *base = (PyTypeObject *)PyType_GetSlot(ctypesubclass, Py_tp_base);
Py_DECREF(ctypesubclass);
initproc base_init = PyType_GetSlot(base, Py_tp_init);
if (base_init(self, args, kwargs) < 0) {

This comment has been minimized.

Copy link
@encukou

encukou Sep 9, 2019

Member

Is this setup to get the superclass necessary?
It should be possible to call heapctype_init(self, args, kwargs) directly.

This comment has been minimized.

Copy link
@eduardo-elizondo

eduardo-elizondo Sep 10, 2019

Author Contributor

It is indeed. I actually had your suggestion initially but ultimately ended up changing it to this approach. My rationale was: To show that HeapCTypeSubclass is indeed a C extension subclass and that you can call the tp_init slot by pulling its tp_base out. To me, it seemed closer to what a __init__ would do by calling super().__init__.

That being said, I don't have any strong opinions on either. I'll keep this as is but feel free to add one more comment to change to to call heapctype_init and I will change it this time!

This comment has been minimized.

Copy link
@encukou

encukou Sep 10, 2019

Member

There are some subtle issues in doing dynamic super()-like calls from C code, but using PyState_FindModule/PyObject_GetAttrString is especially fragile.
If the elaborate code manages to set base_init to anything except heapctype_init, it's a bug. It's not the bug this is supposed to be testing; and it's a bug that's likely to segfault.

I'm actually trying to provide a better way to do this without PyState_FindModule (PEP 573), but so far, using heapctype_init is, IMO, best.

Modules/_testcapimodule.c Outdated Show resolved Hide resolved
@eduardo-elizondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@encukou Comments have been addressed. Also, hope the core sprint is going well (if you are there) :)

encukou added 4 commits Sep 9, 2019
Move the added test code
- Move CAPITest additions to the end (they're testing a fairly
  exotic edge case, so I wouldn't make them prominent).
- Have the PyModuleDef for _testcapi close to the PyInit function again
  (this needs a forward declaration, but keeps the module init code
  logically together).
Add docstrings to C classes
This is mainly to make the intention clearer to readers of the code:
the docstrings serve as headings.
Remove unnecessary descriptor
The same descriptor is in the superclass.
@encukou

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Yup, the sprint is going well!

I have added some commits directly to this branch, rather than tell you about the nitpicks I have and let you write the code.
Could you take a look at them? Feel free to remove/change any of them (or tell me to do so) if that doesn't work for you.

@eduardo-elizondo
Copy link
Contributor Author

left a comment

LGTM

@eduardo-elizondo
Copy link
Contributor Author

left a comment

Both changes and the reasoning looks good to me

@eduardo-elizondo
Copy link
Contributor Author

left a comment

Nice catch!

@eduardo-elizondo
Copy link
Contributor Author

left a comment

Looks good

@eduardo-elizondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@encukou I went through all of you changes and they all look good! I also rebased this to master to get rid of the merge conflicts so this change should be good to go!

Given that we are in the topic of heap allocated types, I've got posixmodule to use heap allocated types only in #15892 in case you'd like to take a look 😄

@encukou encukou merged commit ff023ed into python:master Sep 11, 2019

5 checks passed

Azure Pipelines PR #20190911.7 succeeded
Details
bedevere/issue-number Issue number 37879 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Copy link

commented Sep 11, 2019

Thanks @eduardo-elizondo for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒🤖

@miss-islington

This comment has been minimized.

Copy link

commented Sep 11, 2019

Sorry, @eduardo-elizondo and @encukou, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker ff023ed36ea260ab64be5895f1f1f087c798987a 3.8

encukou added a commit that referenced this pull request Sep 11, 2019
[3.8] bpo-37879: Suppress subtype_dealloc decref when base type is a …
…C heap type (GH-15323)

The instance destructor for a type is responsible for preparing
an instance for deallocation by decrementing the reference counts
of its referents.

If an instance belongs to a heap type, the type object of an instance
has its reference count decremented while for static types, which
are permanently allocated, the type object is unaffected by the
instance destructor.

Previously, the default instance destructor searched the class
hierarchy for an inherited instance destructor and, if present,
would invoke it.

Then, if the instance type is a heap type, it would decrement the
reference count of that heap type.  However, this could result in the
premature destruction of a type because the inherited instance
destructor should have already decremented the reference count
of the type object.

This change avoids the premature destruction of the type object
by suppressing the decrement of its reference count when an
inherited, non-default instance destructor has been invoked.

Finally, an assertion on the Py_SIZE of a type was deleted.  Heap
types have a non zero size, making this into an incorrect assertion.

#15323.
(cherry picked from commit ff023ed)

Co-authored-by: Eddie Elizondo <eduardo.elizondorueda@gmail.com>
@bedevere-bot

This comment has been minimized.

Copy link

commented Sep 11, 2019

GH-15966 is a backport of this pull request to the 3.8 branch.

encukou added a commit that referenced this pull request Sep 12, 2019
[3.8] bpo-37879: Suppress subtype_dealloc decref when base type is a …
…C heap type (GH-15323, GH-16004) (GH-15966)

The instance destructor for a type is responsible for preparing
an instance for deallocation by decrementing the reference counts
of its referents.

If an instance belongs to a heap type, the type object of an instance
has its reference count decremented while for static types, which
are permanently allocated, the type object is unaffected by the
instance destructor.

Previously, the default instance destructor searched the class
hierarchy for an inherited instance destructor and, if present,
would invoke it.

Then, if the instance type is a heap type, it would decrement the
reference count of that heap type.  However, this could result in the
premature destruction of a type because the inherited instance
destructor should have already decremented the reference count
of the type object.

This change avoids the premature destruction of the type object
by suppressing the decrement of its reference count when an
inherited, non-default instance destructor has been invoked.

Finally, an assertion on the Py_SIZE of a type was deleted.  Heap
types have a non zero size, making this into an incorrect assertion.

#15323.
(cherry picked from commit ff023ed)
Fixup: #16004.
(cherry picked from commit 5e9caee)

Co-authored-by: Eddie Elizondo <eduardo.elizondorueda@gmail.com>
vrpolakatcisco added a commit to vrpolakatcisco/cpython that referenced this pull request Sep 12, 2019
bpo-37879: Suppress subtype_dealloc decref when base type is a C heap…
… type (pythonGH-15323)

The instance destructor for a type is responsible for preparing
an instance for deallocation by decrementing the reference counts
of its referents.

If an instance belongs to a heap type, the type object of an instance
has its reference count decremented while for static types, which
are permanently allocated, the type object is unaffected by the
instance destructor.

Previously, the default instance destructor searched the class
hierarchy for an inherited instance destructor and, if present,
would invoke it.

Then, if the instance type is a heap type, it would decrement the
reference count of that heap type.  However, this could result in the
premature destruction of a type because the inherited instance
destructor should have already decremented the reference count
of the type object.

This change avoids the premature destruction of the type object
by suppressing the decrement of its reference count when an
inherited, non-default instance destructor has been invoked.

Finally, an assertion on the Py_SIZE of a type was deleted.  Heap
types have a non zero size, making this into an incorrect assertion.

python#15323
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.