[Freeipa-devel] Command instantiation

[Rob Crittenden]

Petr Viktorin wrote:
> On 01/14/2013 05:48 PM, John Dennis wrote:
>> 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?
>
> Good point. Actually, making api a factory would eliminate the reason
> for locking Commands: the foreign plugin could do whatever it wanted to
> its instance of the Command, and all would be well unless it modifies
> the class itself (which is about as bad as using setattr).

I still think this would invite people to do even more dangerous things, 
like changing the API on-the-fly.

>
>> I think the entire design of the plugin system has at it's heart
>> non-modifiable singleton command objects which does not carry state.
>
> Yes. I guess I'm just trying to find ways to get out of that trap...

context is your state, right? It is sort of bolted on but it is 
per-request and guaranteed not to leak into anything else (except batch, 
as noted below).

>
>> 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.
>
> Almost. But, lots of Commands call other Commands, and the caller needs
> to know the internals of the callee to make sure the context attributes
> don't clash. Not to mention the other things, such as connection
> backends, that use the thread-local object. It's fragile.

No argument there. We could clean this up somewhat by imposing a 
namespace into variable naming.

>
> As for clearing the state, you already can't rely on that: the batch
> plugin doesn't do it.
>

Yes, speaking of bolted on, that defines the batch plugin pretty well. 
It should be fairly straightforward to clear the state between 
executions though (or at least parts of it, there may be some 
batch-specif cthings we'd want to maintain).

rob