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

[Inoussa OUEDRAOGO]

2008/8/6 Vinzent Höfler <JeLlyFish.software at gmx.net>:
> 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.

Even this schema is not thread safe : ThreadA create a avl and
populate it. ThreadB
delete some nodes of that avl. The deleted nodes have been created
using ThreadA's
node mem manager while collected by ThreadB's node mem manager.

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

Introducing a AVLTree implementation that skip alltogether the node mem manager
( see my second proposition about TBaseAVLTree, TAVLTree and TAVLManagedTree)
seems like a non breaking change still being thread safe and providing
a way to use
a node mem manager whenever needed.

-- 
Inoussa O.