Discussion:
DO NOT REPLY [Bug 46962] New: Deadlock in PropertyCache class
b***@apache.org
2009-04-03 14:33:03 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

Summary: Deadlock in PropertyCache class
Product: Fop
Version: 0.95
Platform: PC
OS/Version: Linux
Status: NEW
Severity: normal
Priority: P2
Component: general
AssignedTo: fop-***@xmlgraphics.apache.org
ReportedBy: ***@devexperts.com


I have a multithreaded rendering application. Shortly after we had migrated to
some nice smooth and very fast hardware, we started getting Java deadlocks in
FOP code. Stack traces always looked like this one:

"T4-CFD_MR accId=3065" prio=10 tid=0x00002aab37aa1400 nid=0x6123 waiting for
monitor entry [0x000000004193e000..0x0000000041941ac0]
java.lang.Thread.State: BLOCKED (on object monitor)
at
org.apache.fop.fo.properties.PropertyCache.get(PropertyCache.java:204)
- waiting to lock <0x00002aaab388a3f8> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:283)
at
org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:301)
at
org.apache.fop.fo.properties.NumberProperty.getInstance(NumberProperty.java:120)
at
org.apache.fop.fo.expr.PropertyParser.parsePrimaryExpr(PropertyParser.java:262)
at
org.apache.fop.fo.expr.PropertyParser.parseUnaryExpr(PropertyParser.java:212)
at
org.apache.fop.fo.expr.PropertyParser.parseMultiplicativeExpr(PropertyParser.java:177)
at
org.apache.fop.fo.expr.PropertyParser.parseAdditiveExpr(PropertyParser.java:151)
at
org.apache.fop.fo.expr.PropertyParser.parseArgs(PropertyParser.java:378)
at
org.apache.fop.fo.expr.PropertyParser.parsePrimaryExpr(PropertyParser.java:343)
at
org.apache.fop.fo.expr.PropertyParser.parseUnaryExpr(PropertyParser.java:212)
at
org.apache.fop.fo.expr.PropertyParser.parseMultiplicativeExpr(PropertyParser.java:177)
at
org.apache.fop.fo.expr.PropertyParser.parseAdditiveExpr(PropertyParser.java:151)
at
org.apache.fop.fo.expr.PropertyParser.parseProperty(PropertyParser.java:125)

================================

"T3-CFD_MR accId=3031" prio=10 tid=0x00002aab37a68800 nid=0x6122 waiting for
monitor entry [0x000000004183c000..0x0000000041840c40]
java.lang.Thread.State: BLOCKED (on object monitor)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:226)
- waiting to lock <0x00002aaab388a650> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:228)
- locked <0x00002aaab388b200> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:228)
- locked <0x00002aaab388b228> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:228)
- locked <0x00002aaab388b250> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:228)
- locked <0x00002aaab388b278> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:228)
- locked <0x00002aaab388b2a0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:228)
- locked <0x00002aaab388b368> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:228)

================================

"T2-CFD_MR accId=2823" prio=10 tid=0x00002aab37a66c00 nid=0x6121 waiting for
monitor entry [0x000000004173c000..0x000000004173fbc0]
java.lang.Thread.State: BLOCKED (on object monitor)
at
org.apache.fop.fo.properties.PropertyCache.cleanSegment(PropertyCache.java:114)
- waiting to lock <0x00002aaab38a33b8> (a [Z)
at
org.apache.fop.fo.properties.PropertyCache.put(PropertyCache.java:176)
- locked <0x00002aaab388a650> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:287)
at
org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:301)
at
org.apache.fop.fo.properties.NumberProperty.getInstance(NumberProperty.java:120)
at
org.apache.fop.fo.expr.PropertyParser.parsePrimaryExpr(PropertyParser.java:262)
at
org.apache.fop.fo.expr.PropertyParser.parseUnaryExpr(PropertyParser.java:212)
at
org.apache.fop.fo.expr.PropertyParser.parseMultiplicativeExpr(PropertyParser.java:177)
at
org.apache.fop.fo.expr.PropertyParser.parseAdditiveExpr(PropertyParser.java:151)
at
org.apache.fop.fo.expr.PropertyParser.parseArgs(PropertyParser.java:378)
at
org.apache.fop.fo.expr.PropertyParser.parsePrimaryExpr(PropertyParser.java:343)
at
org.apache.fop.fo.expr.PropertyParser.parseUnaryExpr(PropertyParser.java:212)
at
org.apache.fop.fo.expr.PropertyParser.parseMultiplicativeExpr(PropertyParser.java:177)

================================

"T1-CFD_MR accId=3070" prio=10 tid=0x00002aab37a5a000 nid=0x6120 waiting for
monitor entry [0x000000004163c000..0x000000004163ed40]
java.lang.Thread.State: BLOCKED (on object monitor)
at
org.apache.fop.fo.properties.PropertyCache.cleanSegment(PropertyCache.java:114)
- waiting to lock <0x00002aaab38a33b8> (a [Z)
at
org.apache.fop.fo.properties.PropertyCache.put(PropertyCache.java:176)
- locked <0x00002aaab388a330> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:287)
at
org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:301)
at
org.apache.fop.fo.properties.NumberProperty.getInstance(NumberProperty.java:109)
at
org.apache.fop.fo.expr.PropertyParser.parsePrimaryExpr(PropertyParser.java:258)
at
org.apache.fop.fo.expr.PropertyParser.parseUnaryExpr(PropertyParser.java:212)
at
org.apache.fop.fo.expr.PropertyParser.parseMultiplicativeExpr(PropertyParser.java:177)
at
org.apache.fop.fo.expr.PropertyParser.parseAdditiveExpr(PropertyParser.java:151)
at
org.apache.fop.fo.expr.PropertyParser.parseProperty(PropertyParser.java:125)
at org.apache.fop.fo.expr.PropertyParser.parse(PropertyParser.java:91)
at
org.apache.fop.fo.properties.PropertyMaker.make(PropertyMaker.java:436)
at
org.apache.fop.fo.properties.CompoundPropertyMaker.make(CompoundPropertyMaker.java:207)

