[Bug 29606] Reduce number of needed Account becomeReady calls

[bugzilla-daemon at freedesktop.org]

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

--- Comment #3 from Olli Salli <ollisal at gmail.com> 2010-08-31 02:09:07 PDT ---
I'll reply to the parts I don't agree first and then fix the things I agree
with (the ones I'm not commenting on here) to give you room to respond.

(In reply to comment #2)
> In classes that the constructors
> are not public, declare them first in the protected/private sectors and then
> the other methods, but always implement them before the methods impl. If the
> class has a create method declare/implement it first.

Really? I mean really? Wouldn't it make more sense to have them in the same
order as the class declaration does? I see you've changed TpQt4 to follow this
arbitrary rule where they're not in the same order in other places though so
I'll reorder my added classes for now, but let's discuss if we should make the
implementations ordered identically to the declarations globally somewhere in
the future, though.

> Please make Cache a class inside DBusProxyFactory::Private and declare the
> private class inside dbus-proxy-factory-internal.h.

Why? It'd just make the names horribly long, wouldn't it? Also, moc doesn't
need to scan the Private class and I don't want to expose it to the Cache
implementation anyway. It's not like having the "class Cache"
forward-declaration in the public class declaration would cause any
recompilation - the class body is not there after all (which COULD cause them).

Actually, OptionalInterfaceFactory keeps its entire cache class declared in the
public header... I think that's a bit bad, because any change is going to cause
almost a library-wide recompilation, but that's not an issue for just having a
forward declaration like DBusProxyFactory::Private.

> 
> You may also want to receive DBusProxyFactory *parent in
> DBusProxyFactory::Private constructor and pass it as a parent to Cache, so
> there is no need delete the cache in the destructor.

DBusProxyFactory is not a QObject, so it can't be a parent of another QObject
(and I don't want to make it one just for this).

> 
> - dbus-proxy-factory.cpp
> +    connect(dynamic_cast<DBusProxy*>(obj.data()),
> +            SIGNAL(invalidated(Tp::DBusProxy*,QString,QString)),
> You probably want to dynamic_cast it before using connect and warn in case it
> returns NULL. Same applies to other places you use dynamic_cast without
> checking the return value. Ignore this in places that you are 100% sure it
> won't return NULL.

Well, connect() already warns if you give it NULL I think. However, I did
Q_ASSERT wherever I wanted to be extra sure. The code you're quoting is called
from a place which already Q_ASSERTs.

Anyway, all of these dynamic casts will disappear (and be just implicit
upcasts) in the API/ABI break, thankfully.

> +    // No features requested or they are all ready - optimize a bit by not
> calling ReadinessHelper
> +    PendingReady *readyOp = new PendingReady(mPriv->features, proxyQObject,
> proxyQObject);
> +    readyOp->setFinished();
> +    return readyOp;
> Please use ReadinessHelper here if you want object->isReady() to return true
> when no features are requested.
> 

The intention is that no features (not even core) means that the factory should
not make the class ready ie. make no features ready. We need this a) to be able
to emulate the pre-factory behavior b) for eg. account editing UIs, which
probably want to pass a factory that doesn't make Connections ready to their AM
instance.

So, in short, I DON'T want object->isReady() to return true.

This deserves a mention in the doc comments to come, though, obviously.

> - I think Account/ConnectionFactory classes deserve their own header/source
> file.
> 

Ok, I'll also put FixedFeatureFactory in its own then. The thing is, the
division between the base class and the subclasses sort of went back and forth
during development - at first, Acc/ConnFact were just implementing two virtual
methods, with an oneliner implementation each!

> - Do we really want AccountFactory::coreFactory, I would add another
> constructor/create that takes the features as a param, like create(bus,
> features). I just didn't like the coreFactory method :/

The intention here (which I'll surely explain in the docs) is that coreFactory
when passed to AM will produce the same behavior as AM did in the pre-factory
era.

Adding a Features constructor (with a default param of none probably) is a good
idea though, will do that, and make coreFactory use it. Doing this another
option would be to have the AM default parameter be
AccountFactory::create(QDBusConnection::sessionBus(), Account::FeatureCore) but
I think as a default parameter that surely is long and convoluted to the point
of being unreadable (consider all of the three factory params being like this).

I guess it could be called createWithPreFactoryBehavior or something, but
that's a) convoluted b) doesn't encode what the actual behavior is. Thus the
current naming which is similar to eg. QDBusConnection::sessionBus() (that's
not createForSessionBus() either, is it?)

> 
> - AccountFactory::stockFreshFactory. Please don't add a new name for things
> like create, it will just make users more confuse IMHO, just add
> AccountFactory::create(bus) and create(bus, features) if needed. I know this is
> temporary, but in case you want to add this as public API I would rename it
> first.

Similarly, stockFreshFactory is *NOT* a synonym for create(). It's intended to
produce the current behavior of returning the tp-qt4 Channel subclasses as
appropriate, but with no features set.

In the future when I make ChannelFactory be a bit more dynamic, it will be
possible to specify the subclasses to construct and features to make ready on
them at runtime. Then, create() would return a empty factory with no
subclasses/features set (so always just Channel), and you could build it
yourself - but stockFreshFactory would still be there if one wants to start
from the TpQt4 default subclasses instead (and we want that to be able to
specify it as the default factory in other classes' constructors without a
ridiculously convoluted sequence of like passing all the channel class fixed
properties and a constructor function for them in the default parameter
declaration).

I think the class has a comment just above stockFreshFactory that I don't want
to declare a create() constructor yet exactly because you can't customize it
anyway beyond stockFreshFactory() yet - but rest assured, create() will be
added once ChannelFactory is able to act differently from stockFreshFactory().
Returning what stockFreshFactory() now returns from a create() method would put
us in a situation where the future create() starts being silently incompatible,
as it no longer fills the default behavior in.

>   The current channel-factory is internal, so feel free to remove/add methods
> as you wish, like removing the current create method.

I was a bit wary of doing this as it had been declared TELEPATHY_QT4_EXPORT
(accidentally?). Although there are no headers installed, it's still part of
the .so this way, so I'd think any binary compatibility checkers distributions
/ other downstream have would whine at us for removing/changing the current
static ChannelFactory::create() (and we'd have to explain to each and every one
of them that headers weren't installed, exporting it was accidental and nobody
probably ever used it etc).

At ABI break though, sure, let's do it.

That said, I don't even want to remove the current create() yet before
ChannelFactory is used in the DBusProxyFactory way everywhere in the library
itself.

> - How to ensure that the factories received as param have the same
> busConnection as AM for example? We probably want to _at least_ warn in case
> they differ. Imagine someone creating an AM with sessionBus and the factories
> with systemBus, it won't work really.

QDBusConnection doesn't have a == operator so this is a bit hard to do. Maybe
QDBusConnection::name() could be used, though, but is that reliable? At least
it would indicate different connections to the same bus, which would otherwise
work, as different - but that's something we might want to not allow anyway,
though.

Also, the only reason that the DBusProxyFactory even has a bus param is that I
don't want its cache key to additionally have something unique to a dbus
connection, as it seems that a single factory will only ever be used for a
single dbus connection anyway even if it didn't have the param.

I didn't add a sessionBus() default param to the factory constructors exactly
because I feared users would forget to pass the bus to them even if they used a
different bus for eg. the AM by the way.

So, what should we do? Assert on name() being the same? I'm OK with that, if
that's bound to work.

> 
> - I would rename finalBusNameFrom to uniqueNameFrom to be consistent with what
> we already have in DBusProxy.
> 

My intention was that this is not necessarily the unique name. In fact, for
AccountFactory it will just be the well-known name. The intention in the naming
is that it should be the final name the object will have when constructed,
after any normalization/uniquefying. Will document appropriately, of course. Is
this acceptable?

> +    static AccountManagerPtr create(const ChannelFactoryConstPtr
> &channelFactory,
> +            const ConnectionFactoryConstPtr &connectionFactory =
> +                ConnectionFactory::create(QDBusConnection::sessionBus()),
> +            const AccountFactoryConstPtr &accountFactory =
> +                AccountFactory::coreFactory(QDBusConnection::sessionBus()));
> I wouldn't add default params here, mainly to force people to actually pass the
> factories as param. When we break API we can think of having default factories
> for Core Features everywhere, but having just a channelFactory not being
> default now seems a bit awkward.
> 

Actually, channelFactory is effectively default too. Note that we still have
the old create() and create(bus) variants we have to be ABI compatible with.
Remember that C++ default params work caller-side - having create(bus =
defaultparam) only generates code for create(bus) and the compiler then
auto-fills the default parameter in the caller code if it's not provided, for
example. Here I've updated the create() and create(bus) variants to pass the
default channel factory in addition to the default connection and account
factories, which is exactly the same API usage wise as if channelFactory had a
default param too.

Of course, in the API/ABI break, we should ditch the variants without the
factory params and just have all-default parameter variants. I actually
explained this in a API/ABI break TODO comment just a few lines before the
declaration you quoted, didn't I?

> 
> So after seeing the code I came up with some questions that I believe that we
> should think about before continue doing it.
> 
> Problems that we want to solve with factories:
> 1 - Being able to get objects with a set of features ready in all accessors
> that return such an object
> 2 - Being able to share object instances, so for example a handler would use
> the same connection object as an account (in case they match) if someone is
> using both AM and client in the code.
> 
> Problems that I see with the factories approach:
> - If someone creates a factory and passes it to AM for example and use a
> different factory to create the ClientRegistrar, the objects won't be shared,
> so we still have problem 2

ClientRegistrar will have a constructor which takes an AM and starts using its
factories, which solves this without any extra complexity. The constructor will
be documented as "always use this if you're using an AM", but that's for
another branch and bug #29609. Just wait for it...

> - Simon pointed that some places one may want objects with certain features
> that are not required in other places. In order to do that we would need to use
> different factories which would break 2.

Simon pointed out that this is a problem if we have library-global state, and
then have uncooperative modules (eg. plugins the host application can't
control) which then mess each other up.

However, the solution me and Simon agreed on in the semantics branch was not
having any library-global state (even bus-wide pseudo-singletons) and instead
make ClientRegistrar/AbstractClient take the AM as a constructor param. This
enables those hostile plugins to each construct their own AM and
ClientRegistrar with different sets of features. Note that they should not
share the objects anyway if we consider them not being able to coordinate their
usage of the objects sufficiently - otherwise they'd still be able to mess each
other up.

Also, we agreed that anywhere a single AM/ClientRegistrar is used (a single
plugin, or a host application which provides restricted access to an AM /
ClientRegistrar it constructs for its plugins) we can assume they can be
coordinated to not mess up the objects. And think about it a while until you
say "they're going to mess up each other inevitably, we have to guard against
it" :) As a bit of a straw man, that would be almost equivalent to saying that
we shouldn't allow passing a QDBusConnection from eg. Account to Connection
when constructing it, because the Connection may then mess up Account by
disconnecting it.

> 
> So I was thinking about a solution to this and came up with the following idea,
> correct me if I am wrong:
> - We should have a global cache of objects that should be identified by
> dbusConnection+uniqueName+objectpath.

No we shouldn't, read above ^^ :)

>   To be able to cover #1 we would pass features as param in the constructor of
> AM, Account, Connection, ClientRegistrar, etc. The objects returned by these
> object would have these configured features ready.

This won't work at all, leads to constructor explosion. Consider all the
possibilities about configuring ChannelFactory in the future. It needs to be
configurable on a per-set-of-immutable-properties basis, and at least the
subclass and the features should be configurable for each. So we'd need to add
all of this configurability to ctors everywhere where an object potentially
constructing a Channel is (directly or indirectly) contained. This would mean
currently AccountManager, Account, Connection, PendingChannel, ClientRegistry
and ChannelDispatchOperation at least.

Also, we can't even add the Channel subclass, features construction options yet
because we don't have the implementation to do so yet, so we'd have to now add
ctor variants which can take Account and Connection construction options, and
then deprecate those and add the ones which also take the convoluted Channel
construction options instead. If it will be in a factory, we can just have the
placeholder non-configurable factory param in the constructors and then add
configurability to the factory to be passed through that same constructor.

Consider adding yet another configurable to the picture - the prepare() virtual
allows saying "I want to always groupAddSelfHandle() whenever I'm local pending
in a channel that's constructed for me" in ChannelFactory or a subclass
thereof. Making this kind of configuration option without factories would mean
deprecating all of the then-current AccountManager, Account, Connection,
PendingChannel, ClientRegistry and ChannelDispatchOperation constructors and
adding variants where you can specify this. Granted, this problem is not that
obvious at this stage as I've only yet made AccountManager thus configurable,
and none of the objects it (recursively) constructs.

With factories, adding something like alwaysJoinWhenLocalPending is just a
simple matter of adding that configuration option to ChannelFactory. As the AM
would pass its ChannelFactory to Account, Account to Connection and any future
requestChannelAndHandleIt machinery, etc, the adjustment would propagate to the
entire object tree without any changes to any constructors, no deprecations etc
- just adding a setter and an accessor to ChannelFactory.

In other words, having every proxy construction configurable contained in the
factory is really good because we can just then pass the factory around without
having to understand the semantics of what the user has asked from the factory.
The only semantic is that "we get a PendingReady from the factory, and when it
completes, we can present the object to the user in the state the user asked
for".

>   That would cover all situations, even the one that Simon pointed as we would
> reuse objects everywhere. If I have a connection with FeatureRoster ready
> already and someone wants to create a ClientRegistrar with the connection
> having the FeatureCore only, there is no problem, as it won't have to
> introspect Roster again (it's already there), and in case both want
> FeatureRoster for eg, ReadinessHelper will take care of not
> creating/introspecting the object again if the introspection started already.
> 
> In other words, I believe we should make factories private to the class and
> have global objects that should be used everywhere.

Naa... :) That is actually approximately equivalent to one of the evolutions of
my suggested solution approach to bug #29451 but as explained above and in the
later bug #29451 comments I don't think that's a good idea anymore.

To reiterate: if the parts of an application can't be guaranteed to be able to
coordinate which features it wants everywhere, it can't be guaranteed to be
able to safely handle shared instances of that object either. So we should
exactly share what we specify features in a shared fashion for.

Also note that the factories as passed to AM (and in the future also other
proxy) ctors are ConstPtr params, and that eg. FixedFeatureFactory only ever
allows adding features, not removing them. This means nobody can a) add
conflicting features to make ready in an AM's factory instance accessing it
through AM->accountFactory() etc and b) an application constructing the factory
can't mess up 

We should of course document that the application shouldn't allow access to the
non-const pointers to the factories after passing them to the AM/whatever
constructor, at least not to potentially untrusted plugins. I don't think this
is much of a problem though, as the use will naturally fall into the following
pattern I think:

{
   AccountFactoryPtr accFact = AccountFactory::create(bus)

}

Sorry for the missing semicolons, my custom keyboard layout and opera together
don't allow typing that in bugzilla comments (because it's the altgr+capslock
keys on a typical PC qwerty layout :))

If you like, we can add something like isReadonly() / setReadonly() to the 

> 
> I am not sure if I made myself clear, but I believe now after looking at this,
> that we are trying to solve some issues and find other issues to be worried
> about.

Please get back to me if you still disagree after my above rationale. I'll
surely add some of the rationale to the documentation even if you agree,
though.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.