Fix reference leak with types created using PyType_FromSpec

[pitrou]

BPO	15142
Nosy	@loewis, @ncoghlan, @pitrou, @durban

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2012-06-23.12:49:42.400>
created_at = <Date 2012-06-22.19:56:06.629>
labels = ['interpreter-core', 'performance']
title = 'Fix reference leak with types created using PyType_FromSpec'
updated_at = <Date 2012-06-23.12:49:42.399>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2012-06-23.12:49:42.399>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2012-06-23.12:49:42.400>
closer = 'pitrou'
components = ['Interpreter Core']
creation = <Date 2012-06-22.19:56:06.629>
creator = 'pitrou'
dependencies = []
files = []
hgrepos = []
issue_num = 15142
keywords = []
message_count = 9.0
messages = ['163470', '163532', '163541', '163542', '163552', '163563', '163572', '163600', '163601']
nosy_count = 6.0
nosy_names = ['loewis', 'ncoghlan', 'pitrou', 'daniel.urban', 'python-dev', 'Robin.Schreiber']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue15142'
versions = ['Python 3.2', 'Python 3.3']

[pitrou]

The following patch seems to fix the issue explained in http://mail.python.org/pipermail/python-dev/2012-June/120659.html :

diff --git a/Objects/typeobject.c b/Objects/typeobject.c
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -2417,6 +2417,10 @@ PyType_FromSpec(PyType_Spec *spec)
     if (res->ht_type.tp_dictoffset) {
         res->ht_cached_keys = _PyDict_NewKeysForClass();
     }
+    if (res->ht_type.tp_dealloc == NULL) {
+        /* It's a heap type, so needs the heap types' dealloc */
+        res->ht_type.tp_dealloc = subtype_dealloc;
+    }
 
     if (PyType_Ready(&res->ht_type) < 0)
         goto fail;

[ncoghlan]

That does look like it will fix the leak, but now I'm actually thinking there's more code from type_new that should also be executed in the PyType_FromSpec case.

I mean things like:

• ensuring __new__ is a static method
• ensuring the standard attribute lookup machinery is configured
• hooking up tp_as_number, tp_as_mapping, etc
• ensuring GC support is configured correctly

If that's all happening somehow, it could use a comment, because I certainly can't see it.

If not, we probably need to factor out some helper functions that type_new and PyType_FromSpec can both call to make sure everything is fully configured.

[loewis]

• ensuring __new__ is a static method

This shouldn't be necessary. __new__ won't be a method at all,
and not even exist. Instead, a type may or may not fill the tp_new
slot.

• ensuring the standard attribute lookup machinery is configured

This is what PyType_Ready does, no?

• hooking up tp_as_number, tp_as_mapping, etc

This is indeed missing. Robin Schreiber is working on a patch.

• ensuring GC support is configured correctly

This is the responsibility of the caller, as always with C types.

[loewis]

In any case, one issue at a time, please. This issues is about a reference leak.

[ncoghlan]

You're right, I was confusing what happens automatically for classes defined in Python (i.e. the full treatment in type_new) vs those defined statically (i.e. just the parts in PyType_Ready).

Given that PyType_FromSpec doesn't currently support inheritance, providing a default tp_dealloc before the inherit_slots() call in PyType_Ready would work OK in the near term.

However, once inheritance support is added by bpo-15146 then it would be wrong - the default slot entry would override an inherited one.

So, I think this adjustment actually needs to be handled in PyType_Ready, at some point after the inherit_slots() call.

Something like:

/* Sanity check for tp_dealloc. */
if ((type->tp_flags & Py_TPFLAGS_HEAPTYPE) &&
    (type->tp_dealloc == type_dealloc)) {
    /* Type has been declared as a heap type, but has inherited the
       default allocator. This can happen when using the limited API
       to dynamically create types.
     */
    type->tp_dealloc = subtype_dealloc;
}

[pitrou]

However, once inheritance support is added by bpo-15146 then it would be
wrong - the default slot entry would override an inherited one.

It would not be wrong. subtype_dealloc will properly call a base class'
tp_dealloc, if necessary.

[ncoghlan]

True, I didn't follow the bouncing ball far enough. In that, case I think all that is needed is a comment like:

"subtype_dealloc walks the MRO to call the base dealloc function, so it is OK to block inheritance of the slot"

[python-dev]

New changeset 1794308c1ea7 by Antoine Pitrou in branch '3.2':
Issue bpo-15142: Fix reference leak when deallocating instances of types created using PyType_FromSpec().
http://hg.python.org/cpython/rev/1794308c1ea7

New changeset 9945d7dfa72c by Antoine Pitrou in branch 'default':
Issue bpo-15142: Fix reference leak when deallocating instances of types created using PyType_FromSpec().
http://hg.python.org/cpython/rev/9945d7dfa72c

[pitrou]

Ok, fixed, thanks.