================================

"T0-CFD_MR accId=2852" prio=10 tid=0x00002aab37a59800 nid=0x611f waiting for
monitor entry [0x000000004153a000..0x000000004153dcc0]
java.lang.Thread.State: BLOCKED (on object monitor)
at
org.apache.fop.fo.properties.PropertyCache.cleanSegment(PropertyCache.java:114)
- waiting to lock <0x00002aaab38a33b8> (a [Z)
at
org.apache.fop.fo.properties.PropertyCache.put(PropertyCache.java:176)
- locked <0x00002aaab388a3f8> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:287)
at
org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:301)
at
org.apache.fop.fo.properties.NumberProperty.getInstance(NumberProperty.java:120)
at
org.apache.fop.fo.expr.PropertyParser.parsePrimaryExpr(PropertyParser.java:262)
at
org.apache.fop.fo.expr.PropertyParser.parseUnaryExpr(PropertyParser.java:212)
at
org.apache.fop.fo.expr.PropertyParser.parseMultiplicativeExpr(PropertyParser.java:177)
at
org.apache.fop.fo.expr.PropertyParser.parseAdditiveExpr(PropertyParser.java:151)
at
org.apache.fop.fo.expr.PropertyParser.parseArgs(PropertyParser.java:378)
at
org.apache.fop.fo.expr.PropertyParser.parsePrimaryExpr(PropertyParser.java:343)
at
org.apache.fop.fo.expr.PropertyParser.parseUnaryExpr(PropertyParser.java:212)
at
org.apache.fop.fo.expr.PropertyParser.parseMultiplicativeExpr(PropertyParser.java:177)

================================
END OF STACKS


with some combination of get(), put(), cleanSegment() and rehash() methods.

Can you provide any kind of workaround for this that can be done quickly ?? We
are in production and suffer from these nasty deadlocks badly, since this
application is mostly launched by schedule and we can't monitor it all the
time. Is there something that can be done ??
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2009-04-03 18:53:03 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962





--- Comment #1 from Andreas L. Delmelle <***@apache.org> 2009-04-03 11:53:01 PST ---

Thanks for the report. We will investigate this closer ASAP.
For now, the only immediate relief would be to switch to FOP Trunk, which
allows to disable the PropertyCache via a system property
"org.apache.fop.fo.properties.use-cache". Set it to "false" to avoid caching.
The drawback is obviously that the processes will all use up more memory (the
difference can be quite significant if you have a lot of identical
property-specs on a lot of FOs).

For the rest, it would also be interesting to know more about the environment.
Which Java VM (vendor + version) are you using? If it's GNU Classpath, I'd
first try if switching to a Sun VM helps.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2009-04-03 19:20:08 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962





--- Comment #2 from Andreas L. Delmelle <***@apache.org> 2009-04-03 12:20:07 PST ---
(In reply to comment #1)
For now, the only immediate relief would be to switch to FOP Trunk, ...
The addition of the system property to disable caching of the properties
apparently never made it into 0.95, but the required modifications are pretty
straightforward:
- add a 'useCache' member to PropertyCache
- in the constructor, initialize it to reflect the value of the system property
- in the generic, private fetch() method, if useCache is "true", bypass the
entire method body, and simply return the parameter instance

Apart from that, no easy solution I'm afraid. It comes down to choosing the
lesser of two 'evils':
* either use the Trunk version, which means, strictly speaking, no guarantees
about stability, although there are people who do use it in production
environments
* or modify the sources in the 0.95 branch, which also leaves you with an
unofficial version.

Again, we'll be look into it closer soon, but these types of issues are almost
always very difficult to reproduce...
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-08-02 14:37:31 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

Alexis Giotis <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-08-02 19:37:07 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #3 from Alexis Giotis <***@gmail.com> 2011-08-02 19:37:07 UTC ---
Deadlocks in o.a.f.fo.properties.PropertyCache still occur in FOP 1.0 with
similar stacktraces (see below). By looking into the code and the stacktraces,
the deadlock occurs when the map is rehashed. In short, this is a typical case:

* Thread A invokes put() and acquires a lock on segment 1.

* Thread B invokes put() and acquires a lock on segment 2.

* Both threads determine that their segment should be cleared and invoke
cleanSegment().

* Thread A acquires first the lock on votesForRehash, determines that a rehash
is required and calls it.

* Thread B holds the lock on segment 2 and waits to acquire the lock on
votesForRehash.

* Thread A executes the rehash() method which tries to recursively acquire
locks on all segments.

* Thread A and thread B deadlock because neither will release locks that the
other wants.



Relevant stacktraces from a production server:

"Thread A" stacktrace:

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:245)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:247)

org.apache.fop.fo.properties.PropertyCache.cleanSegment(PropertyCache.java:151)
org.apache.fop.fo.properties.PropertyCache.put(PropertyCache.java:195)

org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:317)

org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:331)

org.apache.fop.fo.properties.CondLengthProperty.getCondLength(CondLengthProperty.java:161)

org.apache.fop.fo.properties.CommonBorderPaddingBackground.initBorderInfo(CommonBorderPaddingBackground.java:400)

org.apache.fop.fo.properties.CommonBorderPaddingBackground.<init>(CommonBorderPaddingBackground.java:316)

org.apache.fop.fo.properties.CommonBorderPaddingBackground.getInstance(CommonBorderPaddingBackground.java:350)

org.apache.fop.fo.PropertyList.getBorderPaddingBackgroundProps(PropertyList.java:576)
org.apache.fop.fo.flow.table.TableCell.bind(TableCell.java:77)
org.apache.fop.fo.FObj.processNode(FObj.java:123)
org.apache.fop.fo.flow.table.TableFObj.processNode(TableFObj.java:233)

