Avoiding reference leaks in heap types with custom tp_dealloc
Hello, The new test_importlib.extension.test_loader is currently leaking references, (issue24268). There is a simple hack to stop this, but I'm inclined to not apply quick hacks and rather dig into the root cause. (It's a test module, the refleaks are relatively harmless.) The tests are based directly on the "xxlimited" example, xxlimited.Xxo, which exhibits the same bug -- it's just not tested. It's is caused by a combination of a few factors, but I'm not sure what's a bug and what's just undocumented behavior, so I'm asking for input to put me on the right track. As reported in issue16690, heap types with a naïve custom tp_dealloc leak references to the type when instantiated. According to [0], it seems that tp_dealloc should check if it has been overridden, and if so, decref the type. This needs to be documented (regardless of the solution to the other problems), and I intend to document it. We can change xxlimited to do the check and decref, but isn't it too ugly for a module that showcases the extension module API? (xxlimited.Xxo can technically skip the check, since it doesn't allow subclasses, but that would be setting a nasty trap for anyone learning from that example.) The nice way out would be taking advantage of PEP 442: xxlimited.Xxo can ditch tp_dealloc in favor of tp_traverse and tp_finalize (the former of which it needs anyway to behave correctly). Unfortunately, tp_finalize is not available in the stable ABI (issue24345). I think it should be added; is it too late for 3.5? Another problem is that xxlimited is untested. It's only built for non-debug builds, because Py_LIMITED_API and Py_DEBUG are incompatible. Would it make sense to build and test it without Py_LIMITED_API in debug mode, instead of not building it at all? [0] http://bugs.python.org/issue15653#msg168449
On Mon, 1 Jun 2015 16:38:35 +0200 Petr Viktorin <encukou@gmail.com> wrote:
Hello, The new test_importlib.extension.test_loader is currently leaking references, (issue24268). There is a simple hack to stop this, but I'm inclined to not apply quick hacks and rather dig into the root cause. (It's a test module, the refleaks are relatively harmless.)
The tests are based directly on the "xxlimited" example, xxlimited.Xxo, which exhibits the same bug -- it's just not tested. It's is caused by a combination of a few factors, but I'm not sure what's a bug and what's just undocumented behavior, so I'm asking for input to put me on the right track.
Yes, the issue is really nasty. There are several situations to take into account: - derived heap type inherits from base heap type, both have custom deallocs - derived heap type inherits from base heap type, only one has a custom dealloc - derived heap type inherits from Python-defined class (the latter having subtype_dealloc) - derived heap type inherits from static type - ... It is unreasonable to expect developers of C extensions come up with the correct incantation (and ideally, they shouldn't have to think about it at all).
The nice way out would be taking advantage of PEP 442: xxlimited.Xxo can ditch tp_dealloc in favor of tp_traverse and tp_finalize (the former of which it needs anyway to behave correctly). Unfortunately, tp_finalize is not available in the stable ABI (issue24345). I think it should be added; is it too late for 3.5?
Well, but.... the stable ABI is supposed to be a subset of the API that's safe to program against, regardless of the Python version (at least from the point where the stable ABI was introduced). What happens if you define a Py_tp_finalize and run your C extension type on a pre-3.5 version? Do you get an error at definition time? A resource leak? A crash? I don't get why Benjamin committed the change so quick. Regards Antoine.
On Mon, Jun 1, 2015 at 5:33 PM, Antoine Pitrou <solipsis@pitrou.net> wrote:
On Mon, 1 Jun 2015 16:38:35 +0200 Petr Viktorin <encukou@gmail.com> wrote: [...]
The nice way out would be taking advantage of PEP 442: xxlimited.Xxo can ditch tp_dealloc in favor of tp_traverse and tp_finalize (the former of which it needs anyway to behave correctly). Unfortunately, tp_finalize is not available in the stable ABI (issue24345). I think it should be added; is it too late for 3.5?
Well, but.... the stable ABI is supposed to be a subset of the API that's safe to program against, regardless of the Python version (at least from the point where the stable ABI was introduced).
That part's not true. From the PEP: During evolution of Python, new ABI functions will be added. Applications using them will then have a requirement on a minimum version of Python; this PEP provides no mechanism for such applications to fall back when the Python library is too old.
What happens if you define a Py_tp_finalize and run your C extension type on a pre-3.5 version? Do you get an error at definition time? A resource leak? A crash?
Looking at PyType_FromSpecWithBases code, you should get RuntimeError("invalid slot offset") in this case.
On Mon, 1 Jun 2015 17:52:13 +0200 Petr Viktorin <encukou@gmail.com> wrote:
On Mon, Jun 1, 2015 at 5:33 PM, Antoine Pitrou <solipsis@pitrou.net> wrote:
On Mon, 1 Jun 2015 16:38:35 +0200 Petr Viktorin <encukou@gmail.com> wrote: [...]
The nice way out would be taking advantage of PEP 442: xxlimited.Xxo can ditch tp_dealloc in favor of tp_traverse and tp_finalize (the former of which it needs anyway to behave correctly). Unfortunately, tp_finalize is not available in the stable ABI (issue24345). I think it should be added; is it too late for 3.5?
Well, but.... the stable ABI is supposed to be a subset of the API that's safe to program against, regardless of the Python version (at least from the point where the stable ABI was introduced).
That part's not true. From the PEP: During evolution of Python, new ABI functions will be added. Applications using them will then have a requirement on a minimum version of Python; this PEP provides no mechanism for such applications to fall back when the Python library is too old.
I think we have been laxist with additions to the stable ABI: apparently, they should be conditioned on the API version requested by the user. For example, in pystate.h: #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x03030000 /* New in 3.3 */ PyAPI_FUNC(int) PyState_AddModule(PyObject*, struct PyModuleDef*); PyAPI_FUNC(int) PyState_RemoveModule(struct PyModuleDef*); #endif (those were added by Martin, so I assume he knew what he was doing :-)) This way, failing to restrict yourself to a given API version fails at compile time, not at runtime. However, it's also more work to do so when adding stuff, which is why we tend to skimp on it. Regards Antoine.
On Mon, Jun 1, 2015 at 6:00 PM, Antoine Pitrou <solipsis@pitrou.net> wrote: [...]
I think we have been laxist with additions to the stable ABI: apparently, they should be conditioned on the API version requested by the user. For example, in pystate.h:
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x03030000 /* New in 3.3 */ PyAPI_FUNC(int) PyState_AddModule(PyObject*, struct PyModuleDef*); PyAPI_FUNC(int) PyState_RemoveModule(struct PyModuleDef*); #endif
(those were added by Martin, so I assume he knew what he was doing :-))
This way, failing to restrict yourself to a given API version fails at compile time, not at runtime. However, it's also more work to do so when adding stuff, which is why we tend to skimp on it.
I see! I completely missed that memo. I filed a patch that wraps my 3.5 additions as issue 24365. I think this should be in the PEP, so people like me can find it. Does the attached wording look good?
On Tue, 2 Jun 2015 21:09:52 +0200 Petr Viktorin <encukou@gmail.com> wrote:
On Mon, Jun 1, 2015 at 6:00 PM, Antoine Pitrou <solipsis@pitrou.net> wrote: [...]
I think we have been laxist with additions to the stable ABI: apparently, they should be conditioned on the API version requested by the user. For example, in pystate.h:
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x03030000 /* New in 3.3 */ PyAPI_FUNC(int) PyState_AddModule(PyObject*, struct PyModuleDef*); PyAPI_FUNC(int) PyState_RemoveModule(struct PyModuleDef*); #endif
(those were added by Martin, so I assume he knew what he was doing :-))
This way, failing to restrict yourself to a given API version fails at compile time, not at runtime. However, it's also more work to do so when adding stuff, which is why we tend to skimp on it.
I see! I completely missed that memo. I filed a patch that wraps my 3.5 additions as issue 24365.
I think this should be in the PEP, so people like me can find it. Does the attached wording look good?
It looks good to me, but Martin should probably validate it. If he hasn't answered yet in one month, perhaps you can add it anyway :) Regards Antoine.
On Mon, Jun 1, 2015, at 11:33, Antoine Pitrou wrote:
On Mon, 1 Jun 2015 16:38:35 +0200 Petr Viktorin <encukou@gmail.com> wrote:
Hello, The new test_importlib.extension.test_loader is currently leaking references, (issue24268). There is a simple hack to stop this, but I'm inclined to not apply quick hacks and rather dig into the root cause. (It's a test module, the refleaks are relatively harmless.)
The tests are based directly on the "xxlimited" example, xxlimited.Xxo, which exhibits the same bug -- it's just not tested. It's is caused by a combination of a few factors, but I'm not sure what's a bug and what's just undocumented behavior, so I'm asking for input to put me on the right track.
Yes, the issue is really nasty. There are several situations to take into account: - derived heap type inherits from base heap type, both have custom deallocs - derived heap type inherits from base heap type, only one has a custom dealloc - derived heap type inherits from Python-defined class (the latter having subtype_dealloc) - derived heap type inherits from static type - ...
It is unreasonable to expect developers of C extensions come up with the correct incantation (and ideally, they shouldn't have to think about it at all).
The nice way out would be taking advantage of PEP 442: xxlimited.Xxo can ditch tp_dealloc in favor of tp_traverse and tp_finalize (the former of which it needs anyway to behave correctly). Unfortunately, tp_finalize is not available in the stable ABI (issue24345). I think it should be added; is it too late for 3.5?
Well, but.... the stable ABI is supposed to be a subset of the API that's safe to program against, regardless of the Python version (at least from the point where the stable ABI was introduced). What happens if you define a Py_tp_finalize and run your C extension type on a pre-3.5 version? Do you get an error at definition time? A resource leak? A crash?
I don't get why Benjamin committed the change so quick.
I thought all the slots were supposed to be available through the stable ABI, since you need them to properly define c types.
On Mon, 01 Jun 2015 12:09:37 -0400 Benjamin Peterson <benjamin@python.org> wrote:
I thought all the slots were supposed to be available through the stable ABI, since you need them to properly define c types.
You're right, I was thinking specifically about the ABI versioning issues (which I exposed in my reply to Petr). Regards Antoine.
participants (3)
-
Antoine Pitrou -
Benjamin Peterson -
Petr Viktorin