[fpc-devel] TAVLTree(avl_tree.pp) thread safety, fcl-xml(DOM) is also concerned.

[Vinzent Höfler]

Inoussa OUEDRAOGO wrote:
> 2008/8/6 Mattias Gaertner <nc-gaertnma at netcologne.de>:
>> On Wed, 6 Aug 2008 19:41:27 +0100
 >>
>> Providing a local node mem manager does not repair xml dom.
> 
> A "threadvar" declared variable is local to the calling thread.

Yes. But the reference this threadvar is pointing to is _not_.

ThreadVar don't help here unless you provide them with a different node 
manager reference in each thread which is kind of senselessly redundant 
considering that your patch already can provide a local reference for 
each instance created.

And even so, the XML code does not call this new API unless changed, so 
your patch actually changes nothing in regard to the XML data.

It only works in cases where I directly instantiate an AVLTree. Or, how 
else is me - the programmer - supposed to know if a class uses an 
AVLTree or not? And even if I knew that (by digging through the whole 
source), I couldn't do anything about it, because the classes don't 
provide that interface to me. IOW: I don't have control over it.

[moving NodeManager to interface]
> Good solution, but : if ThreadA create an avl object that is _exclusively_ used
> in ThreadA, why should ThreadA wait in a multithreaded node mem manager?

That's where a ThreadVar becomes useful: Each thread could install its 
personal instance of the (unsafe) NodeMemManager via this "global" (yet 
thread-local) reference. No further changes to the code needed, but the 
programmer must be aware of this problem each time he creates a thread 
using any instance of the AVLTree directly or indirectly.

That's surely not a nice solution, but something you can at least work 
with. Although it still smells rotten to me.

> That is when comes the constructor that takes a node mem manager.
> That way, ThreadA could provide a local node mem manager that is not
> multithreaded and thus not paying penalty of multithreaded node mem manager.

That's theoretically possible (but then again, this is perfectly 
solvable without ThreadVars), but this change in the API must then be 
followed up to *any* class that /might/ use an AVLTree instance to be 
any effective. I'd say: No way to do that, especially not in the 
timeframe you propose. And then you may consider the required changes to 
all the existing code, to make that actually thread-safe...


IMO, the least intrusive change would be to define the NodeManager as 
ThreadVar and let it "somehow" determine if it's running in a new thread 
and then overwrite the thread local variable with a new instance. It 
would leave all existing interfaces untouched and even would make 
already existing code automatically thread-safe in that regard.

Of course, removing the usage of the AVLTree altogether would achieve 
about the same, but would be a slightly more intrusive change.


Vinzent.