Re: Exceptions due to freemarker's use of non-thread-safe DOM library

Tuesday, July 19, 2011, 6:12:08 PM, Newman, John W wrote:

>>"So, should the DOM wrapper put sync on the DOM *Document* (the root) each time it accesses a node in it? Or what's less blocking way of synchronizing?"
>
> As far as I understand, you need to sync on the current node when
> invoking getChildNodes() and also iterating through a node list.

(Already sound tricky... locking of multiple objects in an
uncontrolled order will lead to deadlocks.)

> Those are the two areas where our code using the dom has already
> experienced problems, adding a sync block cleared it up. You also
> need to sync around importNode() but I don't see any of that in your
> dom package. They have not been very clear on what needs synced and
> what is ok .. I have a lingering feeling that it could be much more
> than that, possibly everything..

Does it mean that you can't even know what need to be synchronized,
except by reverse engineering Xerces? That doesn't sound good at all.
Especially because then things can break in a future Xerces release.

> At this point thought it looks like 4 or 5 small changes.   
>
> " I can imagine that many re-parse the XML for every incoming request...
> I would be certainly cautious if I going to share *anything*
> between two data-models, but that's maybe just me."
>
> Yeah we're still considering something like that.. it's basically
> pooling or some variation thereof.
>
>
> I'll copy and paste the classes out as an extension and we'll see
> how it goes.  I am also concerned about the performance impact, but
> unfortunately I have to prefer a slower app to one can't stay
> running.  =)  There's a lot of performance improvements we need to
> make in other areas first anyway.   If it fixes the problems without
> deadlocks and performance is not impacted, I'll submit some patches
> to have an extension of the dom package for thread safe mode.  I
> guess we'll just have to see what happens, this is a really lousy situation.
>
> I am going to follow up again with xerces after this is all said in
> done and see why they can't at least provide a properly synced API
> on top of the unsafe implementation. ... a simple extension to do
> what they say the caller needs to do... imagine that, it would
> actually be sensible.

BTW, maybe some of the non-DOM XML API-s + Jaxen (to add XPath
support) are thread-safe?

> Thanks again.
>
> -----Original Message-----
> From: Daniel Dekany [mailto:[email protected]] 
> Sent: Tuesday, July 19, 2011 4:17 AM
> To: Newman, John W
> Cc: FreeMarker-user ([email protected])
> Subject: Re: [FreeMarker-user] Exceptions due to freemarker's use of non-thread-safe DOM library
>
> Monday, July 18, 2011, 9:19:38 PM, Newman, John W wrote:
>
>>>"The strange thing is, if thread-safe DOM isn't important for the industry (and so it seems, if none of the implementations are thread-safe)"
>>
>> Actually it is kind of important now... I brought it up with xerces 
>> and they said it has been brought up time and time again.  Yet they 
>> have a good case for leaving the sync up to the client code
>> (freemarker) since to declare it fully thread safe would require 
>> locking at much lower levels than makes sense.  The best way to go 
>> about it is for the consumers to lock at the highest level they need 
>> to.  While this is really unfortunate, I can agree with that and as a 
>> consumer, I have put locks in where I need to.
>>
>> See
>> http://old.nabble.com/Making-Xerces-DOM-thread-safe-for-read-td1423058
>> 4.html
>>
>> " DOM Working Group recommended that you *implement threadsafety in 
>> the code which uses the DOM* rather than in the DOM itself."
>>
>>>"do any other consumers solve this somehow? Like XSLT implementations?"
>>
>> Well, they'd need to.  Some probably do, some probably don't.  
>> I'll try to dig around on that a bit, but yes, I bet they do or they'd 
>> be single thread only.
>
> So, should the DOM wrapper put sync on the DOM *Document* (the
> root) each time it accesses a node in it? Or what's less blocking way of synchronizing?
>
>> My whole point is, for you to write this line
>>
>> NodeList children = element.getChildNodes();
>>
>> would normally be just fine.  I don't think your assumptions are 
>> wrong, and I sort of agree with your stance of "well, it's really not our fault".
>>
>> But from a practical standpoint today, that line there _is broken for 
>> everyone, and arguably is incorrect.  You should either remove it, fix 
>> it, or at the very least document that 'your underlying dom 
>> implementation is probably not thread safe, and therefore using this 
>> package is not a good idea in a multithreaded application'.
>> Currently I can download freemarker.jar, throw some nodes at it (with 
>> the jdk default mind you) and get bugs out of it that don't
>> include any of my stack frames.   
>>
>>>" What others use the DOM wrapper for, I don't know... funny that this issue was never brought up."
>>
>> Yep that's what's really odd about this.  For every 10 people using 
>> the DOM, I bet only 4 of them know it's not thread safe.  Heck, I 
>> didn't know that until we built this whole thing up - I blindingly 
>> assumed it would be fine, how could reading a simple xml document not 
>> work from two threads right?
>
> I can imagine that many re-parse the XML for every incoming request...
> I would be certainly cautious if I going to share *anything*
> between two data-models, but that's maybe just me.
>
>> I am probably not the only one in
>> the world using freemarker servlet and the dom package....  So yes, it 
>> is very odd to me as well that this has never been brought up before.  
>> It's not very often the error occurs, I guess it takes a fairly 
>> 'lucky' situation to trigger it.  It's unfortunate that our app has 
>> been built up all the way using the DOM.  I can't just go rearchitect 
>> the whole thing in two weeks, but clearly we have to do something 
>> differently than we are today.
>>
>> (a) Don't reuse them, recreate them every time they are needed
>>    Too slow  .. these are larger documents, 20-100 concurrent users on 
>> the same document
>>
>> (b) If (a) is too slow, then pool them  
>>    Same thing, really would require too much ram....
>
> I wonder that if the templates are XPath-intensive what throughput
> will you get with sync-ed DOM method calls and 20-100 concurrent
> users. Surely if that will be a problem you can still start pooling the "hotspot" DOM-s...
>
>> I'll revisit the proxy solution, maybe I can get that to work 
>> somehow..  I have an idea.  If not I'm going to be hacking at your library pure and simple.
>
> And if you sign a Contributor License Agreement then you could commit that.
>
>> -John
>
> --
> Best regards,
>  Daniel Dekany
>
>
>
> ------------------------------------------------------------------------------
> Magic Quadrant for Content-Aware Data Loss Prevention
> Research study explores the data loss prevention market. Includes in-depth
> analysis on the changes within the DLP market, and the criteria used to
> evaluate the strengths and weaknesses of these DLP solutions.
> http://www.accelacomm.com/jaw/sfnl/114/51385063/
> _______________________________________________
> FreeMarker-user mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/freemarker-user
>

-- 
Best regards,
 Daniel Dekany


------------------------------------------------------------------------------
Magic Quadrant for Content-Aware Data Loss Prevention
Research study explores the data loss prevention market. Includes in-depth
analysis on the changes within the DLP market, and the criteria used to
evaluate the strengths and weaknesses of these DLP solutions.
http://www.accelacomm.com/jaw/sfnl/114/51385063/