org.apache.fop.fo.FOTreeBuilder$MainFOHandler.startElement(FOTreeBuilder.java:282)
org.apache.fop.fo.FOTreeBuilder.startElement(FOTreeBuilder.java:171)
org.xml.sax.helpers.XMLFilterImpl.startElement(XMLFilterImpl.java:527)
org.xml.sax.helpers.XMLFilterImpl.startElement(XMLFilterImpl.java:527)

com.idocs.export2.filters.IgnoreThisSectionFilter.startElement(IgnoreThisSectionFilter.java:33)

org.apache.xml.serializer.ToXMLSAXHandler.closeStartTag(ToXMLSAXHandler.java:206)

org.apache.xml.serializer.ToSAXHandler.flushPending(ToSAXHandler.java:279)

org.apache.xml.serializer.ToXMLSAXHandler.startPrefixMapping(ToXMLSAXHandler.java:350)

org.apache.xml.serializer.ToXMLSAXHandler.startPrefixMapping(ToXMLSAXHandler.java:320)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1317)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)
org.apache.xalan.templates.ElemTemplate.execute(ElemTemplate.java:394)

org.apache.xalan.templates.ElemCallTemplate.execute(ElemCallTemplate.java:248)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.transformer.TransformerImpl.applyTemplateToNode(TransformerImpl.java:2270)

org.apache.xalan.transformer.TransformerImpl.transformNode(TransformerImpl.java:1356)

org.apache.xalan.transformer.TransformerImpl.run(TransformerImpl.java:3447)

org.apache.xalan.transformer.TransformerHandlerImpl.endDocument(TransformerHandlerImpl.java:408)
org.xml.sax.helpers.XMLFilterImpl.endDocument(XMLFilterImpl.java:473)
org.xml.sax.helpers.XMLFilterImpl.endDocument(XMLFilterImpl.java:473)

com.idocs.export2.filters.SplitElementsXMLFilter.endDocument(SplitElementsXMLFilter.java:59)
org.xml.sax.helpers.XMLFilterImpl.endDocument(XMLFilterImpl.java:473)
org.xml.sax.helpers.XMLFilterImpl.endDocument(XMLFilterImpl.java:473)

com.idocs.export2.filters.SectionSplittingXMLFilter.endSection(SectionSplittingXMLFilter.java:218)

com.idocs.export2.filters.SectionSplittingXMLFilter.startNewSection(SectionSplittingXMLFilter.java:195)

com.idocs.export2.filters.SectionSplittingXMLFilter.findMatchingXslt(SectionSplittingXMLFilter.java:166)

com.idocs.export2.filters.SectionSplittingXMLFilter.startElement(SectionSplittingXMLFilter.java:106)
org.xml.sax.helpers.XMLFilterImpl.startElement(XMLFilterImpl.java:527)

com.idocs.export2.filters.XPathMatchingFilter.startElement(XPathMatchingFilter.java:70)
org.apache.xerces.parsers.AbstractSAXParser.startElement(Unknown
Source)

org.apache.xerces.impl.XMLNSDocumentScannerImpl.scanStartElement(Unknown
Source)

org.apache.xerces.impl.XMLDocumentFragmentScannerImpl$FragmentContentDispatcher.dispatch(Unknown
Source)

org.apache.xerces.impl.XMLDocumentFragmentScannerImpl.scanDocument(Unknown
Source)
org.apache.xerces.parsers.XML11Configuration.parse(Unknown Source)
org.apache.xerces.parsers.XML11Configuration.parse(Unknown Source)
org.apache.xerces.parsers.XMLParser.parse(Unknown Source)
org.apache.xerces.parsers.AbstractSAXParser.parse(Unknown Source)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)

org.apache.xalan.transformer.TransformerIdentityImpl.transform(TransformerIdentityImpl.java:485)

com.idocs.export2.filters.FromXmlFilterChain.transformSaxSource(FromXmlFilterChain.java:224)

com.idocs.export2.filters.FromXmlFilterChain.transformToYarEntries(FromXmlFilterChain.java:233)

com.idocs.export2.filters.FromXmlFilterChain.tranform(FromXmlFilterChain.java:93)

com.idocs.export2.SectionsWorker$1.writeContent(SectionsWorker.java:208)

com.idocs.base.document.DirectContentWriter.writeContent(DirectContentWriter.java:96)

com.idocs.base.connection.docrep2.Docrep2Connection.writeContent(Docrep2Connection.java:476)

com.idocs.base.connection.docrep2.Docrep2Connection.insertDocument(Docrep2Connection.java:413)

com.idocs.base.connection.docrep2.Docrep2Connection.insert(Docrep2Connection.java:367)
com.idocs.base.connection.Session.insert(Session.java:539)
com.idocs.export2.SectionsWorker.writeOutputTo(SectionsWorker.java:218)
com.idocs.export2.SectionsWorker.transform(SectionsWorker.java:162)
com.idocs.export2.SectionsWorker.work(SectionsWorker.java:118)

com.idocs.base.worker.WorkerWrapper.invokeInContext(WorkerWrapper.java:87)
com.idocs.base.worker.WorkerWrapper.invoke(WorkerWrapper.java:68)

com.idocs.base.cluster.node.TaskExecutor$WorkerRunnable.executeTask(TaskExecutor.java:289)

com.idocs.base.cluster.node.TaskExecutor$WorkerRunnable.run(TaskExecutor.java:206)
java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441)
java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
java.util.concurrent.FutureTask.run(FutureTask.java:138)

java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)

java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
java.lang.Thread.run(Thread.java:619)






"Thread B" stacktrace:


org.apache.fop.fo.properties.PropertyCache.cleanSegment(PropertyCache.java:135)
org.apache.fop.fo.properties.PropertyCache.put(PropertyCache.java:195)

org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:317)

org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:331)

org.apache.fop.fo.properties.CondLengthProperty.getCondLength(CondLengthProperty.java:161)

