Bug 45514

Summary:	completes GAsyncResults in an insufficiently idle way
Product:	Telepathy	Reporter:	Simon McVittie <smcv>
Component:	tp-glib	Assignee:	Simon McVittie <smcv>
Status:	RESOLVED FIXED	QA Contact:	Telepathy bugs list <telepathy-bugs>
Severity:	normal
Priority:	medium	Keywords:	patch
Version:	git master
Hardware:	Other
OS:	All
URL:	http://cgit.freedesktop.org/~smcv/telepathy-glib/commit?h=insufficiently-idle-45514
Whiteboard:	r+ when unblocked
i915 platform:		i915 features:
Bug Depends on:	45554
Bug Blocks:
Attachments:	Complete results of TpProxy D-Bus calls in an idle

Description Simon McVittie 2012-02-01 12:09:44 UTC

Created attachment 56466 [details] [review]
Complete results of TpProxy D-Bus calls in an idle

GAsyncResult guarantees to call your callback from the main loop.
For historical reasons (basically "5 years ago I didn't know any
better"), TpProxy does not: if the interface is missing or the proxy
has been invalidated, the callback is called re-entrantly. I'm going
to fix that in Telepathy 1.0, but for now, let's give GAsyncResult
the correct semantics.

(This patch is for master; I'm not sure whether the safer thing for 0.14 would be to sprinkle in_idle throughout, or leave it as it is.)

Unfortunately, the patch fails tests, because TpAccount makes broken assumptions about the lifetimes of data and async results. Shaving this particular yak is a job for tomorrow-man.

Comment 1 Jonny Lamb 2012-02-01 12:37:55 UTC

(In reply to comment #0)
> Created attachment 56466 [details] [review] [review]
> Complete results of TpProxy D-Bus calls in an idle

In general, this patch makes me very unhappy indeed. Completing an async result in an idle when it's always necessary makes debugging a pain.

But I agree that it should be there.

Your patch looks fine but I didn't audit any of the cases to see if it was actually necessary.

> (This patch is for master; I'm not sure whether the safer thing for 0.14 would
> be to sprinkle in_idle throughout, or leave it as it is.)

I think leave it without.

> Unfortunately, the patch fails tests, because TpAccount makes broken
> assumptions about the lifetimes of data and async results. Shaving this
> particular yak is a job for tomorrow-man.

Okay I only just noticed there isn't a patch keyword here and yet I still reviewed it. Oh well.

Comment 2 Simon McVittie 2012-02-02 09:41:42 UTC

(In reply to comment #0)
> Unfortunately, the patch fails tests, because TpAccount makes broken
> assumptions about the lifetimes of data and async results.

Now fixed, Bug #45554. This patch applies cleanly after the branch from Bug #45554 is merged into master, and passes tests; so I'll consider it pre-r+ (thanks for reviewing, btw) unless you object.

I'll revert it on next, after I've made D-Bus calls, contact inspection etc. actually always idle when necessary.

(In reply to comment #1)
> In general, this patch makes me very unhappy indeed. Completing an
> async result in an idle when it's always necessary makes debugging a pain.

Assuming you mean "when it's not always necessary": agreed, but it's a necessary evil...

Comment 3 Simon McVittie 2012-04-11 03:22:43 UTC

This is still stalled by Bug #45554, which I'll look at next.

Comment 4 Simon McVittie 2012-04-12 04:27:13 UTC

Fixed in git for 0.19.0. Reverted in next, where it's no longer necessary (because TpProxy is always async in next).

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.