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 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...
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.
As for clearing the state, you already can't rely on that: the batch
plugin doesn't do it.
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
