Bug 45514 - completes GAsyncResults in an insufficiently idle way
Summary: completes GAsyncResults in an insufficiently idle way
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL: http://cgit.freedesktop.org/~smcv/tel...
Whiteboard: r+ when unblocked
Keywords: patch
Depends on: 45554
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-01 12:09 UTC by Simon McVittie
Modified: 2012-04-25 07:27 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Complete results of TpProxy D-Bus calls in an idle (18.74 KB, patch)
2012-02-01 12:09 UTC, Simon McVittie
Details | Splinter Review

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.