[pygobject] Refactor boxed wrapper memory management strategy
- From: Simon Feltman <sfeltman src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [pygobject] Refactor boxed wrapper memory management strategy
- Date: Mon, 18 Aug 2014 08:32:32 +0000 (UTC)
commit 85175047e66dfc0c0263eac91d8056a95d0a60a0
Author: Simon Feltman <sfeltman src gnome org>
Date: Tue Jul 29 19:29:28 2014 -0700
Refactor boxed wrapper memory management strategy
Change pygi_boxed_new() to accept "copy_boxed" instead of "free_on_dealloc".
This changes memory management so the PyGIBoxed wrapper owns the boxed
pointer given to it. Use __del__ instead of dealloc for freeing the boxed
memory. This is needed for edge cases where objects like GSource can
trigger the finalized callback during de-alloc, resulting in the PyObjects
references counts being manipulated and triggering a re-entrant de-alloc.
Add hack to keep Gtk.TreeIter.do_iter_next/previous implementations working
which rely on pass-by-reference.
See also: https://bugzilla.gnome.org/show_bug.cgi?id=734465
https://bugzilla.gnome.org/show_bug.cgi?id=722899
https://bugzilla.gnome.org/show_bug.cgi?id=726999
gi/gimodule.c | 6 +++-
gi/overrides/GLib.py | 4 --
gi/overrides/GObject.py | 3 ++
gi/pygi-boxed.c | 54 ++++++++++++++++++++++++------
gi/pygi-boxed.h | 4 +-
gi/pygi-source.c | 6 ++-
gi/pygi-struct-marshal.c | 81 +++++++++++++++++++++++++++++++++++++++++++--
tests/test_gi.py | 1 -
tests/test_source.py | 8 +++-
9 files changed, 140 insertions(+), 27 deletions(-)
---
diff --git a/gi/gimodule.c b/gi/gimodule.c
index 44a8fbd..00c9422 100644
--- a/gi/gimodule.c
+++ b/gi/gimodule.c
@@ -520,7 +520,11 @@ _wrap_pyg_variant_type_from_string (PyObject *self, PyObject *args)
py_type = _pygi_type_import_by_name ("GLib", "VariantType");
- py_variant = _pygi_boxed_new ( (PyTypeObject *) py_type, type_string, FALSE, 0);
+ /* Pass the string directly and force a boxed copy. This works because
+ * GVariantType is just a char pointer. */
+ py_variant = _pygi_boxed_new ( (PyTypeObject *) py_type, type_string,
+ TRUE, /* copy_boxed */
+ 0); /* slice_allocated */
return py_variant;
}
diff --git a/gi/overrides/GLib.py b/gi/overrides/GLib.py
index 214d507..e72ed36 100644
--- a/gi/overrides/GLib.py
+++ b/gi/overrides/GLib.py
@@ -575,10 +575,6 @@ class Source(GLib.Source):
def __init__(self, *args, **kwargs):
return super(Source, self).__init__()
- def __del__(self):
- if hasattr(self, '__pygi_custom_source'):
- self.unref()
-
def set_callback(self, fn, user_data=None):
if hasattr(self, '__pygi_custom_source'):
# use our custom pyg_source_set_callback() if for a GSource object
diff --git a/gi/overrides/GObject.py b/gi/overrides/GObject.py
index 30ac542..e922ac0 100644
--- a/gi/overrides/GObject.py
+++ b/gi/overrides/GObject.py
@@ -216,6 +216,9 @@ class Value(GObjectModule.Value):
if self._free_on_dealloc and self.g_type != TYPE_INVALID:
self.unset()
+ # We must call base class __del__() after unset.
+ super(Value, self).__del__()
+
def set_boxed(self, boxed):
# Workaround the introspection marshalers inability to know
# these methods should be marshaling boxed types. This is because
diff --git a/gi/pygi-boxed.c b/gi/pygi-boxed.c
index 4fd90d2..557584b 100644
--- a/gi/pygi-boxed.c
+++ b/gi/pygi-boxed.c
@@ -28,18 +28,26 @@
static void
_boxed_dealloc (PyGIBoxed *self)
{
+ Py_TYPE (self)->tp_free ((PyObject *)self);
+}
+
+static PyObject *
+boxed_del (PyGIBoxed *self)
+{
GType g_type;
+ gpointer boxed = pyg_boxed_get_ptr (self);
- if ( ( (PyGBoxed *) self)->free_on_dealloc) {
+ if ( ( (PyGBoxed *) self)->free_on_dealloc && boxed != NULL) {
if (self->slice_allocated) {
- g_slice_free1 (self->size, pyg_boxed_get_ptr (self));
+ g_slice_free1 (self->size, boxed);
} else {
g_type = pyg_type_from_object ( (PyObject *) self);
- g_boxed_free (g_type, pyg_boxed_get_ptr (self));
+ g_boxed_free (g_type, boxed);
}
}
+ pyg_boxed_set_ptr (self, NULL);
- Py_TYPE (self)->tp_free ((PyObject *)self);
+ Py_RETURN_NONE;
}
void *
@@ -103,7 +111,7 @@ _boxed_new (PyTypeObject *type,
goto out;
}
- self = (PyGIBoxed *) _pygi_boxed_new (type, boxed, TRUE, size);
+ self = (PyGIBoxed *) _pygi_boxed_new (type, boxed, FALSE, size);
if (self == NULL) {
g_slice_free1 (size, boxed);
goto out;
@@ -136,30 +144,48 @@ _boxed_init (PyObject *self,
PYGLIB_DEFINE_TYPE("gi.Boxed", PyGIBoxed_Type, PyGIBoxed);
PyObject *
-_pygi_boxed_new (PyTypeObject *type,
+_pygi_boxed_new (PyTypeObject *pytype,
gpointer boxed,
- gboolean free_on_dealloc,
+ gboolean copy_boxed,
gsize allocated_slice)
{
PyGIBoxed *self;
+ GType gtype;
if (!boxed) {
Py_RETURN_NONE;
}
- if (!PyType_IsSubtype (type, &PyGIBoxed_Type)) {
+ if (!PyType_IsSubtype (pytype, &PyGIBoxed_Type)) {
PyErr_SetString (PyExc_TypeError, "must be a subtype of gi.Boxed");
return NULL;
}
- self = (PyGIBoxed *) type->tp_alloc (type, 0);
+ gtype = pyg_type_from_object ((PyObject *)pytype);
+
+ /* Boxed objects with slice allocation means they come from caller allocated
+ * out arguments. In this case copy_boxed does not make sense because we
+ * already own the slice allocated memory and we should be receiving full
+ * ownership transfer. */
+ if (copy_boxed) {
+ g_assert (allocated_slice == 0);
+ boxed = g_boxed_copy (gtype, boxed);
+ }
+
+ self = (PyGIBoxed *) pytype->tp_alloc (pytype, 0);
if (self == NULL) {
return NULL;
}
- ( (PyGBoxed *) self)->gtype = pyg_type_from_object ( (PyObject *) type);
- ( (PyGBoxed *) self)->free_on_dealloc = free_on_dealloc;
+ /* We always free on dealloc because we always own the memory due to:
+ * 1) copy_boxed == TRUE
+ * 2) allocated_slice > 0
+ * 3) otherwise the mode is assumed "transfer everything".
+ */
+ ((PyGBoxed *)self)->free_on_dealloc = TRUE;
+ ((PyGBoxed *)self)->gtype = gtype;
pyg_boxed_set_ptr (self, boxed);
+
if (allocated_slice > 0) {
self->size = allocated_slice;
self->slice_allocated = TRUE;
@@ -182,6 +208,11 @@ static PyGetSetDef pygi_boxed_getsets[] = {
{ NULL, 0, 0 }
};
+static PyMethodDef boxed_methods[] = {
+ { "__del__", (PyCFunction)boxed_del, METH_NOARGS },
+ { NULL, NULL, 0 }
+};
+
void
_pygi_boxed_register_types (PyObject *m)
{
@@ -192,6 +223,7 @@ _pygi_boxed_register_types (PyObject *m)
PyGIBoxed_Type.tp_dealloc = (destructor) _boxed_dealloc;
PyGIBoxed_Type.tp_flags = (Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE);
PyGIBoxed_Type.tp_getset = pygi_boxed_getsets;
+ PyGIBoxed_Type.tp_methods = boxed_methods;
if (PyType_Ready (&PyGIBoxed_Type))
return;
diff --git a/gi/pygi-boxed.h b/gi/pygi-boxed.h
index 8177588..189a02c 100644
--- a/gi/pygi-boxed.h
+++ b/gi/pygi-boxed.h
@@ -26,9 +26,9 @@ G_BEGIN_DECLS
extern PyTypeObject PyGIBoxed_Type;
-PyObject * _pygi_boxed_new (PyTypeObject *type,
+PyObject * _pygi_boxed_new (PyTypeObject *pytype,
gpointer boxed,
- gboolean free_on_dealloc,
+ gboolean copy_boxed,
gsize allocated_slice);
void * _pygi_boxed_alloc (GIBaseInfo *info,
diff --git a/gi/pygi-source.c b/gi/pygi-source.c
index d2f39c6..afc43e6 100644
--- a/gi/pygi-source.c
+++ b/gi/pygi-source.c
@@ -239,8 +239,10 @@ pyg_source_new (void)
source = (PyGRealSource*) g_source_new (&pyg_source_funcs, sizeof (PyGRealSource));
py_type = _pygi_type_import_by_name ("GLib", "Source");
- /* g_source_new uses malloc, not slices */
- source->obj = _pygi_boxed_new ( (PyTypeObject *) py_type, source, FALSE, 0);
+ /* Full ownership transfer of the source, this will be free'd with g_boxed_free. */
+ source->obj = _pygi_boxed_new ( (PyTypeObject *) py_type, source,
+ FALSE, /* copy_boxed */
+ 0); /* slice_allocated */
return source->obj;
}
diff --git a/gi/pygi-struct-marshal.c b/gi/pygi-struct-marshal.c
index 3ffaf68..5068064 100644
--- a/gi/pygi-struct-marshal.c
+++ b/gi/pygi-struct-marshal.c
@@ -382,9 +382,11 @@ pygi_arg_struct_to_py_marshal (GIArgument *arg,
arg->v_pointer);
} else if (g_type_is_a (g_type, G_TYPE_BOXED)) {
if (py_type) {
+ /* Force a boxed copy if we are not transfered ownership and the
+ * memory is not caller allocated. */
py_obj = _pygi_boxed_new ((PyTypeObject *) py_type,
arg->v_pointer,
- transfer == GI_TRANSFER_EVERYTHING || is_allocated,
+ transfer == GI_TRANSFER_NOTHING && !is_allocated,
is_allocated ?
g_struct_info_get_size(interface_info) : 0);
}
@@ -442,6 +444,36 @@ arg_struct_to_py_marshal_adapter (PyGIInvokeState *state,
iface_cache->is_foreign);
}
+static PyObject *
+arg_boxed_to_py_marshal_pass_by_ref (PyGIInvokeState *state,
+ PyGICallableCache *callable_cache,
+ PyGIArgCache *arg_cache,
+ GIArgument *arg)
+{
+ PyObject *py_obj = NULL;
+ PyGIInterfaceCache *iface_cache = (PyGIInterfaceCache *)arg_cache;
+
+ if (arg->v_pointer == NULL) {
+ Py_RETURN_NONE;
+ }
+
+ if (g_type_is_a (iface_cache->g_type, G_TYPE_BOXED)) {
+ if (iface_cache->py_type) {
+ py_obj = _pygi_boxed_new ((PyTypeObject *) iface_cache->py_type,
+ arg->v_pointer,
+ FALSE, /* copy_boxed */
+ 0); /* slice_alloc */
+ ((PyGBoxed *)py_obj)->free_on_dealloc = FALSE;
+ }
+ } else {
+ PyErr_Format (PyExc_NotImplementedError,
+ "expected boxed type but got %s",
+ g_type_name (iface_cache->g_type));
+ }
+
+ return py_obj;
+}
+
static void
arg_foreign_to_py_cleanup (PyGIInvokeState *state,
PyGIArgCache *arg_cache,
@@ -524,11 +556,52 @@ arg_struct_from_py_setup (PyGIArgCache *arg_cache,
static void
arg_struct_to_py_setup (PyGIArgCache *arg_cache,
GIInterfaceInfo *iface_info,
- GITransfer transfer)
+ GITransfer transfer,
+ GIArgInfo *arg_info)
{
PyGIInterfaceCache *iface_cache = (PyGIInterfaceCache *)arg_cache;
iface_cache->is_foreign = g_struct_info_is_foreign ( (GIStructInfo*)iface_info);
- arg_cache->to_py_marshaller = arg_struct_to_py_marshal_adapter;
+
+ /* HACK to force GtkTreeModel:iter_next() and iter_previous() vfunc implementations
+ * to receive their Gtk.TreeIter argument as pass-by-reference. We create a new
+ * PyGIBoxed wrapper which does not copy the memory and also does not free it.
+ * This is needed to hack the noted vfunc implementations so they can continue
+ * working with bug https://bugzilla.gnome.org/show_bug.cgi?id=722899
+ * being fixed. This hack should be removed once GTK+ has fixed bug
+ * https://bugzilla.gnome.org/show_bug.cgi?id=734465
+ * and we've moved to a new major version.
+ */
+ if (arg_info && g_strcmp0 (iface_cache->type_name, "Gtk.TreeIter") == 0) {
+
+ /* GICallbackInfo */
+ GIBaseInfo *info = g_base_info_get_container (arg_info);
+ if (info && g_base_info_get_type (info) == GI_INFO_TYPE_CALLBACK &&
+ (g_strcmp0 (g_base_info_get_name (info), "iter_next") == 0 ||
+ g_strcmp0 (g_base_info_get_name (info), "iter_previous") == 0)) {
+
+ /* GITypeInfo */
+ info = g_base_info_get_container (info);
+ if (info && g_base_info_get_type (info) == GI_INFO_TYPE_TYPE &&
+ g_type_info_get_tag ((GITypeInfo *)info) == GI_TYPE_TAG_INTERFACE) {
+
+ /* GIFieldInfo */
+ info = g_base_info_get_container (info);
+ if (info && g_base_info_get_type (info) == GI_INFO_TYPE_FIELD) {
+
+ /* GIStructInfo */
+ info = g_base_info_get_container (info);
+ if (info && g_base_info_get_type (info) == GI_INFO_TYPE_STRUCT &&
+ g_strcmp0 (g_base_info_get_name (info), "TreeModelIface") == 0) {
+ arg_cache->to_py_marshaller = arg_boxed_to_py_marshal_pass_by_ref;
+ }
+ }
+ }
+ }
+ }
+
+ if (arg_cache->to_py_marshaller == NULL) {
+ arg_cache->to_py_marshaller = arg_struct_to_py_marshal_adapter;
+ }
if (iface_cache->is_foreign)
arg_cache->to_py_cleanup = arg_foreign_to_py_cleanup;
@@ -556,7 +629,7 @@ pygi_arg_struct_new_from_info (GITypeInfo *type_info,
}
if (direction & PYGI_DIRECTION_TO_PYTHON) {
- arg_struct_to_py_setup (cache, iface_info, transfer);
+ arg_struct_to_py_setup (cache, iface_info, transfer, arg_info);
}
return cache;
diff --git a/tests/test_gi.py b/tests/test_gi.py
index 2c850a7..d4ce662 100644
--- a/tests/test_gi.py
+++ b/tests/test_gi.py
@@ -2276,7 +2276,6 @@ class TestPythonGObject(unittest.TestCase):
@unittest.skipUnless(hasattr(GIMarshallingTests, 'callback_owned_boxed'),
'requires newer version of GI')
- @unittest.expectedFailure # bug 722899
def test_callback_owned_box(self):
def callback(box, data):
self.box = box
diff --git a/tests/test_source.py b/tests/test_source.py
index e0910f9..4c8bcc5 100644
--- a/tests/test_source.py
+++ b/tests/test_source.py
@@ -1,5 +1,6 @@
# -*- Mode: Python -*-
+import gc
import unittest
import warnings
@@ -123,6 +124,7 @@ class TestSource(unittest.TestCase):
return s
s = f()
+ gc.collect()
self.assertTrue(s.is_destroyed())
def test_remove(self):
@@ -205,8 +207,9 @@ class TestSource(unittest.TestCase):
self.finalized = True
source = S()
- id = source.attach()
- print('source id:', id)
+ self.assertEqual(source.ref_count, 1)
+ source.attach()
+ self.assertEqual(source.ref_count, 2)
self.assertFalse(self.finalized)
self.assertFalse(source.is_destroyed())
@@ -214,6 +217,7 @@ class TestSource(unittest.TestCase):
pass
source.destroy()
+ self.assertEqual(source.ref_count, 1)
self.assertTrue(self.dispatched)
self.assertFalse(self.finalized)
self.assertTrue(source.is_destroyed())
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