org.apache.fop.fo.properties.CommonBorderPaddingBackground.initBorderInfo(CommonBorderPaddingBackground.java:400)

org.apache.fop.fo.properties.CommonBorderPaddingBackground.<init>(CommonBorderPaddingBackground.java:321)

org.apache.fop.fo.properties.CommonBorderPaddingBackground.getInstance(CommonBorderPaddingBackground.java:350)

org.apache.fop.fo.PropertyList.getBorderPaddingBackgroundProps(PropertyList.java:576)
org.apache.fop.fo.flow.table.TableCell.bind(TableCell.java:77)
org.apache.fop.fo.FObj.processNode(FObj.java:123)
org.apache.fop.fo.flow.table.TableFObj.processNode(TableFObj.java:233)

org.apache.fop.fo.FOTreeBuilder$MainFOHandler.startElement(FOTreeBuilder.java:282)
org.apache.fop.fo.FOTreeBuilder.startElement(FOTreeBuilder.java:171)
org.xml.sax.helpers.XMLFilterImpl.startElement(XMLFilterImpl.java:527)
org.xml.sax.helpers.XMLFilterImpl.startElement(XMLFilterImpl.java:527)

com.idocs.export2.filters.IgnoreThisSectionFilter.startElement(IgnoreThisSectionFilter.java:33)

org.apache.xml.serializer.ToXMLSAXHandler.closeStartTag(ToXMLSAXHandler.java:206)

org.apache.xml.serializer.ToSAXHandler.flushPending(ToSAXHandler.java:279)

org.apache.xml.serializer.ToXMLSAXHandler.startPrefixMapping(ToXMLSAXHandler.java:350)

org.apache.xml.serializer.ToXMLSAXHandler.startPrefixMapping(ToXMLSAXHandler.java:320)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1317)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.templates.ElemApplyTemplates.transformSelectedNodes(ElemApplyTemplates.java:395)

org.apache.xalan.templates.ElemApplyTemplates.execute(ElemApplyTemplates.java:178)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.templates.ElemLiteralResult.execute(ElemLiteralResult.java:1376)

org.apache.xalan.transformer.TransformerImpl.executeChildTemplates(TransformerImpl.java:2400)

org.apache.xalan.transformer.TransformerImpl.applyTemplateToNode(TransformerImpl.java:2270)

org.apache.xalan.transformer.TransformerImpl.transformNode(TransformerImpl.java:1356)

org.apache.xalan.transformer.TransformerImpl.run(TransformerImpl.java:3447)

org.apache.xalan.transformer.TransformerHandlerImpl.endDocument(TransformerHandlerImpl.java:408)
org.xml.sax.helpers.XMLFilterImpl.endDocument(XMLFilterImpl.java:473)
org.xml.sax.helpers.XMLFilterImpl.endDocument(XMLFilterImpl.java:473)

com.idocs.export2.filters.SplitElementsXMLFilter.endDocument(SplitElementsXMLFilter.java:59)
org.xml.sax.helpers.XMLFilterImpl.endDocument(XMLFilterImpl.java:473)
org.xml.sax.helpers.XMLFilterImpl.endDocument(XMLFilterImpl.java:473)

com.idocs.export2.filters.SectionSplittingXMLFilter.endSection(SectionSplittingXMLFilter.java:218)

com.idocs.export2.filters.SectionSplittingXMLFilter.startNewSection(SectionSplittingXMLFilter.java:195)

com.idocs.export2.filters.SectionSplittingXMLFilter.findMatchingXslt(SectionSplittingXMLFilter.java:166)

com.idocs.export2.filters.SectionSplittingXMLFilter.startElement(SectionSplittingXMLFilter.java:106)
org.xml.sax.helpers.XMLFilterImpl.startElement(XMLFilterImpl.java:527)

com.idocs.export2.filters.XPathMatchingFilter.startElement(XPathMatchingFilter.java:70)
org.apache.xerces.parsers.AbstractSAXParser.startElement(Unknown
Source)

org.apache.xerces.impl.XMLNSDocumentScannerImpl.scanStartElement(Unknown
Source)

org.apache.xerces.impl.XMLDocumentFragmentScannerImpl$FragmentContentDispatcher.dispatch(Unknown
Source)

org.apache.xerces.impl.XMLDocumentFragmentScannerImpl.scanDocument(Unknown
Source)
org.apache.xerces.parsers.XML11Configuration.parse(Unknown Source)
org.apache.xerces.parsers.XML11Configuration.parse(Unknown Source)
org.apache.xerces.parsers.XMLParser.parse(Unknown Source)
org.apache.xerces.parsers.AbstractSAXParser.parse(Unknown Source)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)
org.xml.sax.helpers.XMLFilterImpl.parse(XMLFilterImpl.java:333)

org.apache.xalan.transformer.TransformerIdentityImpl.transform(TransformerIdentityImpl.java:485)

com.idocs.export2.filters.FromXmlFilterChain.transformSaxSource(FromXmlFilterChain.java:224)

com.idocs.export2.filters.FromXmlFilterChain.transformToYarEntries(FromXmlFilterChain.java:233)

com.idocs.export2.filters.FromXmlFilterChain.tranform(FromXmlFilterChain.java:93)

com.idocs.export2.SectionsWorker$1.writeContent(SectionsWorker.java:208)

com.idocs.base.document.DirectContentWriter.writeContent(DirectContentWriter.java:96)

com.idocs.base.connection.docrep2.Docrep2Connection.writeContent(Docrep2Connection.java:476)

com.idocs.base.connection.docrep2.Docrep2Connection.insertDocument(Docrep2Connection.java:413)

com.idocs.base.connection.docrep2.Docrep2Connection.insert(Docrep2Connection.java:367)
com.idocs.base.connection.Session.insert(Session.java:539)
com.idocs.export2.SectionsWorker.writeOutputTo(SectionsWorker.java:218)
com.idocs.export2.SectionsWorker.transform(SectionsWorker.java:162)
com.idocs.export2.SectionsWorker.work(SectionsWorker.java:118)

