GNOME Bugzilla – Bug 695631
Add qdata API to GSource
Last modified: 2013-03-20 00:35:26 UTC
GSource has proved difficult to wrap in a good way in the C++ binding, glibmm. The problems have been discussed for a long time in glibmm bug 561885. The most urgent need is a callback function which is called when a GSource instance is finalized, with enough information availabĺe to be able to delete the C++ wrapper. A pointer to the C++ wrapper must be available. With objects derived from GObject this is done with a call to g_object_set_qdata_full(), with the 'data' argument equal to the required pointer. A similar API in GSource would be very helpful. The same goes for other reference counted classes, but here I prefer to restrict the discussion to GSource. There is already a finalize callback function available to users of GSource. Why don't we use it? Two reasons: 1. No user data, which could be a pointer to a C++ wrapper, is available. 2. If the GSource instance is not created by a call to g_source_new(), but by a call to, let's say, g_io_create_watch() or g_timeout_source_new(), we can't set our own finalize callback, and we can't store extra data by giving g_source_new() a struct_size parameter greater than sizeof(GSource). Weak references, like g_object_weak_ref() and g_object_weak_unref(), would be acceptable as an alternative, but a qdata API is better. Data stored with g_object_set_qdata() or g_object_set_qdata_full() is available in all callback functions, not only when the object is being finalized.
Created attachment 238579 [details] [review] patch: GSource: Add qdata API Here's a patch, showing what I would like. Each of the 4 added functions consists mainly of a call to a g_datalist_id_xxx() function. There is an alternative to this API that adds less code to GSource. GData** g_source_get_qdata_datalist (GSource *source) { g_return_val_if_fail (source != NULL, NULL); return &source->priv->qdata; } Then the user would e.g. call wrapper = g_datalist_id_get_data (g_source_get_qdata_datalist (source), wrapper_quark); instead of wrapper = g_source_get_qdata (source, wrapper_quark); The only new code in GSource would then be one new short function and the call to g_datalist_clear() in g_source_unref_internal(). I chose the alternative in the patch because it resembles the code in GObject and GParamSpec.
I don't like a full-blown qdata API for this. What if you used some sort of chaining with g_source_set_callback()? Then you could wrap the user's userdata in a struct that carried their user_data, their callback, plus any extras you need.... When the source goes away, it's your destroy notify being called... There is also g_source_set_closure() that could be used similarly (and closures have notifiers).
(In reply to comment #2) > I don't like a full-blown qdata API for this. I can understand that. That's why I mentioned a "mini-API" as an alternative. > What if you used some sort of chaining with g_source_set_callback()? Then you > could wrap the user's userdata in a struct that carried their user_data, > their callback, plus any extras you need.... When the source goes away, it's > your destroy notify being called... Actually our present wrapping of GSource uses the destroy notify function, set by g_source_set_callback(). The C++ wrapper is deleted when that destroy notify function is called. That's the problem with our present wrapping! That function can be called too early, when there are still references to the GSource instance and to the Glib::Source C++ wrapper instance. It later results in accesses to deallocated memory. That's why the glibmm bug 561885 was filed. > There is also g_source_set_closure() that could be used similarly (and > closures have notifiers). Aren't such notifiers also called before the GSource instance is finalized, e.g. when g_source_destroy() is called? I would like a possibility to register a callback function which is called when the GSource instance is finalized, with a gpointer user_data available (being a pointer to the C++ wrapper instance). I don't say that it's impossible to fix the glibmm bug without modifying glib, but with a suitable small modification in glib, the fix could be simpler and not at all hackish.
Created attachment 239064 [details] [review] patch: GSource: Add qdata API A patch with the mini-API, comprising only g_source_get_qdatalist().
For completeness, it should be said that there is another small fix to the GSource API discovered in the glibmm bug which would allow us to correctly free the C++ wrappers. In essence, if the unreferencing of the callback data and the NULLing out of the GSource's callback_data and callback_funcs fields are delayed from when the GSource is destroyed in g_source_destroy_internal() to when the GSource loses its last reference after finalization of the GSource in g_source_unref_internal() (which is called by g_source_destroy_internal()), it would allow the callback data that we use to store the wrapper to survive until finalization at which time it would also be accessible so that the C++ wrapper can be properly freed. For the case in which the GSource is not created by a call to g_source_new() (as described in the description, ie. comment 0), we would have to temporarily replace the existing finalize function of the GSource, saving the existing one (if any), so that it can later be called in our custom finalize function, making proper destruction of an existing C++ wrapper also possible. We would have the small inconvenience of having to save the original finalize function (if any) and then call it. However, it would be possible to solve the problem that way. I'm sure, though, that we would accept any solution available. A patch with the necessary changes described follows.
Let me make a small correction. I think this last solution just described would work without modifying GSource much. I think it's a pretty descent solution.
Created attachment 239090 [details] [review] Allow bindings to correctly free their GSource wrappers.
I like this change more but I'm worried that it could result in reference cycles being introduced in some pretty innocent places. Consider the case of someone with a GSource for polling a file descriptor. They keep the source referenced from their user_data for whatever reason -- maybe to change the poll mask or maybe to destroy it themselves if they need to. They also return 'FALSE' if they have EOF condition on the stream, expecting this to destroy the source and free their data (which, in turn, frees the source). Would it not be possible to just whip up another wrapper if the first one is found to be gone? What do you do for something like GVariant, for example?
I talked to Owen and Murray about this on IRC in context of bug 561885. I think we agreed that it is better to work around this issue on the glibmm side.
Note, however, I didn't agree to anything. I'm innocent here. I have no idea about any of this any more.
(In reply to comment #8) > Would it not be possible to just whip up another wrapper if the first one is > found to be gone? What do you do for something like GVariant, for example? I would agree with the sentiment behind this question. There are two separate cases: A) Accessing a GSource created in GLib. In this case, a strongly pinned language object is not needed, and there is no provision in GLib for doing such pinning. B) Creating a custom GSource. In this case, a strongly-pinned language object can be implemented by storing a pointer in the structure derived from GSource. Now, if you were trying to pin in A) anyways - perhaps the current API of a language binding (like glibmm) implies that pinning in case A) is needed. Is g_source_set_callback() useful to implement such pinning? I can see how it could be used in combination with an external "pin table" (hash table of GSource to language object) to possibly do this. Does it matter in that case if the source callback is called before finalize()? I don't see why - the ->finalize() vfunc is completely an implementation detail of GLib. I'm very opposed to change the details of g_source_set_callback() to make it work better as a pinning hack. If anything, a new API would be needed. But I don't really think that is needed is either - it's probably manageable with current APIs, or maybe glibmm needs a small API break to restrict what applications can do to something that is actually implementable.
I pretty much agree with everything said in the last few comments. I think that if there is a way to fix this just in glibmm it would be the best course to take. Looking at the actual glibmm bug, I think I do see one other much more easy way to fix this just in glibmm. And from the reply in comment 8, if it were still necessary to store the C++ wrapper and then destroy it with some sort of callback, the most flexible way would be to add new API to store that wrapper and not use g_source_set_callback() for that since it can be used in the C API before the GSource might be placed in a C++ wrapper.
I think I accidentally changed the fix status of this bug. Marking back to WONT_FIX.