[Bug 76120] [next] preparatory cleanup for GDBus migration

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Mon Mar 17 05:06:54 PDT 2014


https://bugs.freedesktop.org/show_bug.cgi?id=76120

--- Comment #5 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
(In reply to comment #4)
>  - "account-request test: don't leak account manager": I like adding in
> setup:
> 
>   g_object_add_weak_pointer (G_OBJECT (foo), &foo);
> 
>   and in teardown:
> 
>   g_object_unref (foo);
>   g_assert(foo == NULL);

An ingenious hack, but I don't really like overloading @foo to mean two
different things (the strong ref and the weak ref). I prefer the approach taken
by tp_tests_assert_last_unref(), which I added recently.

In any case, this doesn't actually work in that test. We would have to add a
tp_tests_await_last_unref() which spins the main context, because the TpAM is
kept alive by pending method calls after at least some test-cases (usually the
ones that do something incredibly trivial, like constructing a $object and
immediately destroying it, leading to the test *after* the trivial one
failing).

I'm not sure that I really want tp_tests_await_last_unref(), but I could be
persuaded.

>  - "account-request test: prepare account manager": that really is a job for
> the g_object_add_weak_pointer() trick I said above to be sure we are not
> reusing the same singleton in next test.

This is the same situation. I did try fixing it by adding
tp_tests_await_last_unref(), and it worked, but I didn't really like that
solution: it's all too easy for it to become a hard-to-explain "timed out" and
then you have to try to find where it was leaked. In general, leaked pointers
having observable side-effects are Bad™ - I'm really regretting the weak_object
parameters in our APIs now.

I wonder whether the default TpAM (or in Bug #76111 world, the default client
factory) should "forget" otherwise-singleton objects if they are invalidated,
not just if they are disposed. Then we'd never get this situation persisting
beyond the short term "in real life", and regression tests could maybe even
forcibly invalidate them.

Then again, in Bug #76111 world, we'd probably avoid using the global default
TpClientFactory in tests (except in the single test designed to test it), and
call tp_tests_assert_last_unref() on the temporary TpClientFactory in teardown?

>  - "_tp_channel_got_properties: if we were invalidated, don't continue": Why
> do we get the callback if the proxy is invalidated? I though that was one of
> the difference between tp_cli_foo_call_bar() and g_dbus_connection_call(),
> the former does not call callback and the later does call the callback but
> with a GError set.

No. I think you're thinking of cancellation: cancelling a tp_cli_foo_call_bar()
call (either by explicit cancellation, or death of the weak object) means the
callback is not called at all, even if the result has already been received and
the only thing remaining to do is to actually call the callback (in an idle).

Cancelling a cancellable GIO async call means the callback is called with
either the real result or G_IO_ERROR_CANCELLED (usually the latter, but there
is no guarantee - you might be "too late").

I should write *simple* test-cases for this stuff. call-cancellation.c and
disconnection.c don't count, because they're full of corner-cases (although
they have been invaluable for getting the corner-cases right).

>  - "tp_private_proxy_set_implementation: don't use g_assert_cmp* family":
> you can use g_str_equal() when you know both strings are non-NULL.

Yeah, I suppose so. If one of the strings happens to be NULL then we'll jsut
crash, which is what we wanted anyway, since this is "internal error, something
has gone horribly wrong" territory...

>  - "stream-tube test: don't assume one event per main loop iteration": In
> create_tube_service() you should set test->tube_conns to NULL after
> g_list_free_full() I think.

Reasonable.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.


More information about the telepathy-bugs mailing list