com.idocs.base.worker.WorkerWrapper.invokeInContext(WorkerWrapper.java:87)
com.idocs.base.worker.WorkerWrapper.invoke(WorkerWrapper.java:68)

com.idocs.base.cluster.node.TaskExecutor$WorkerRunnable.executeTask(TaskExecutor.java:289)

com.idocs.base.cluster.node.TaskExecutor$WorkerRunnable.run(TaskExecutor.java:206)
java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441)
java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
java.util.concurrent.FutureTask.run(FutureTask.java:138)

java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)

java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
java.lang.Thread.run(Thread.java:619)






Stacktraces from a test case that reproduces this issue:

"Thread A":
"pool-1-thread-2" prio=5 tid=7fe2831a7000 nid=0x11670d000 waiting for monitor
entry [11670b000]
java.lang.Thread.State: BLOCKED (on object monitor)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:252)
- waiting to lock <7f333b970> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b960> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b950> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b940> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b930> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b920> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b910> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b900> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b8f0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b8e0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b8d0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b8c0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b8b0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b8a0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b890> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b880> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b870> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b860> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b850> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b840> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b830> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b820> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b810> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b800> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b7f0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b7e0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b7d0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.rehash(PropertyCache.java:254)
- locked <7f333b7c0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at
org.apache.fop.fo.properties.PropertyCache.cleanSegment(PropertyCache.java:156)
- locked <7f333b630> (a [Z)
at org.apache.fop.fo.properties.PropertyCache.put(PropertyCache.java:200)
- locked <7f333b8c0> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:332)
at org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:346)
at
org.apache.fop.fo.properties.PropertyCacheTest.fillCache(PropertyCacheTest.java:37)
at
org.apache.fop.fo.properties.PropertyCacheTest.access$0(PropertyCacheTest.java:33)
at
org.apache.fop.fo.properties.PropertyCacheTest$1.call(PropertyCacheTest.java:22)
at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
at java.util.concurrent.FutureTask.run(FutureTask.java:138)
at
java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
at
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
at java.lang.Thread.run(Thread.java:680)


"Thread B":
"pool-1-thread-1" prio=5 tid=7fe282236000 nid=0x11660a000 waiting for monitor
entry [116609000]
java.lang.Thread.State: BLOCKED (on object monitor)
at
org.apache.fop.fo.properties.PropertyCache.cleanSegment(PropertyCache.java:140)
- waiting to lock <7f333b630> (a [Z)
at org.apache.fop.fo.properties.PropertyCache.put(PropertyCache.java:200)
- locked <7f333b970> (a
org.apache.fop.fo.properties.PropertyCache$CacheSegment)
at org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:332)
at org.apache.fop.fo.properties.PropertyCache.fetch(PropertyCache.java:346)
at
org.apache.fop.fo.properties.PropertyCacheTest.fillCache(PropertyCacheTest.java:37)
at
org.apache.fop.fo.properties.PropertyCacheTest.access$0(PropertyCacheTest.java:33)
at
org.apache.fop.fo.properties.PropertyCacheTest$1.call(PropertyCacheTest.java:22)
at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
at java.util.concurrent.FutureTask.run(FutureTask.java:138)
at
java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
at
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
at java.lang.Thread.run(Thread.java:680)
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-08-02 19:45:47 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #4 from Alexis Giotis <***@gmail.com> 2011-08-02 19:45:47 UTC ---
Created attachment 27342
--> https://issues.apache.org/bugzilla/attachment.cgi?id=27342
Patch with a unit test to reproduce the deadlock

Attached is a patch that includes a unit test (PropertyCacheTest.java) that
almost always reproduces the deadlock using just 2 threads.

This is not meant to be committed but only as a demonstration of the problem
and as a test of next patches that fix it. Except adding the test case, I also
needed to add a sleep of 1 second in the rehash() method and implement the
equals() and hashcode() of the org.apache.fop.fo.properties.Property class. The
Property class was just more easy to instantiate.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-08-02 22:24:44 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #5 from Alexis Giotis <***@gmail.com> 2011-08-02 22:24:44 UTC ---
Created attachment 27343
--> https://issues.apache.org/bugzilla/attachment.cgi?id=27343
Patch fixing PropertyCache issue.

Attached is a proposed patch that fixes this issue with minimal changes. The
patch is against the revision 1067783 of PropertyCache in FOP trunk.

On the other hand, it might better to base the implementation of the
PropertyCache on ConcurrentHashMap.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-08-05 20:21:52 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #6 from Alexis Giotis <***@gmail.com> 2011-08-05 20:21:52 UTC ---
Created attachment 27357
--> https://issues.apache.org/bugzilla/attachment.cgi?id=27357
Concurrent map based implementation of property cache supporting hashCode
collisions
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-08-05 20:31:09 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

Alexis Giotis <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #27342|0 |1
is obsolete| |
Attachment #27343|0 |1
is obsolete| |
Attachment #27357|0 |1
is obsolete| |

--- Comment #7 from Alexis Giotis <***@gmail.com> 2011-08-05 20:31:09 UTC ---
Created attachment 27358
--> https://issues.apache.org/bugzilla/attachment.cgi?id=27358
Patch with rewritten PropertyCache (trunk revision 1154337)

In short, the new PropertyCache implementation attached is:
- Up to 3 times faster (depending on the tests, the -Xmx, and the retention or
not of strong refs to he cached entries)
- 3 times less lines of code
- Obviously thread-safe
- Written using JDK5 generics
- Has similar memory requirements

Additionally this patch fixes broken hashCode() and equals() methods of classes
extending Property (including the patch in bug 51625). Those caused in tests
many hashCode collisions.



In more detail, I wrote 2 new implementations of PropertyCache (one in this
patch and the one in attachment 27357) and tested both against the original one
with the fix contained in attachment 27343.

