homepage

This issue tracker has been migrated to GitHub , and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Fix reference leak with types created using PyType_FromSpec
Type: resource usage Stage: resolved
Components: Interpreter Core Versions: Python 3.2, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Robin.Schreiber, daniel.urban, loewis, ncoghlan, pitrou, python-dev
Priority: normal Keywords:

Created on 2012年06月22日 19:56 by pitrou, last changed 2022年04月11日 14:57 by admin. This issue is now closed.

Messages (9)
msg163470 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012年06月22日 19:56
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;
msg163532 - (view) Author: Alyssa Coghlan (ncoghlan) * (Python committer) Date: 2012年06月23日 05:59
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.
msg163541 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012年06月23日 07:32
> - 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.
msg163542 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012年06月23日 07:33
In any case, one issue at a time, please. This issues is about a reference leak.
msg163552 - (view) Author: Alyssa Coghlan (ncoghlan) * (Python committer) Date: 2012年06月23日 08:42
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 #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;
 }
msg163563 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012年06月23日 09:40
> However, once inheritance support is added by #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.
msg163572 - (view) Author: Alyssa Coghlan (ncoghlan) * (Python committer) Date: 2012年06月23日 10:15
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"
msg163600 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012年06月23日 12:48
New changeset 1794308c1ea7 by Antoine Pitrou in branch '3.2':
Issue #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 #15142: Fix reference leak when deallocating instances of types created using PyType_FromSpec().
http://hg.python.org/cpython/rev/9945d7dfa72c 
msg163601 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012年06月23日 12:49
Ok, fixed, thanks.
History
Date User Action Args
2022年04月11日 14:57:31adminsetgithub: 59347
2012年06月23日 12:49:42pitrousetstatus: open -> closed
resolution: fixed
messages: + msg163601

stage: patch review -> resolved
2012年06月23日 12:48:55python-devsetnosy: + python-dev
messages: + msg163600
2012年06月23日 10:15:37ncoghlansetmessages: + msg163572
2012年06月23日 09:40:12pitrousetmessages: + msg163563
2012年06月23日 08:42:36ncoghlansetmessages: + msg163552
2012年06月23日 07:33:56loewissetmessages: + msg163542
2012年06月23日 07:33:04loewissetnosy: + Robin.Schreiber
2012年06月23日 07:32:34loewissetmessages: + msg163541
2012年06月23日 06:01:49ncoghlansetnosy: + daniel.urban
2012年06月23日 05:59:54ncoghlansetnosy: + ncoghlan
messages: + msg163532
2012年06月22日 19:56:06pitroucreate

AltStyle によって変換されたページ (->オリジナル) /