Bug 24257

oops, ECOMPONENT

0.10 blocker:

14:38 < smcv> http://bugs.freedesktop.org/show_bug.cgi?id=24257 worries me a 
              little
14:38 < smcv> in practice it means "if the account is deleted, prepare never 
              terminates"
14:40 < sjoerd> smcv: that should probably be a blocker indeed

Okay, so I've taken a look through the code, and I think I have something which might work, but it seems somehow too easy. If it looks like an obviously stupid idea then shout and I can avoid wasting too much time.

Current Situation
===================


Looking at the code, it seems that the only place where prepare_async is called is from tp_account_manager_ensure_account. If prepare_finish returns an error, it is silently ignored (_tp_account_manager_account_ready_cb silently returns). Therefore, having prepare return an error won't actually do anything in this case.

There is an _tp_account_manager_account_invalidated_cb but it is only registered if/when prepare finishes successfully. Also, it only listens for domain == TP_DBUS_ERRORS && code == TP_DBUS_ERROR_OBJECT_REMOVED. Are there any other cases/reasons why an account might be invalidated?

Proposed solution
====================

1) Register account_invalidated_cb in ensure_account rather than in account_ready_cb.

2) For completeness if anyone else wants to call prepare_async and get called back.

define _tp_account_invalidated_cb () which loops over priv->callbacks and makes them all fail. I think I know how to do this, after reading the gio docs.

Register invalidated_cb with the invalidated signal (somewhere inside _tp_account_constructed)

Concerns
=========

