GNOME Bugzilla – Bug 704867
Need a way to cancel gupnp_service_info_get_introspection_async
Last modified: 2013-12-10 13:44:45 UTC
GUPnP provides a function to asynchronously retrieve service description files. Unfortunately, it does not provide a cancellation method to cancel such operations. This makes it difficult for clients to close down cleanly, or cancel certain user initiated operations. For more information see the following mailing list post. https://mail.gnome.org/archives/gupnp-list/2013-July/msg00003.html A possible solution would be to 1. Deprecate gupnp_service_info_get_introspection_async. 2. Add a new version of gupnp_service_info_get_introspection_async that returns a handle that can be used for cancellation, e.g., GUPnPServiceIntrospectionHandle gupnp_service_info_get_introspection_async_v2 (GUPnPServiceInfo *info, GUPnPServiceIntrospectionCallback callback, gpointer user_data); 3. Add a new function that will allow users to cancel requests to retrieve service introspection data, e.g., void gupnp_service_info_get_introspection_async_cancel(GUPnPServiceIntrospectionHandle handle)
hrm, v2 is so ugly. Also, to be nice to G-I the handle needs to be boxed or fundamental I think. Just something to keep in mind
I think I'd go for void gupnp_service_info_get_introspection_async_full (GUPnPServiceInfo *info, GUPnPServiceIntrospectionCallback callback, GCancellable *cancellable, gpointer user_data); I suppose "callback" should still be called with GError set to G_IO_ERROR_CANCELLED (Or something similar) ?
This might interfere with _dispose where all pending messages are cancelled :-/
(In reply to comment #3) > This might interfere with _dispose where all pending messages are cancelled :-/ Can you elaborate on that? Your idea seems good to me. I thought in g_s_i_get_introspection_async_full() we would set a cancelled signal handler with GetSCPDURLData* (or something like it) as the userdata. If the handler is called after the GetSCPDURLData* is freed in dispose, that would be a problem but I suppose we would do a g_cancellable_disconnect() in dispose and that should prevent our handler being called, right? That does imply taking a reference to the GCancellable in GetSCPDURLData*.
I meant the "send a signal on cancel" is interfering with the code in dispose. Currently, the only way a pending SoupMessage is cancelled is during _dispose, so the SoupMessage's "got_scdp_url" [1] handler is just returning on cancel and not removing the SoupMessage from the list of pending messages because that would be list modification while iterating in dispose. We would need two code paths to distinguish between "user cancelled" and "we cancelled because of shutdown" to prevent recursion and being able signal the user in the "user cancelled" case. Maye the signal handler should be attach to the message's signal and not on the session and disconnected in one of the two cases. Does that make sense? [1] https://git.gnome.org/browse/gupnp/tree/libgupnp/gupnp-service-info.c#n619
(In reply to comment #5) > I meant the "send a signal on cancel" is interfering with the code in dispose. > > Currently, the only way a pending SoupMessage is cancelled is during _dispose, > so the SoupMessage's "got_scdp_url" [1] handler is just returning on cancel and > not removing the SoupMessage from the list of pending messages because that > would be list modification while iterating in dispose. The canceled callback can remove the pending message as well, can't it? here's a potential double free there as the application "got introspection" callback could call g_cancellable_cancel() which would trigger our canceled callback, but that's avoidable. > We would need two code paths to distinguish between "user cancelled" and "we > cancelled because of shutdown" to prevent recursion and being able signal the > user in the "user cancelled" case. The canceled callback should be able to call the applications "got introspection" callback with G_IO_ERROR_CANCELLED, I think. Well, it doesn't seem like a massive amount of work so I could just as well try it...
Yeah, that was more of a note. I've half a patch (adding the API but not the cancel handling) lying around at home, just noticed I forgot to attach it here
Created attachment 263440 [details] [review] Add gupnp_service_info_get_introspection_async_full() This version of get_introspection can be cancelled using GCancellable.
Created attachment 263441 [details] [review] tests: Add a way to test cancelling introspection gets First ctrl-C in test-introspection will cancel all current and future gupnp_service_info_get_introspection_async_full() calls.
I happened to do this while not connected to internet: feel free to use that one or your own. The second patch was what I used for testing, I guess it could be added as well...
Review of attachment 263440 [details] [review]: Wrong indent, 4 vs. 8. Also, technically, there are some single-line ifs that could do without the curlies, but I'm wondering whether we should change the style to require curlies all the time. Please commit after fixing the indent, this is better than my stuff.
Review of attachment 263441 [details] [review]: Looks good, please commit after committing the other patch.
Fixed indenting and removed the braces in singleline ifs (I guess any rule is better than no rule). And one more fix: made sure we don't g_object_ref() a NULL pointer in gupnp_service_info_get_introspection_async_full().