When user calls {create_ensure}_and_handle(), we do this in handle_channels(): if (tp_proxy_get_invalidated (channel) == NULL) { /* Keep the handler alive while the channel is valid so keep a ref on * ourself until the channel is invalidated */ g_object_ref (self); g_signal_connect (channel, "invalidated", G_CALLBACK (acr_channel_invalidated_cb), self); } That's because we have to keep the TpSimpleHandler alive while the channel is being handled to avoid MC closing it thinking the Handler is done. TpAccountChannelRequest also haves a reference on the TpChannel. So the only way to get the invalidated signal fired is that the channel is properly closed. So, if the user releases its ref on the TpChannel but for any reason the channel is not closed, we leak the TpChannel until it's properly closed.
Here is a branch that remove completely that ref cycle. It was even more complex :(
16:22 < wjt> Zdra: is there *actually* a weak ref, or is it a borrowed ref? 16:22 < wjt> Zdra: (the commit message and/or comment should say, in any case) 16:23 < Zdra> wjt, it is a weakref by using "invalidated" signal 16:23 < Zdra> wjt, in that cb it remove the channel from the hashtable 16:24 < wjt> as someone who doesn't think proxies should emit invalidated in 'dispose' 16:24 < wjt> i do not enjoy that explanation :p ... 16:29 < smcv> I don't think being a neglectful handler is a supported action, tbh 16:29 < smcv> (where by "neglectful" I mean having a channel and not closing it when you're finished with it) 16:30 < smcv> I'd prefer some documentation to that effect 16:30 < smcv> (applies only to Handler, not to observer/approver) ... 16:33 < smcv> I think my preferred resolution would be to document that yes, if you are a Handler, you are expected to close any channel you're no longer using 16:34 < smcv> shaky rationale: you know best when you've stopped using it 16:36 < Zdra> well, tell handlers to call Close() but still try to make it just work if you just drop your last ref, is nicer IMO 16:37 < smcv> I suppose you could have it g_warning() and Close() if the last ref is dropped without closing it...
> + * borrowed path (gchar *) => weakref TpChannel */ That's not a weak ref: nobody has called g_object_weak_ref(). If you're relying on the fact that TpProxy::invalidated is emitted in dispose, you should specifically say so. I still think it's wrong to assume that unreffing a TpChannel is enough to get it closed; if you're particularly attached to this implementation, please add a comment indicating that you're relying on TpProxy::invalidated during dispose, and emit a g_warning() or something if the proxy is invalidated with TP_DBUS_ERROR_PROXY_UNREFERENCED (which a correct Handler shouldn't cause to happen). That'll also mean that when we stop emitting invalidated during dispose, the use of TP_DBUS_ERROR_PROXY_UNREFERENCED will become an error and we'll fix the assumption that's no longer true :-) An alternative would be to handle the "channel released without closing it" case with g_object_weak_ref(), and emit the g_warning() from the weak-ref callback.
There is no high-level API to close a channel and TpSimpleHandler's doc never say we have to close a channel... So with current API it seemed obvious that unreffing the TpChannel is enough to be done with it. Note that when using tp_account_channel_request_create_and_handle_channel_async(), there is actually no need to close the channel since unreffing it will cause the handler to die and MC close the channel for us. I think that API is made to make the simple case easy and it would be cool to not have to explicitly Close() the channel. So I disagree with adding a g_warning() since I see now reason an explicit Close() is required, and not emitting "invalidated" on dispose would be API break IMO. Though the API doc could recommand to close a channel.
(In reply to comment #4) > So I disagree with adding a g_warning() since I see now reason an explicit > Close() is required, and not emitting "invalidated" on dispose would be API > break IMO. Though the API doc could recommand to close a channel. Not emitting invalidated on dispose would indeed be an API break, but it's an API break we want to make for telepathy-glib 1.0. As a general design principle I'm against having things happen implicitly in dispose, because I want it to be safe to add appropriate refs. For instance, a GAsyncResult holds a ref to both its source object and the user-data, for the duration of the async call - that prevents a whole class of bugs, but if you rely on dispose to Close channels, it'll delay the Close. (For the cases where you really do need to pass a weak ref into an async call, we have TpWeakRef, which is designed to be used as a user-data.)
I know understand better how it was supposed to work: ref is kept on TpChannel only in the case we are an handler. In that case caller is responsible to call Close() on the channel to make it invalidated which will break the ref cycle. Here is a branch documenting this and fixing a real leak: git://git.collabora.co.uk/git/user/xclaesse/telepathy-glib.git
Sorry the URL is: http://git.collabora.co.uk/?p=user/xclaesse/telepathy-glib.git;a=shortlog;h=refs/heads/channels
review+ for commit 37e849d3ac9a8.
Thanks, merged.
This deserves fixing in 0.12, too.
pushed to 0.12 now
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.