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

>"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-td14230584.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.  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 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'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.

-John
 
-----Original Message-----
From: Daniel Dekany [mailto:[email protected]] 
Sent: Monday, July 18, 2011 2:26 PM
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, 3:54:51 PM, Newman, John W wrote:

> I've got to push back on you guys here.  There aren't _any thread safe 
> DOM implementations in java out there, so essentially your entire .dom 
> package that potentially is very useful is really useless and flat out 
> dangerous.

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), why is it suddenly so important when the DOM is consumed by FreeMarker? I mean, the DOM is always consumed by something... do any other consumers solve this somehow? Like XSLT implementations?
Other than synchronizing on the DOM root of course (that you can do in FM as well of course).

> Freemarker needs to either:
>
> A) Delete the entire dom package as it's impossible for anyone to use 
> it with any java implementations without getting corrupt documents 
> (BAD)  (unless their application has only one thread, but come on)

I have used it quite much, however that was always in single-threaded environment (like in FMPP). What others use the DOM wrapper for, I don't know... funny that this issue was never brought up.

> B) Add the appropriate locks to your code that is using the dom 
> library in an unsafe manner.  If xerces were the only
> non-thread-safe implementation, you'd be right.   But EVERY
> implementation that exists out there is not thread safe by design.

Note that the promise that FM will make them thread-safe is implied at best. At least I hope the documentation doesn't say such a thing. It's just like with any non-thread safe POJO... putting them into the data-model doesn't make them more or less thread safe. But of course, this case really need warnings everywhere.

> C) Provide your own implementation of org.w3c.dom that is somehow 
> thread safe to be used from the NodeModel classes [good luck] " The 
> correct way to fix this, assuming you want to keep using this 
> particular DOM implementation, is to create your own proxy classes 
> that implement the DOM interfaces, wrap and delegate to the underlying 
> DOM objects, synchronizing the operations on them. You can use CGLIB 
> to create these proxies really quickly. Alternatively, you can come up 
> with a wrapping/delegating+synchronizing freemarker.ext.xml.NodeModel 
> implementation too."
>
> I've done this and it doesn't work.  Wrapping the element to sync on 
> it is all well and good, but I can't sync on
> element.child.child.child.   My template is having your library call
> element.child.child.child.nodeList()  which is not thread safe. 
> Your call to nodeList() is where the sync block is supposed to be.

I think Attila meant it so that you also use proxies for the return values.

> "I don't think we'd need to introduce synchronization in our library 
> to accommodate an implementation artifact of a particular data model 
> in a completely unrelated library."
>
> I don't disagree, really this is a very unfortunate situation.  But 
> actually I think you do need to introduce syncs, since there really
> is no alternative.    If you can find me any way to safely use this
> package as it sits today from multiple threads I'd love to hear it. 
> Every implementation of DOM is not thread safe, so what do we do here?

I think you need to step back and start out from the fact that DOM-s are not thread-safe. It happens very often with other kind of objects as well, right? And if trivial sync-ing would limit throughput too much, what we always do is:

(a) Don't reuse them, recreate them every time they are needed

(b) If (a) is too slow, then pool them