When strong references to the cached entries are kept, then the performance of
all is similar. When they are not (more common case), the ones based on the
concurrent hash map are up to 3 times faster. The tests were allocating 1M
(million) Property instances from which 100K were equal (but different
instances).

The first implementation based on the concurrent map supports caching not-equal
objects with the same hashcode but is fairly complex. The one attached in the
final patch does not. After some experimentation and tests with large (1000
page) documents hashcode collisions were caused due to buggy hashCode and equal
implementations. Handling this case has a performance penalty. In a test that
caches 1M entries from which there are only 100 different hashCodes the time to
complete was:

52 seconds for the initial implementation
12 seconds the the concurrent map that can cache not-equal objects with the
same hashcode.
1 second for the concurrent map that keeps the more recent one.

In other words, in this case (which is due to buggy hashcode()/equals), the
cost of creating a new instance and replacing the previously cached one is by
far smaller that the one to maintain in memory the different instances.

Note that this implementation does not provide any guarantee related to the
uniqueness of equal instances when concurrently executed. Such a guarantee is
not only complex to code but also it requires additional locking. In practice,
it is not very probable that this will happen, finally there will be only one
and of course this should be a tolerable situation. After all, the caching can
be globally disabled with the same system property as before.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-09-09 10:59:44 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

Vincent Hennebert <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Summary|Deadlock in PropertyCache |[PATCH] Deadlock in
|class |PropertyCache class
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-09-09 16:25:09 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

Alexis Giotis <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #27358|0 |1
is obsolete| |

--- Comment #8 from Alexis Giotis <***@gmail.com> 2011-09-09 16:25:09 UTC ---
Created attachment 27477
--> https://issues.apache.org/bugzilla/attachment.cgi?id=27477
Updated patch with rewritten PropertyCache that fixes more hashCode/equals()
problems
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-09-14 15:22:44 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

Mehdi Houshmand <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #27477|0 |1
is obsolete| |

--- Comment #9 from Mehdi Houshmand <***@gmail.com> 2011-09-14 15:22:44 UTC ---
Created attachment 27495
--> https://issues.apache.org/bugzilla/attachment.cgi?id=27495
propery cache patch

I've made a few changes to this patch a little:
- Created a static method Property.eq() which tests for object equality (and
reference equality for performance reasons)
- Fixed some checkstyle issues, these were not specific to this patch but
rather fixed some issues as I went along
- Unit tests have been added

Some of the tests added require Mockito, I tried to avoid using mocking as much
as possible for the obvious reason that the commiters haven't agreed to add it
as a dependency. However, some of these classes were are a nightmare to test
without mocking them.

This patch makes the previous work obsolete, I don't know what the etiquette is
when making someone else's work obsolete. I mean no offence.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-09-15 12:09:38 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #10 from Alexis Giotis <***@gmail.com> 2011-09-15 12:09:38 UTC ---
Medhi, thanks for checking & fixing checkstyle issues. Using the Property.eq()
definitely makes the equals() methods easier to read. I used the eclipse auto
generated equals() methods to avoid making any mistake because I did not add
the unit tests that you did.

The only line I would delete is a
// TODO Auto-generated method stub
that is found in NCnamePropertyTestCase.java


I guess the issues related to the usage of Mockito and the etiquette for
replacing older work, are not for me to comment. I can only say that I checked
your changes over my patch and I find them very good. I have also applied my
previous patch on production and it works fine.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-09-15 13:01:18 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #11 from Mehdi Houshmand <***@gmail.com> 2011-09-15 13:01:18 UTC ---
Excellent, thanks for looking over it, I agree, that //TODO shouldn't have
gotten through.

