GNOME Bugzilla – Bug 161177
Allow creation of python classes from g_object_new
Last modified: 2006-01-09 14:12:49 UTC
Attached is a patch to improve support for creating Python implemented GObjects from g_object_new. It calls __new__ in the Python class if one is defined and associates the wrapper with the GObject ref before non-construct properties are set. It does not call __init__ because I could not find a way in libgobject to completely override the object creation process. __init__ could be called, but it wouldn't control when the GObject is created, so it's probably simpler to not call it at all. The use case for this will be creating objects via libglade when libglade supports generic GObject, so maybe it's okay to skip the __init__ method. It should be possible to write a object that can be instantiated either from Python or with g_object_new. This uses a write-only, construct-only property to transfer the PyObject* pointer through g_object_new which is ugly. The alternative is to use a per-thread variable to transfer data, which is probably a bit less ugly because no property name is exposed. The patch currently doesn't support threads; I'll add in the ensure & locking calls later. I'd like feedback on whether to call __init__ or not and whether to use the property value to communicate the PyObject* pointer.
Created attachment 34800 [details] [review] g_object_new support Not thread safe
Created attachment 34801 [details] [review] Working patch Forgot to test after cleaning it up a bit
The patch doesn't work if one python GObject class derives from another one because it's always looking up the parent of the leaf class when finding the parent's class constructor. It could always use the default constructor defined in the root GObject class, but that's probably not the right thing to do if another constructor is defined. I'm beginning to think that this may not be doable given the current libgobject api and the way pygtk creates GObject's. I'll try to look at how other language bindings do this (if they do).
A better approach is probably to put the code to create the PyObject in set_property and not to set a constructor. This avoids the problem with multiple python GObject classes and the PyObject will be created before non-construct properties are set.
Created attachment 38137 [details] [review] Implementation not using gobject constructor field Here's a patch using set_property to set the obj field, which works in my admittedly light testing. The basic question I have is whether we want to bypass __init__ when constructing objects through gobject.new -- it seems like another complication that I don't fully understand the ramifications of. Two arguments in favor of it is that it doesn't require modifications to gobject and that the python pickle mechanism also bypasses __init__ (and one of the motivating examples for this, libglade, is a deserialization library). To support __init__ methods, a mechanism needs to be added to gobject, libglade, or whatever library creates the objects so that a type could specify a closure that would act as a constructor, it would take a list of properties and return an object reference. The current constructor field is too constrained too support the pygtk, at least with the current api -- many of the issues come from the GObject not being created before the __init__ method is run.
Created attachment 46490 [details] [review] using a GPrivate to track object creation This is an alternative solution to the problem. I'm using GObject's constructor and a GPrivate to track object creation. The code will probably causes some issues if someone derives from Python created GObjects in C code, but other than that, it works and is garbage collected correctly. I've been using and testing it with a GStreamer Python element loader.
Created attachment 46912 [details] Problem analysis and alternative resolution plan I've been researching an alternative plan to solve this bug and the construct properties one at the same time. Here are my notes describing it. It requires gobject enhancements, and it's not a simple fix, but I feel that at least this is a clean solution. It doesn't need mutexes, global variables, or special GObject properties. Comments welcome.
Created attachment 46940 [details] [review] updated patch I've updated my patch with suggestions from the developers and tried to add useful documentation. The code itself still does the same.
Now it's much more readable! Although the solution is still a bit of a hack... About this part in pygobject_init: + pyg_object_set_caller(PYG_OBJECT_CALLER_PYTHON); self->obj = g_object_newv(object_type, n_params, params); - if (self->obj) - pygobject_register_wrapper((PyObject *)self); - else + pyg_object_set_caller(PYG_OBJECT_CALLER_EXTERN); If I read this correct, we need to change caller state around g_object_newv for all constructors, not just GObject.__init__, so the patch by itself doesn't completely fix the problem.
Created attachment 46986 [details] [review] alternate implementation using patch in bug #305560
Created attachment 46999 [details] [review] alternative implementation, fixed A small bug fix. I tested this now, seems to work ok! Reference counting appears to be in order as well. Question: do we really want construction properties to be passed into __init__ as keyword arguments?
Created attachment 48693 [details] [review] patch using TLS and InstanceInitFunc Notice that manually coded constructors need to be updated. I only updated a couple. If you don't update, you get an "interesting" infinite recursion when normally trying to instantiate the type, or a GObject leak if using g_object_new from C or gobject.new from Python. I have no idea yet why the infinite recursion happens. In any case, constructors will need to be updated like this (example from gtk.Dialog constructor): - self->obj = g_object_new(pyg_type_from_object((PyObject*) self), NULL); + pygobject_push_wrapper((PyObject *) self); + if (!self->obj) + self->obj = g_object_new(pyg_type_from_object((PyObject*) self), NULL); + pygobject_pop_wrapper();
Hm.. I think the infinite recursion is caused by calling tp_init in the InstanceInitFunc of the gobject: PyObject is created -> __init__ called -> calls parent __init__ -> calls g_object_new -> InstanceInitFunc is called -> since TLS has no wrapper, a new PyObject is created -> calls __init__ on the PyObject -> chains to parent __init__ -> calls g_object_new again -> ...ad infinitum... At least this means constructors that need to be updated are easy to find *wink*
Created attachment 48725 [details] [review] new patch Removed push/pop_wrapper APIs, and added pygobject_construct/constructv instead. Also, to maintain API compatiblity, types need to declare their constructors are compatible with the new interface by calling pyg_set_object_has_new_constructor. The code generator got a new option --new-constructors to do this for all types in a module. Still to do is to check all constructors defined in .override files and update them to the new API...
Okay, attachment 48725 [details] [review] looks a lot better to me, assuming old code compiled against it works fine. John, Benjamin: Last chance for objections? Otherwise this'll go in tomorrow or so.
A couple things from reading the patch: * Can pygobject_construct just push self & call g_object_new, rather than using cut-n-paste code to go through the params? * You may want to note why pygobject_register_wrapper is not called in pygobject__g_instance_init -- I think it's so the sink function isn't called. Actually, will reference counts be correct if the python GC runs before pygobject_register_wrapper is called.
* I can't call g_object_new since it takes a variable number of arguments. But I suppose I could call g_object_new_valist instead. That'll eliminate a good chunk of code. The only problem is having to copy some code from pygobject_constructv. Duplicated code sometimes leads to bugs when you change in one function but not the other. But since it's only 3-4 lines, I suppose it's harmless; I'll change it... * Yes, you are right about correct sink() handling; it's on my TODO list to fix that. I didn't want to make the patch appear to complicated at first :) * I don't see how the python GC could possibly run before pygobject_register_wrapper... We have the GIL; in case we have constructor using new API, we have a wrapper in the stack, so the g_instance_init function follows a code path that doesn't allocate any python objects, thus couldn't possibly trigger GC collection.
Can't properties be set during the call to g_object_new? If they can, arbitrary code gets run so there's a possibility that Python code is run.
John: That's already handled in pyg_object_set_property. Remember gobject code can never call python code directly, everything is passed through our handles, and att all places we call PyObject_Call* we already acquiring the GIL properly.
I was more worried about the reference to the pyobject held by pygobject_wrapper_key in the gobject's data. This is not INCREF'd until pygobject_register_wrapper is called after g_object_newv is returned and the GC support code may assume that it has been. This can occur in the single threaded case -- it's not a result of GIL issues. Speaking of the GIL, shouldn't it be acquired if needed in ygobject__g_instance_init? g_object_new can be called without holding the GIL.
Owen pointed out that using TLS won't work for non-python objects creating new python objects in instance_init. Do we care enough to not include this in 2.8? If a construct_full (or similar functionallity) is added to glib we should be able to adopt it. Is that possible with the current API?
A more common case is: 1. python code creates an object 2. it's wrapper is pushed onto the stack 3. code to set a property ends up calling g_object_new for a python implemented object 4. the gobject init function for the 2nd object looks for a wrapper and finds one, but it's for the initial object because it hasn't been popped from the stack yet
That's a very good point!.. I certainly hadn't thought of that scenario... Unfortunately I don't have any idea at the moment on how to solve this :|
One idea: 1. forget the wrapper stack, make it a plain TLS single-value variable; 2. the constructor wrapper "sets" the TLS variable to the wrapper; the g_instance_init func gets the value but then clears it before returning; the constructor wrapper also clears the TLS (because if the object type is not python-derived, there is no g_instance_init function available to clear it). This should work because properties are only set after all g_instance_init functions are executed. And, yes, I also forgot the GIL guards when calling into python code from g_instance_init. In any case, my advice is to get something committed until the API freeze deadline, and worry about corner cases later. With a good API (I mean pygobject C API, not Pyhton one) in place that encapsulates all aspects of object creation, we should be able to tweak it in order solve the most important cases. The only Python API we need to stipulate and freeze is this one: when gobject.new or g_object_new is called for a GType implemented in python, __init__ get's called on the type with no arguments. The C APIs that we should add before freeze are pygobject_construct[v] and pyg_set_object_has_new_constructor, as defined in my patch. Other functions might be needed also, like pygobject_register_wrapper and pygobject_new variants that do not sink() the gobjects, although we can keep them in private API.
Created attachment 48850 [details] [review] new patch New patch considering the changes I mentioned. I could not test this because of problems with the gprops path that I need to debug first..
OK, now that the gprops patch is fixed, this patch passes distcheck.
Created attachment 48867 [details] [review] new (hopefully final) patch In this patch, I've been updating the manually written constructors in gtk. Most were update to the new API, but a few were not even using g_object_new because of API limitations (some constructor functions have no equivalent construction properties). This caused me to reconsider the strategy of flagging which types have updated constructors and which don't. While initially I had added a codegen parameter to set this flag for all types in any given module, I now realize that getting all constructors updated for a module may be impossible. So, now the codegen automatically sets this flag for new-style autogenerated types. Old-style constructors (not using parameters) and manually written ones don't get that flag set automatically, and is the responsibility of of the bindings author to do this manually after updating the code, like I do in the patch. This patch also fixes the floating references handling. And I fixed a reference leak in gobject.new. As far as I could tell, reference counting is now correct in all situations: 1. calling FooBar() directly where FooBar is a builtin class; 2. gobject.new(FooBar) for builtin class; 3. and 4. the same for a user-defined class. Finally, I implemented one very important bit that was missing. When we have this: class Foo(gtk.Label): def __init__(self): gtk.Label.__init__(self, "hello") In this case, gtk.Label.__init__ realizes self->obj is non-NULL, so doesn't create a new one, but it still has to g_object_set_property from the parameters it receives. As a side effect of implementing this, I had to go back in the old pygobject_construct implementation that collected values and then called pygobject_constructv. Now I'm trying to debug some problems when subclassing gtk.Window and chaining to parent constructor... although this problem apparently already existed in pygtk 2.6 :|
Committed to CVS, with some tests..
*** Bug 123891 has been marked as a duplicate of this bug. ***
John Ehresman found a problem with reference counting on gtk.Window's created with gobject.new. The reference returned by g_object_new is not owned by the caller.
Created attachment 49635 [details] [review] fixes reference counting on gtk.Window's PyGTK already contains 'sink functions', which are called by pygobject_new to 1. transform a floating reference into a normal one; 2. increment reference count on windows when constructing them, since we initially do not own them. The problem is that the window sink func is: if (object->ref_count == 1 && GTK_WINDOW(object)->has_user_ref_count) { g_object_ref(object); } Which means it is sensitive to the reference count. Well, since pyg_object_new (gobject.new) calls g_object_new, then pygobject_new, which increfs the window and then calls the sink functions, when the sink function runs the refcount is == 2, so it doesn't do anything. So, to fix this all we need is to, in pyg_object_new, call the sink functions after the g_object_unref, instead of inside pygobject_new. There's a pygobject_new_full now which lets you avoid calling the sink functions. This patch also promotes sink_object to a private pygobject API, and calls it in pyg_object_new after the g_object_unref.
Comment on attachment 49635 [details] [review] fixes reference counting on gtk.Window's >Index: gobject/gobjectmodule.c >@@ -1649,10 +1649,11 @@ pyg_object_new (PyGObject *self, PyObjec > if (obj) >- self = (PyGObject *) pygobject_new ((GObject *)obj); >+ self = (PyGObject *) pygobject_new_full((GObject *)obj, FALSE); > else > self = NULL; > g_object_unref(obj); >+ pygobject_sink(obj); Does this need to be done for GtkObject and GtkInvisible aswell? What about the type specific sink functions in gtkmodule.c ? >Index: tests/testmodule.py >+class PyWindow(gtk.Window): >+ __gtype_name__ = 'PyWindow' >+ >+ def __init__(self): >+ gtk.Window.__init__(self, gtk.WINDOW_TOPLEVEL) Can you use type() here instead and move it to a real test? (eg, so it's created & tested at test time)
> Does this need to be done for GtkObject and GtkInvisible aswell? It is done. > What about the type specific sink functions in gtkmodule.c ? They are being used.
Created attachment 49984 [details] [review] fixes reference counting on gtk.Window's v2 Now creates the PyWindow type inside each test, using type().
OK, patch committed.