> Thanks
> John
> -----Original Message-----
> From: Daniel Dekany [mailto:[email protected]]
> Sent: Friday, June 03, 2011 1:55 PM
> 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
>
> FreeMarker data-models are in general not shareable between threads.
> After all, they may contain POJO-s which aren't thread-safe (like 
> Xerces DOM trees, it seems), so it's hopeless to guarantee 
> thread-safety in FreeMarker. Since sharing data-models among threads 
> wasn't a design goal, TemplateModel-s (the things that wrap the
> POJO-s) aren't thread-safe either (unless by pure accident). Is there 
> some part in the documentation that made you believe otherwise?
>
> Still, a data-model can be thread-safe, but then it has to be built 
> carefully. Like, it seems, you would need to write a 
> SyncronizedNodeModel class or like...
>
> --
> Best regards,
>  Daniel Dekany
>
>
> Friday, June 3, 2011, 6:04:46 PM, Newman, John W wrote:
>
>> Hello,
>>  
>> In production under load, occasionally we'll get this stack trace 
>> from a template (using freemarker servlet).
>>  
>> java.lang.NullPointerException: 
>> (no message)
>> at org.apache.xerces.dom.ParentNode.nodeListItem(Unknown Source) at 
>> org.apache.xerces.dom.ParentNode.item(Unknown Source) at 
>> freemarker.ext.dom.NodeListModel.<init>(NodeListModel.java:89)
>> at freemarker.ext.dom.NodeModel.getChildNodes(NodeModel.java:302)
>> at freemarker.ext.dom.ElementModel.get(ElementModel.java:124)
>> at freemarker.core.Dot._getAsTemplateModel(Dot.java:76)
>> at freemarker.core.Expression.getAsTemplateModel(Expression.java:89)
>> at freemarker.core.IteratorBlock.accept(IteratorBlock.java:94)
>> at freemarker.core.Environment.visit(Environment.java:209)
>> [ a few dozen stack frames visit/accept/macro/ifblock from template 
>> itself ] at freemarker.core.Environment.visit(Environment.java:209)
>> at freemarker.core.Environment.process(Environment.java:189)
>> at freemarker.template.Template.process(Template.java:237)
>> at
>> freemarker.ext.servlet.FreemarkerServlet.process(FreemarkerServlet.ja
>> va:458)
>> at
>> freemarker.ext.servlet.FreemarkerServlet.doGet(FreemarkerServlet.java
>> :397) at javax.servlet.http.HttpServlet.service(HttpServlet.java:690)
>> at javax.servlet.http.HttpServlet.service(HttpServlet.java:803)
>>  
>>  
>> Our application uses XML elements that are sometimes cloned from 
>> another element.  The freemarker templates do a lot of various xml 
>> operations, ${element.@Attribute} , ${element[xpath]}, etc.  things 
>> like that that normally works perfectly fine.  In investigating the 
>> stack trace, it really looks like
>> https://issues.apache.org/jira/browse/XERCESJ-727
>>  
>> Please read through that, essentially the org.apache.xerces.dom 
>> library is deliberately not thread safe by design.  [This is news to 
>> me, a bit unexpected...]  Apparently these element implementations 
>> maintain a local cache of what has already been parsed through, and 
>> if a second thread jumps in it will end up stepping on this shared 
>> cache causing undefined behavior.  So the burden falls on the caller 
>> to synchronize on the elements before doing certain operations. (I'm not sure exactly which).
>>  
>> I've looked through the freemarker xml package a bit, and I didn't 
>> find any sync blocks.  I tried isolating the problem in there and 
>> getting a good repeatable test case, but unfortunately I don't 
>> understand your library well enough to do so.  The best test case I 
>> can provide you is attached - it's basically an adaptation of what 
>> was posted in that issue.  I think the freemarker library needs to 
>> add a few sync blocks here and there around these xml operations, but 
>> unfortunately I can't say exactly where, possibly quite a few places.  
>> And hopefully it's not one big wide block on the outer method that 
>> would kill performance.  Please look at this and let me know what you 
>> think.  I was hoping to just get a patch to send, but after several 
>> hours I'm not having much luck.  A simple pojo test case is attached.
>>  
>> Thanks,
>> John
>>  
>>  
>>  
>> John W. Newman
>> Programmer
>>  
>> 5750 Centre Avenue, Suite 500
>> Pittsburgh, PA 15206
>> Tel  412-204-0116
>> [email protected]
>> www.d3onc.com
>> Fax 412-365-0749
>>  
>> This e-mail may contain confidential information of the sending 
>> organization. Any unauthorized or improper disclosure, copying, 
>> distribution, or use of the contents of this e-mail and attached
>> document(s) is prohibited. The information contained in this e-mail 
>> and attached document(s) is intended only for the personal and 
>> confidential use of the recipient(s) named above. If you have 
>> received this communication in error, please notify the sender 
>> immediately by e-mail and delete the original e-mail and attached document(s).

--
Best regards,
 Daniel Dekany



------------------------------------------------------------------------------
Storage Efficiency Calculator
This modeling tool is based on patent-pending intellectual property that
has been used successfully in hundreds of IBM storage optimization engage-
ments, worldwide.  Store less, Store more with what you own, Move data to 
the right place. Try It Now! http://www.accelacomm.com/jaw/sfnl/114/51427378/