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

Fix reference leak with types created using PyType_FromSpec #59347

Closed
pitrou opened this issue Jun 22, 2012 · 9 comments
Closed

Fix reference leak with types created using PyType_FromSpec #59347

pitrou opened this issue Jun 22, 2012 · 9 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@pitrou
Copy link
Member

pitrou commented Jun 22, 2012

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
Copy link
Member Author

pitrou commented Jun 22, 2012

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;

@pitrou pitrou added interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Jun 22, 2012
@ncoghlan
Copy link
Contributor

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
Copy link
Mannequin

loewis mannequin commented Jun 23, 2012

  • 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
Copy link
Mannequin

loewis mannequin commented Jun 23, 2012

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

@ncoghlan
Copy link
Contributor

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
Copy link
Member Author

pitrou commented Jun 23, 2012

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
Copy link
Contributor

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
Copy link
Mannequin

python-dev mannequin commented Jun 23, 2012

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
Copy link
Member Author

pitrou commented Jun 23, 2012

Ok, fixed, thanks.

@pitrou pitrou closed this as completed Jun 23, 2012
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
Projects
None yet
Development

No branches or pull requests

2 participants