[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:
>
>> TAVLTree in avl_tree.pp is not thread safe due to the node
>> allocation and de-allocation done through the global
>> declared "NodeMemManager" variable. TAVLTreeNodeMemManager
>> implementation is cleary not thread safe, which btw IMHO
>> is a good thing ( for performance reason).
>>
>> Proposition :
>>
>> (a) TAVLTree should allow, at construct time, to
>>    specify a Node memory manager which will be kept and used.
>>    If not specified the global one will be used.
>>
>> (b) "NodeMemManager" should be declared as "ThreadVar".
>
> Huh? What's the point of having a global (read: constant) default declared
> as threadvar? So, (b) is unnecessary (at least in your patch).
>
> Another solution would be to declare it as ThreadVar and overwriting it upon
> construction.
>
> But wait... this makes me think if we can't do it automatically: If we have
> a new thread, we need a new instance in most cases anyway (except when
> there's actually only one thread handling XML-data, which I consider rather
> unlikely).
>
>> This changes does not break the API while making the
>> implementation thread safe.
>
> Depending solely on the discipline of the caller. And consider the XML-case,
> it wouldn't even be possible without changing this API, too. And most likely
> a lot of other APIs on the way to tackle it down.
>
>> By the way note that the XML DOM's implementation ( TDOMNode_WithChildren
>> )
>> uses TAVLTree, so every code that uses the fcl-xml package mainly
>> the DOM unit, is not thread safe. If this proposition is accepted
>> it will be nice to have it introduced in the soon to be release
>> fpc 2.2.2, in the case that is still possible, mainly for server
>> programming.
>
> As I just pointed out, it wouldn't make handling XML data threadsafe.
> There's no way to specify upon creation of the TXMLDocument that this will
> be used in another thread, needing a new NodeManager instance.

Indeed.

-- 
Inoussa O.