[Freeipa-devel] Command instantiation

[John Dennis]

On 01/14/2013 11:06 AM, Petr Viktorin wrote:
>
> IPA Command objects sometimes need to pass some data between their
> various methods. Currently that's done using the thread-local context.
> For an example see dnsrecord_del, which sets a "del_all" flag in
> pre_callback and then checks it in execute.
>
> While that works for now, it's far from best practice. For example, if
> some Command can call another Command, we need to carefully check that
> the global data isn't overwritten.
>
>
> The other way data is passed around is arguments. The callback methods
> take a bunch of arguments that are the same for a particular Command
> invocation (ldap, entry_attrs, *keys, **options). By now, there's no
> hope of adding a new one, since all the callbacks would need to be
> rewritten. (Well, one could add an artificial option, but that's clearly
> not a good solution.)
> In OOP, this problem is usually solved by putting the data in an object
> and passing that around. Or better, putting it in the object the methods
> are called on.
>
> This got me thinking -- why do we not make a new Command instance for
> every command invocation? Currently Command objects are only created
> once and added to the api, so they can't be used to store per-command data.
> It seems that having `api.Command.user_add` create a new instance would
> work better for us. (Of course it's not the best syntax for creating a
> new object, but having to change all the calling code would be too
> disruptive).
> What do you think?
>

Just a few thoughts, no answers ... :-)

I agree with you that using thread local context blocks to pass 
cooperating data is indicative of a design flaw elsewhere.

See the discussion from a couple of days ago concerning api locking and 
making our system robust against foreign plugins. As I understand it 
that design prohibits modifying the singleton command objects. Under 
your proposal how would we maintain that protection? The fact the 
commands are singletons is less of an issue than the fact they lock 
themselves when the api is finalized. If the commands instead acted as a 
"factory" and produced new instances wouldn't they also have to observe 
the same locking rules thus defeating the value of instantiating copies 
of the command?

I think the entire design of the plugin system has at it's heart 
non-modifiable singleton command objects which does not carry state.

FWIW, I was never convinced the trade-off between protecting our API and 
being able to make smart coding choices (such as your suggestion) struck 
the right balance.

Going back to one of your suggestions of passing a "context block" as a 
parameter. Our method signatures do not currently support that as you 
observe. But given the fact by conscious design a thread only executes 
one *top-level* command at a time and then clears it state the thread 
local context block is effectively playing the almost exactly same role 
as if it were passed as a parameter.



-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/