I suspect that there's some memory management that's going to mess me up here: If you connect to a signal on yourself, does it add a ref to you like it does in python? If so then will we be kept alive forever? Would it be better to register a separate signal for the duration of the prepare (register each time prepare is called, keeping the id in priv->callbacks, and unregister it when we're done)?



Also: do you want a test for this? If so, that might be a little harder because I don't know what else calls prepare, and just implementing 1) would fix the account_manager case.

(In reply to comment #3)
> Looking at the code, it seems that the only place where prepare_async is called
> is from tp_account_manager_ensure_account.

Isn't it public API? I'm pretty sure it is. If so, then the only place where it is called is "anywhere" :-P

> There is an _tp_account_manager_account_invalidated_cb but it is only
> registered if/when prepare finishes successfully. Also, it only listens for
> domain == TP_DBUS_ERRORS && code == TP_DBUS_ERROR_OBJECT_REMOVED. Are there any
> other cases/reasons why an account might be invalidated?

A TpProxy can be invalidated for any reason at any time. It shouldn't ever be invalidated twice, and after it has been invalidated once, prepare_async should notice that tp_proxy_get_invalidated() returns a non-NULL GError, and just fail with that GError "immediately" (i.e. soon, in an idle).

> 2) For completeness if anyone else wants to call prepare_async and get called
> back.
> 
> define _tp_account_invalidated_cb () which loops over priv->callbacks and makes
> them all fail. I think I know how to do this, after reading the gio docs.
> 
> Register invalidated_cb with the invalidated signal (somewhere inside
> _tp_account_constructed)

Yes, please do this.

> I suspect that there's some memory management that's going to mess me up here:
> If you connect to a signal on yourself, does it add a ref to you like it does
> in python? If so then will we be kept alive forever?

I don't think connecting to your own signal does ref you, unless you explicitly ref the user_data passed to the signal, so it should be OK. Check with refdbg...

> Also: do you want a test for this?

Yes please, if possible. I realise that making a stub Account with appropriate behaviour could be tricky, though.

Okay, code can be found at http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=shortlog;h=refs/heads/fix-24257

I will look into writing tests tomorrow morning. invalidated-while-invoking-signals.c will likely be my first reference point unless someone has a better idea.

status update:

I thought I was pretty much there for AccountManager, in http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=shortlog;h=refs/heads/fix-24257 and was about to move on to writing tests for Account. Then I noticed that prepare_finish was returning true when is_prepared was false (I was only checking the former previously).

It seems that that particular bug was there before I touched anything. Will try to get this fixed, then rebase everything on top of it.

Okay, so it turns out it was just a typo in my test. Should have been TP_ACCOUNT_MANAGER_FEATURE_CORE but I wrote TP_ACCOUNT_FEATURE_CORE.

Well that's annoying.

*gets on with the rest of his tests*

Okay: A hefty chunk of rebase later, and I give you:

http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=shortlog;h=refs/heads/fix-24257-alsuren

<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=aa085b71d2ab0d91e347484c578a200dae581a2c>:

newline between statement and "if" block in the second hunk, please

<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=7c079e56bd7c6a1300d8106893d63dd505f9c66e>:
<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=66aa9a1de2590e59aef33a7658479ed15cd425d4>:
> Fix prepare for Account

I'd prefer a one-liner like "fd.o#24257: tp_account_prepare: fail pending calls if invalidated"; likewise for the AM.

(fd.o# because many Telepathy users - GNOME, Maemo, every distro - have their own idea of what bug numbers mean; bug# at the beginning because it's short and has a high information density; reference the module; explain what you're fixing, rather than just saying "fix")

<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=d9a7adfca0941e0c6b468e8f793b7380f22d9f21>:

The check should be a g_return_val_if_fail, and should probably use tp_dbus_check_valid_object_path() directly. It can replace the existing path != NULL check, since tp_dbus_check_valid_object_path() will itself g_return_val_if_fail on NULL.

<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=90837609f00c62ade6fb980dcdcb5a1b81f8bb3f>:

-2009 at the high end of the copyright ranges please.

>+/* FIXME: Do a purge of headers at some point */

Please sort the telepathy-glib/foo block alphabetically, at least.

>+G_DEFINE_TYPE_WITH_CODE (SimpleAccountManager,

Doesn't need a trailing semicolon either in the code or after the last parenthesis (G_DEFINE_TYPE_WITH_CODE supplies both for you, so the two extra semicolons create two empty statements, which some C compilers whine about).

>+/* This structure is actually unused. I included it so I could extend things
>+ * in future if I wanted without huge re-compiles. Turns out everything is
>+ * compiled into one big lib though, so changing this .c file still kicks off
>+ * a big re-compile.

This isn't worth commenting - doing the "pimpl" coding style (FooPrivate) is not unusual. Also, it's a big relink, not a big recompile :-P

>+ */
>+struct _SimpleAccountManagerPrivate
>+{
>+  /* Also unused. Only included because private structures must be >0 bytes. */
>+  GList accounts;

If it's unused, I'd prefer "int dummy;", which appears in some of the examples too.

>+simple_account_manager_get_property (GObject *object,

If you're responding to my coding style nitpicking anyway, I'd like to request a blank line between cases (so between break and default, here) :-)


>+static const char *ACCOUNT_MANAGER_INTERFACES[] = {
>+    TP_IFACE_ACCOUNT_MANAGER,
...
>+      "Interfaces implemented. In this case just AccountManager",

That should just be the empty list. Interfaces is defined to be the set of "extra interfaces" - if you're requesting the o.f.T.AM.Interfaces property and it worked, you ought to be able to infer that o.f.T.AM is a supported interface without any additional help :-)

<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=1744d8be9481a6e09eb583906e4e419f464056bd>:
+  // TODO: call things like tp_account_manager_create_account_async.
+  // This clearly needs to be done in the prepare callback. Might have to
+  // think of a way to refactor this.

Either fix this, or file a trivial/low bug for it (and use C-style comments rather than C++-style).

<http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=8ab93a727864f0117c08b789cc62a438e4adf15f>:
> +  g_assert (test->prepared == tp_account_manager_is_prepared (test->am, TP_ACCOUNT_MANAGER_FEATURE_CORE));

g_assert_intcmp (t_a_m_i_p (), ==, t->p) ?

> +assert_failed_action (gpointer script_data,

Don't you want to free the GError here? If tests don't leak, it's easier to valgrind the library. (Either put a const GError * in the struct, or copy any unowned errors.)

(In reply to comment #9)
> <http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=aa085b71d2ab0d91e347484c578a200dae581a2c>:
> 
> newline between statement and "if" block in the second hunk, please
> 
Ah. I wanted to group the error with the if statement, since that's the only place it's used. I defer to your style though.

> I'd prefer a one-liner like "fd.o#24257: tp_account_prepare: fail pending calls
> if invalidated"; likewise for the AM.
> 
"fd.o#24257: tp_account_prepare: fail if invalidated" is clearer when truncated to 50 chars. "fd.o#24257: tp_account_manager_prepare: fail if invalidated" is harder, but easy enough to infer based on the previous commit message.

> <http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=d9a7adfca0941e0c6b468e8f793b7380f22d9f21>:
> 
> The check should be a g_return_val_if_fail, and should probably use
> tp_dbus_check_valid_object_path() directly. 
I'm slightly confused by the difference between return.*if_fail and assert. They are both used for programmer errors, and both cause the program to abort when I use them. I suppose they do different things if you turn asserts off or something?
> It can replace the existing path !=
> NULL check, since tp_dbus_check_valid_object_path() will itself
> g_return_val_if_fail on NULL.
tp_account_new() calls tp_account_parse_object_path() which calls tp_dbus_check_valid_object_path() among other things. 

Not sure whether it's worth repeating lots of complicated asserts, but != NULL is cheap. I just wanted to fail as soon after the error occurred as possible. I might be doing premature optimisation though. 
Changed to g_return_val_if_fail (account != NULL, NULL); and changed the commit message to reflect this. Tell me if you want 
g_return_val_if_fail (tp_account_parse_object_path (object_path, NULL, NULL, NULL, error), NULL); as well.
> <http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=90837609f00c62ade6fb980dcdcb5a1b81f8bb3f>:
> 


> If it's unused, I'd prefer "int dummy;", which appears in some of the examples
> too.
Thanks. I wasn't sure the best way to make this obvious. That's why I commented it so heavily.
> 
> >+simple_account_manager_get_property (GObject *object,
> 
> If you're responding to my coding style nitpicking anyway, I'd like to request
> a blank line between cases (so between break and default, here) :-)
> 

> 
> <http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=1744d8be9481a6e09eb583906e4e419f464056bd>:
> +  // TODO: call things like tp_account_manager_create_account_async.
> +  // This clearly needs to be done in the prepare callback. Might have to
> +  // think of a way to refactor this.
> 
> Either fix this, or file a trivial/low bug for it (and use C-style comments
> rather than C++-style).
> 
My bad. Forgot to remove the comment. I guess the part about testing the rest of AccountManager still stands though. Will file a bug in the morning.

> <http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=8ab93a727864f0117c08b789cc62a438e4adf15f>:
> > +  g_assert (test->prepared == tp_account_manager_is_prepared (test->am, TP_ACCOUNT_MANAGER_FEATURE_CORE));
> 
> g_assert_intcmp (t_a_m_i_p (), ==, t->p) ?
> 
Fair point. Actually, should I be putting !! in front of bools before comparing them, or is gboolean guaranteed to be 1 or 0 somehow?

> > +assert_failed_action (gpointer script_data,
> 
> Don't you want to free the GError here? If tests don't leak, it's easier to
> valgrind the library. (Either put a const GError * in the struct, or copy any
> unowned errors.)
> 
Ah memory management. I'll get used to it eventually. I never did run refdebug or valgrind on anything, did I? Oh well. Maybe tomorrow.

http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=shortlog;h=refs/heads/fix-24257-alsuren is now the product of my 2:00am brain.

(In reply to comment #10)
> "fd.o#24257: tp_account_prepare: fail if invalidated" is clearer when truncated
> to 50 chars.

Given that it's often difficult to write a good one-line commit message in *80* characters, I don't think truncating to 50 should necessarily be a concern :-)

> I'm slightly confused by the difference between return.*if_fail and assert.
> They are both used for programmer errors, and both cause the program to abort
> when I use them. I suppose they do different things if you turn asserts off or
> something?

g_return(_val)_if_fail: the caller of this function was wrong

g_assert: the internal state of this code is wrong

The failure of a g_return(_val)_if_fail is a g_critical (critical warning, not fatal by default) plus an early return. The caller might crash (e.g. presumably they weren't expecting NULL to be returned, certainly not with the GError unset) but that's their problem.

The failure of a g_assert is a g_error, which is always fatal.

Most Telepathy regression tests, and most of our CMs, run with fatal criticals in order to promote high-quality code.

> Changed to g_return_val_if_fail (account != NULL, NULL); and changed the commit
> message to reflect this. Tell me if you want 
> g_return_val_if_fail (tp_account_parse_object_path (object_path, NULL, NULL,
> NULL, error), NULL); as well.

I think that's reasonable.

> > <http://git.collabora.co.uk/?p=user/alsuren/telepathy-glib.git;a=commitdiff;h=8ab93a727864f0117c08b789cc62a438e4adf15f>:
> > > +  g_assert (test->prepared == tp_account_manager_is_prepared (test->am, TP_ACCOUNT_MANAGER_FEATURE_CORE));
> > 
> > g_assert_intcmp (t_a_m_i_p (), ==, t->p) ?
> > 
> Fair point. Actually, should I be putting !! in front of bools before comparing
> them, or is gboolean guaranteed to be 1 or 0 somehow?

gboolean is secretly a gint, so yes it can have non-1 true values. In production code you should never compare gbooleans by equality, but I think in tests it's acceptable to do so - in a test, you want your assertion to be strict, after all.

Looks OK now, I think. No need to patch the test code to use it now, but in future, g_error_free / error = NULL can be replaced with g_clear_error (&error).

(In reply to comment #11)
> > They are both used for programmer errors,
> 
> g_return(_val)_if_fail: the caller of this function was wrong
> 
> g_assert: the internal state of this code is wrong

To put that another way: they're both programmer error, but it's a different programmer! Suppose I call a function in a module you wrote. g_return...if_fail indicates that the resulting error might be my fault - passing wrong parameters or something (I'd have to check the backtrace to be sure, since it could have been one of your functions passing wrong params to another). g_assert indicates that the error is your fault (your code internally getting into a "can't happen" situation).

Merged to master.