Thanks again for taking the time to check through it.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-09-27 08:25:35 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #12 from Simon Pepping <***@apache.org> 2011-09-27 08:25:35 UTC ---
(In reply to comment #9)
Post by b***@apache.org
Some of the tests added require Mockito, I tried to avoid using mocking as much
as possible for the obvious reason that the commiters haven't agreed to add it
as a dependency. However, some of these classes were are a nightmare to test
without mocking them.
I have no problem with the addition of Mickito to FOP's dependencies, since it
helps writing true unit tests.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-10-24 16:59:20 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #13 from Alexis Giotis <***@gmail.com> 2011-10-24 16:59:20 UTC ---
Now that there was a vote and Mockito has been accepted, what about applying
the patch ? Is there something holding it ?
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2011-12-15 12:27:57 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #14 from Alexis Giotis <***@gmail.com> 2011-12-15 12:27:57 UTC ---
Hi Medhi,

Now that you are a commiter and mockito is used, is there a reason for not
applying this patch ?
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-02 16:21:57 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

Alexis Giotis <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #27495|0 |1
is obsolete| |

--- Comment #15 from Alexis Giotis <***@gmail.com> 2012-03-02 16:21:57 UTC ---
Created attachment 28412
--> https://issues.apache.org/bugzilla/attachment.cgi?id=28412
Patch update to fix NPE on JDK5

Added a lock on the PropertyCache that prevents concurrent cleanup of the
cached objects.

There is no reason to concurrently cleanup the cache but most importantly it
protects from an NullPointerException occuring on Sun JDK5

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6312056

This issue was reported by Vincent. The updated patch does not include the
tests added by Mehdi for the following reasons:

- Make clear my contribution.
- There were some cleanups requested by Vincent that I did not fully understand
and I don't wish to delay it's processing.
- I don't think we get added value by testing the hashCode() & equals()
implementations. The tests don't protect from future changes in the tested
classes (e.g. adding a new field) and don't protect if in the future another
class is added in the cache.
- To find broken hashCode() implementation of cached instances, the
PropertyCache counts the number of collisions and reports it in the log if it
exceeds a number.

Of course anyone is welcomed to add tests, if he wishes so. For completeness,
below is Vincent's test that reveals the JDK5 bug:

private final PropertyCache<Integer> cache = new PropertyCache<Integer>();

private class CacheFiller implements Runnable {
private final int start;
CacheFiller(int start) {
this.start = start;
}
public void run() {
for (int i = 0; i < 1000000; i++) {
cache.fetch(new Integer(start + i));
}
}
}
public void testCleanUp() throws InterruptedException {
Thread t1 = new Thread(new CacheFiller(0));
Thread t2 = new Thread(new CacheFiller(10000));
t1.start();
t2.start();
t1.join();
t2.join();
}
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-02 16:33:36 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #16 from Alex Giotis <***@gmail.com> 2012-03-02 16:33:36 UTC ---
*** Bug 51625 has been marked as a duplicate of this bug. ***
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-09 09:12:12 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #17 from Mehdi Houshmand <***@gmail.com> 2012-03-09 09:12:12 UTC ---
<snip/>
Post by b***@apache.org
- I don't think we get added value by testing the hashCode() & equals()
implementations. The tests don't protect from future changes in the tested
classes (e.g. adding a new field) and don't protect if in the future another
class is added in the cache.
While I agree that these tests don't protect from subsequent adding of class
members, I couldn't disagree more that they shouldn't be tested! The equals()
and hashCode() methods have a contract to Object that they should behave in a
certain fashion. If that behaviour is modified, bugs can be hard to track down
and difficult to diagnose because they're so widely used in Java collections
library.
Post by b***@apache.org
- To find broken hashCode() implementation of cached instances, the
PropertyCache counts the number of collisions and reports it in the log if it
exceeds a number.
Of course anyone is welcomed to add tests, if he wishes so. For completeness,
private final PropertyCache<Integer> cache = new PropertyCache<Integer>();
private class CacheFiller implements Runnable {
private final int start;
CacheFiller(int start) {
this.start = start;
}
public void run() {
for (int i = 0; i < 1000000; i++) {
cache.fetch(new Integer(start + i));
}
}
}
public void testCleanUp() throws InterruptedException {
Thread t1 = new Thread(new CacheFiller(0));
Thread t2 = new Thread(new CacheFiller(10000));
t1.start();
t2.start();
t1.join();
t2.join();
}
Why not just put that in a unit test?? None of this code is tested and a little
mistake could have far-reaching ramifications.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-09 09:15:10 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #18 from Mehdi Houshmand <***@gmail.com> 2012-03-09 09:15:10 UTC ---
<snip>
Post by b***@apache.org
Why not just put that in a unit test?? None of this code is tested and a little
mistake could have far-reaching ramifications.
By that I obviously meant the PropertyCache version of this test, however you
tested your latest patch in development...
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-09 16:04:06 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

Alex Giotis <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #28412|0 |1
is obsolete| |

--- Comment #19 from Alex Giotis <***@gmail.com> 2012-03-09 16:04:06 UTC ---
Created attachment 28447
--> https://issues.apache.org/bugzilla/attachment.cgi?id=28447
Patch update including a PropertyCacheTestCase

* Updated patch against revision 1298724 of trunk
* Resolved patch merge conflicts introduced by recent commits.
* Updated code against the latest checkstyle 5.5
* Added a PropertyCacheTestCase
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-21 20:41:37 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #20 from Vincent Hennebert <***@gmail.com> 2012-03-21 20:41:37 UTC ---
Thanks for your patch! I have a few questions following a quick review:
* Why declare the equals and hashCode methods on interfaces (Numeric,
PercentBase)? They are defined on Object anyway, and AFAICT declaring them on
interfaces wouldn't change anything (that is, force the developer to implement
them on sub-classes?).
* Why define those methods as abstract on Property? What if a sub-class is
perfectly happy with the default implementations from Object? Also, it doesn't
allow to call super.hashCode or super.equals any more. I'm not sure at all if
this is desirable.
* Why use Double.doubleToLongBits in equals methods to compare doubles
(NumericProperty, PercentLength)? Why not just thisDouble == otherDouble? (One
could also argue that some epsilon might be necessary, but this is another
topic.)
* shouldn't the specVal field from Property also be tested for equality?

