GNOME Bugzilla – Bug 752252
Handle multiple callbacks having the same destroy
Last modified: 2018-01-10 20:50:45 UTC
This can be seen with API like g_object_bind_property_full() and is fairly annoying to work around. Currently this causes an abort/fail because the closure is being freed too many times. I looked into the closure code and it turns out it is fairly easy to support and a patch with a test is incoming.
Created attachment 307274 [details] [review] tests: Add regression test for multiple callbacks with the same destroy The test function for GObject-Introspection, and I'm all for naming it something different.
Created attachment 307275 [details] [review] Handle multiple callbacks having the same destroy
Review of attachment 307275 [details] [review]: Thanks for looking at this. See also bug 690397 and bug 690394 specifically related bind_property_full. ::: gi/pygi-closure.c @@ +759,3 @@ + } else { + closure = state->arg_values[user_data_cache->c_arg_index].v_pointer; + closure->ref_count += 1; This seems to assume both callbacks are always the same. If there is a use case for this I think the patch is acceptable if we raise when they differ. e.g. partial support for this... This might be accomplished by ensuring py_arg == closure->function, unless we are wrapping input callbacks somewhere else implicitly in pygi... Otherwise things will get a bit more complicated, but probably still possible. I think we would need to maintain a list of python functions in PyGICClosure. Then potentially make a new ffi_closure for each callback and utilize ffi_closure->user_data for storing an index into the list used when it comes time to call the python function in _pygi_closure_handle. That is if ffi_closure->user_data is available for us to use... It looks like the PyGICClosure is passed both as the user data as an argument to the callback as well as stashed on the ffi_closure, so it is a bit unclear to me if a scheme like this could actually work.
Created attachment 307328 [details] [review] Handle multiple callbacks having the same destroy v2 (In reply to Simon Feltman from comment #3) > Review of attachment 307275 [details] [review] [review]: > > Thanks for looking at this. See also bug 690397 and bug 690394 specifically > related bind_property_full. > I tried to get a quick version of that written but the API is missing a few annotations. I will propose a patch to add them along with my other annotation changes (there are quite a few invalid ones!). > ::: gi/pygi-closure.c > @@ +759,3 @@ > + } else { > + closure = state->arg_values[user_data_cache->c_arg_index].v_pointer; > + closure->ref_count += 1; > > This seems to assume both callbacks are always the same. If there is a use > case for this I think the patch is acceptable if we raise when they differ. > e.g. partial support for this... This might be accomplished by ensuring > py_arg == closure->function, unless we are wrapping input callbacks > somewhere else implicitly in pygi... > > Otherwise things will get a bit more complicated, but probably still > possible. I think we would need to maintain a list of python functions in > PyGICClosure. Then potentially make a new ffi_closure for each callback and > utilize ffi_closure->user_data for storing an index into the list used when > it comes time to call the python function in _pygi_closure_handle. That is > if ffi_closure->user_data is available for us to use... It looks like the > PyGICClosure is passed both as the user data as an argument to the callback > as well as stashed on the ffi_closure, so it is a bit unclear to me if a > scheme like this could actually work. I don't know if I took the same approach but the updated test now works with both callbacks being different.
Created attachment 307405 [details] [review] tests: Add regression test for multiple callbacks with the same destroy v2 Updated to pass GI's make check
Created attachment 307406 [details] [review] Handle multiple callbacks having the same destroy v3 Simplified to only use the GPtrArray when dealing with callbacks which have a GDestroyNotify.
Review of attachment 307406 [details] [review]: This is a very clean approach, nice work! ::: gi/pygi-closure.c @@ +720,3 @@ + /* Must be done before the following check as it will set destroy_cache to NULL */ + g_assert ((destroy_cache == NULL && callback_cache->scope != GI_SCOPE_TYPE_NOTIFIED) || + destroy_cache = _pygi_callable_cache_get_arg (callable_cache, callback_cache->destroy_notify_index); What do you think about raising a Python exception? From the POV of a Python programmer, it might make sense to give a more friendly message that the API is not introspection friendly (so bug reports would be redirected to the library in question instead of pygi).
Created attachment 307604 [details] [review] Handle multiple callbacks having the same destroy v4 (In reply to Simon Feltman from comment #7) > Review of attachment 307406 [details] [review] [review]: > > This is a very clean approach, nice work! > > ::: gi/pygi-closure.c > @@ +720,3 @@ > + /* Must be done before the following check as it will set destroy_cache > to NULL */ > + g_assert ((destroy_cache == NULL && callback_cache->scope != > GI_SCOPE_TYPE_NOTIFIED) || > + destroy_cache = _pygi_callable_cache_get_arg (callable_cache, > callback_cache->destroy_notify_index); > > What do you think about raising a Python exception? From the POV of a Python > programmer, it might make sense to give a more friendly message that the API > is not introspection friendly (so bug reports would be redirected to the > library in question instead of pygi). It really shouldn't be possible for it to happen, the annotation docs mention that a GDestroyNotify is required. However, the scanner doesn't currently check that this is the case. I've added both exceptions just to be sure.
Created attachment 307605 [details] [review] tests: Add torture test for callables taking callbacks with different scopes This is a torture test to check that multiple callbacks all with a different scope are handled correctly. Yes, I have seen API that likes to mix the scopes quite a bit. I will attach the corresponding GI patch if this test is desired.
Review of attachment 307605 [details] [review]: ::: tests/test_everything.py @@ +1019,3 @@ + notified_callback2, + ud) + TestCallbacks.called += 1 It is unclear where 3, 4 and 4 are coming from. While I could read the test_callbacks_torture_signature_0() code, I think it would be nice to at least clarify this with a comment. @@ +1021,3 @@ + self.assertEqual(res, 37 * 3 + 39 * 4 + 40 * 4) + + return 39 I recommend using a local dictionary (or collections.Counter) for storing the callback counts individually. So we'd have something like: self.assertEqual(counters['call_callback'], 3 * 100) This has the benefit of ensuring integrity per-callback type as well as allowing removal of dependency on a class variable.
Review of attachment 307604 [details] [review]: LGTM although the patch isn't applying anymore.
Review of attachment 307405 [details] [review]: I'm getting a failure with with Regress-1.0.gir when running "make check" and this patch applied.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/pygobject/issues/102.