Thanks,
Vincent
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-22 02:02:02 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #21 from Alex Giotis <***@gmail.com> 2012-03-22 02:02:02 UTC ---
Vincent, thanks for looking at it.
Post by b***@apache.org
* Why declare the equals and hashCode methods on interfaces (Numeric,
PercentBase)? They are defined on Object anyway, and AFAICT declaring them on
interfaces wouldn't change anything (that is, force the developer to implement
them on sub-classes?).
The definitions of hashcode/equals on the interfaces are not needed. It was
helping
me easily locate which classes implement them. Can be removed.
Post by b***@apache.org
* Why define those methods as abstract on Property? What if a sub-class is
perfectly happy with the default implementations from Object? Also, it doesn't
allow to call super.hashCode or super.equals any more. I'm not sure at all if
this is desirable.
This was for forcing future sub-classes to implement them as it is not obvious
when an implementation is needed. There are properties like ListProperty
(a list of Property instances) and classes like the CompoundPropertyMaker which
use it. For example, the FontFamilyProperty extends ListProperty and uses the
cache. If we rely on the implementations from Object, then the caching is
simply
an overhead as the FontFamilyProperty#make method will never create two
properties with the same identity (Object#equals uses the == operator).

Having said that, if this is not desirable, the abstract methods on Property
can
be removed.
Post by b***@apache.org
* Why use Double.doubleToLongBits in equals methods to compare doubles
(NumericProperty, PercentLength)? Why not just thisDouble == otherDouble? (One
could also argue that some epsilon might be necessary, but this is another
topic.)
Well, if we forget about the epsilon, the doubleToLongBits is the correct way
to check for equality two doubles. This is well explained in Effective Java by
Joshua Bloch, used in Double#compare, Double#equals and by all IDEs that
generate them and libraries like Apache commons-lang EqualsBuilder. Java
has double values for both 0.0 and -0.0, as well as the never equal
"not a number" (NaN). Those can't be checked with ==.
Have a look into the doubleToLongBits source and at the javadoc for
Double.equals().
Post by b***@apache.org
* shouldn't the specVal field from Property also be tested for equality?
It seems that currently this is not needed. The specVal is set by 3 properties,
the FontShorthandProperty, the LineHeightProperty and the URIProperty.
None of them uses the cache, so currently it is not needed in the equality
tests.


Generally, it should be safe to demand from every class using the cache to
correctly
implement the hashCode/equal methods. Unfortunately, we can not in general
enforce it. With that view, it could be better to remove the abstract
declarations on
Property.



Please tell me if you expect an updated patch.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-22 02:08:31 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #22 from Alex Giotis <***@gmail.com> 2012-03-22 02:08:31 UTC ---
This patch should also resolve Bug 50703.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-22 16:54:42 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #23 from Vincent Hennebert <***@gmail.com> 2012-03-22 16:54:42 UTC ---
Hi Alexios,

(In reply to comment #21)
Post by b***@apache.org
Vincent, thanks for looking at it.
Post by b***@apache.org
* Why use Double.doubleToLongBits in equals methods to compare doubles
(NumericProperty, PercentLength)? Why not just thisDouble == otherDouble? (One
could also argue that some epsilon might be necessary, but this is another
topic.)
Well, if we forget about the epsilon, the doubleToLongBits is the correct way
to check for equality two doubles. This is well explained in Effective Java by
Joshua Bloch, used in Double#compare, Double#equals and by all IDEs that
generate them and libraries like Apache commons-lang EqualsBuilder. Java
has double values for both 0.0 and -0.0, as well as the never equal
"not a number" (NaN). Those can't be checked with ==.
Have a look into the doubleToLongBits source and at the javadoc for
Double.equals().
Learning new things every day :-) Thanks for the pointers.
Post by b***@apache.org
Post by b***@apache.org
* shouldn't the specVal field from Property also be tested for equality?
It seems that currently this is not needed. The specVal is set by 3 properties,
the FontShorthandProperty, the LineHeightProperty and the URIProperty.
None of them uses the cache, so currently it is not needed in the equality
tests.
Ok, make sense. Still I'll modify the implementations of equals and hashCode in
the URIProperty class as it makes explicit use of specVal.
Post by b***@apache.org
Please tell me if you expect an updated patch.
It's not necessary.


Thanks,
Vincent
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-22 18:00:46 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #24 from Vincent Hennebert <***@gmail.com> 2012-03-22 18:00:46 UTC ---
Patch applied in rev. 1303891:
http://svn.apache.org/viewvc?rev=1303891&view=rev

Sorry for the delay about this, and thanks for your patience.

I didn't include the test cases. They still require quite some work, which is
more than I can allocate on this. But most of all, I'm not quite sure that they
represent the right approach to the problem. They use a hell lot of mocking
which makes them hard to understand, let alone maintain.

And despite that, they probably don't even bring adequate coverage. Many fields
are set to a mock of some property (see e.g. EnumLengthTestCase). So the equals
method will compare two physically identical instances, which is definitely a
narrow use case. We should also test the cases of properties made of physically
different but logically identical fields.

Likewise, the tests for not equals should be broader and test different
combinations of field values.

Implementing proper coverage would require even more mocking, and things will
start to be really unwieldy.

I'm not sure what's the right approach to this. Maybe some helper library like
EqualsVerifier?
http://code.google.com/p/equalsverifier/

Vincent
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-22 19:20:08 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #25 from Alex Giotis <***@gmail.com> 2012-03-22 19:20:08 UTC ---
Thanks for reviewing and applying this patch !

I am happy that the deadlock is gone. I had a 2nd look on the final changes and
for me this issue is now resolved. I am leaving this bug open to fix the final
declaration of CommonBorderPaddingBackground (mockito can't mock it) and in
case somebody wants to add explicit tests for the hashCode/equals methods.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-22 21:55:19 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

--- Comment #26 from Mehdi Houshmand <***@gmail.com> 2012-03-22 21:55:19 UTC ---
<snip/>
Post by b***@apache.org
And despite that, they probably don't even bring adequate coverage. Many fields
are set to a mock of some property (see e.g. EnumLengthTestCase). So the equals
method will compare two physically identical instances, which is definitely a
narrow use case. We should also test the cases of properties made of physically
different but logically identical fields.
Likewise, the tests for not equals should be broader and test different
combinations of field values.
Implementing proper coverage would require even more mocking, and things will
start to be really unwieldy.
Yup, I came to the same conclusion and did what I could in the limited time I
had... Not ideal, but better than nothing right?
Post by b***@apache.org
I'm not sure what's the right approach to this. Maybe some helper library like
EqualsVerifier?
http://code.google.com/p/equalsverifier/
I did see this and it'd be great if we used this, and it'd be awesome if we
used used a repository management system. But a) it introduces a new version of
objenesis (which may or may not cause compatibility issues) b) 2 new compile
time dependencies c) who's going to create the documentation.

Using these new libraries for testing is great, if people know how to use them
(and that's a big caveat). Take mocking for example, it's used widely enough in
the industry to assume devs either know or can find out how to use the mocking
frameworks. I'm not convinced this particular library is widely used enough and
if something broke we have to be confident ANYONE can fix the issue.

I should say, I'm not against adding these libraries, I think it's important to
take testing seriously, but we have to be aware of the implications of adding
dependencies and not just in terms of the classpath.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-23 11:30:16 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

Vincent Hennebert <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution| |FIXED

--- Comment #27 from Vincent Hennebert <***@gmail.com> 2012-03-23 11:30:16 UTC ---
I uploaded my changes to the test cases in a new Bugzilla entry, see bug
#52977.

I'm closing this one as the original problem has been fixed.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
b***@apache.org
2012-03-28 08:50:25 UTC
Permalink
https://issues.apache.org/bugzilla/show_bug.cgi?id=46962

Alex Giotis <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |CLOSED
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
Loading...