Fop
  1. Fop
  2. FOP-1646

[PATCH] Deadlock in PropertyCache class

    Details

    • Type: Bug Bug
    • Status: Closed
    • Resolution: Fixed
    • Affects Version/s: 0.95
    • Fix Version/s: None
    • Component/s: general
    • Labels:
      None
    • Environment:
      Operating System: Linux
      Platform: PC
    • External issue ID:
      46962

      Description

      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 ??

      1. testCase.patch
        4 kB
        Alexios Giotis
      2. PropertyCache.patch
        3 kB
        Alexios Giotis
      3. PropertyCacheT.java
        8 kB
        Alexios Giotis
      4. PropertyCache.patch
        40 kB
        Alexios Giotis
      5. Bug46962-rev1167130-patch.txt
        62 kB
        Alexios Giotis
      6. propertycache.patch
        113 kB
        Mehdi Houshmand
      7. bz46962-jdk5-compatible.patch
        71 kB
        Alexios Giotis
      8. bz46962-rev1298724.patch
        70 kB
        Alexios Giotis

        Issue Links

          Activity

          Hide
          Andreas L. Delmelle added a comment -

          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.

          Show
          Andreas L. Delmelle added a comment - 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.
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #1)

          Just to be complete:

          > 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...

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #1) Just to be complete: > 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...
          Hide
          Alexios Giotis added a comment -

          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)
          Show
          Alexios Giotis added a comment - 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)
          Hide
          Alexios Giotis added a comment -

          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.

          Show
          Alexios Giotis added a comment - 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.
          Hide
          Alexios Giotis added a comment -

          Attachment testCase.patch has been added with description: Patch with a unit test to reproduce the deadlock

          Show
          Alexios Giotis added a comment - Attachment testCase.patch has been added with description: Patch with a unit test to reproduce the deadlock
          Hide
          Alexios Giotis added a comment -

          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.

          Show
          Alexios Giotis added a comment - 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.
          Hide
          Alexios Giotis added a comment -

          Attachment PropertyCache.patch has been added with description: Patch fixing PropertyCache issue.

          Show
          Alexios Giotis added a comment - Attachment PropertyCache.patch has been added with description: Patch fixing PropertyCache issue.
          Hide
          Alexios Giotis added a comment -

          Attachment PropertyCacheT.java has been added with description: Concurrent map based implementation of property cache supporting hashCode collisions

          Show
          Alexios Giotis added a comment - Attachment PropertyCacheT.java has been added with description: Concurrent map based implementation of property cache supporting hashCode collisions
          Hide
          Alexios Giotis added a comment -

          Attachment PropertyCache.patch has been added with description: Patch with rewritten PropertyCache (trunk revision 1154337)

          Show
          Alexios Giotis added a comment - Attachment PropertyCache.patch has been added with description: Patch with rewritten PropertyCache (trunk revision 1154337)
          Hide
          Alexios Giotis added a comment -

          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 FOP-1956). 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.

          Show
          Alexios Giotis added a comment - 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 FOP-1956 ). 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.
          Hide
          Alexios Giotis added a comment -

          Attachment Bug46962-rev1167130-patch.txt has been added with description: Updated patch with rewritten PropertyCache that fixes more hashCode/equals() problems

          Show
          Alexios Giotis added a comment - Attachment Bug46962-rev1167130-patch.txt has been added with description: Updated patch with rewritten PropertyCache that fixes more hashCode/equals() problems
          Hide
          Mehdi Houshmand added a comment -

          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.

          Show
          Mehdi Houshmand added a comment - 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.
          Hide
          Mehdi Houshmand added a comment -

          Attachment propertycache.patch has been added with description: propery cache patch

          Show
          Mehdi Houshmand added a comment - Attachment propertycache.patch has been added with description: propery cache patch
          Hide
          Alexios Giotis added a comment -

          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.

          Show
          Alexios Giotis added a comment - 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.
          Hide
          Mehdi Houshmand added a comment -

          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.

          Show
          Mehdi Houshmand added a comment - 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.
          Hide
          Simon Pepping added a comment -

          (In reply to comment #9)
          > 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.

          Show
          Simon Pepping added a comment - (In reply to comment #9) > 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.
          Hide
          Alexios Giotis added a comment -

          Now that there was a vote and Mockito has been accepted, what about applying the patch ? Is there something holding it ?

          Show
          Alexios Giotis added a comment - Now that there was a vote and Mockito has been accepted, what about applying the patch ? Is there something holding it ?
          Hide
          Alexios Giotis added a comment -

          Hi Medhi,

          Now that you are a commiter and mockito is used, is there a reason for not applying this patch ?

          Show
          Alexios Giotis added a comment - Hi Medhi, Now that you are a commiter and mockito is used, is there a reason for not applying this patch ?
          Hide
          Alexios Giotis added a comment -

          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(); }
          Show
          Alexios Giotis added a comment - 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(); }
          Hide
          Alexios Giotis added a comment -

          Attachment bz46962-jdk5-compatible.patch has been added with description: Patch update to fix NPE on JDK5

          Show
          Alexios Giotis added a comment - Attachment bz46962-jdk5-compatible.patch has been added with description: Patch update to fix NPE on JDK5
          Hide
          Mehdi Houshmand added a comment -

          <snip/>

          > - 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.

          > - 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(); > }

          Why not just put that in a unit test?? None of this code is tested and a little mistake could have far-reaching ramifications.

          Show
          Mehdi Houshmand added a comment - <snip/> > - 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. > - 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(); > } Why not just put that in a unit test?? None of this code is tested and a little mistake could have far-reaching ramifications.
          Hide
          Mehdi Houshmand added a comment -

          <snip>
          > 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...

          Show
          Mehdi Houshmand added a comment - <snip> > 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...
          Hide
          Alexios Giotis added a comment -
          • 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
          Show
          Alexios Giotis added a comment - 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
          Hide
          Alexios Giotis added a comment -

          Attachment bz46962-rev1298724.patch has been added with description: Patch update including a PropertyCacheTestCase

          Show
          Alexios Giotis added a comment - Attachment bz46962-rev1298724.patch has been added with description: Patch update including a PropertyCacheTestCase
          Hide
          Vincent Hennebert added a comment -

          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

          Show
          Vincent Hennebert added a comment - 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
          Hide
          Alexios Giotis added a comment -

          Vincent, thanks for looking at it.

          > * 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.

          > * 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.

          > * 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().

          > * 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.

          Show
          Alexios Giotis added a comment - Vincent, thanks for looking at it. > * 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. > * 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. > * 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(). > * 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.
          Hide
          Alexios Giotis added a comment -

          This patch should also resolve FOP-1893.

          Show
          Alexios Giotis added a comment - This patch should also resolve FOP-1893 .
          Hide
          Vincent Hennebert added a comment -

          Hi Alexios,

          (In reply to comment #21)
          > Vincent, thanks for looking at it.
          >
          > > * 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.

          > > * 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.

          > Please tell me if you expect an updated patch.

          It's not necessary.

          Thanks,
          Vincent

          Show
          Vincent Hennebert added a comment - Hi Alexios, (In reply to comment #21) > Vincent, thanks for looking at it. > > > * 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. > > * 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. > Please tell me if you expect an updated patch. It's not necessary. Thanks, Vincent
          Hide
          Vincent Hennebert added a comment -

          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

          Show
          Vincent Hennebert added a comment - 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
          Hide
          Alexios Giotis added a comment -

          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.

          Show
          Alexios Giotis added a comment - 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.
          Hide
          Mehdi Houshmand added a comment -

          <snip/>

          > 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?

          >
          > 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.

          Show
          Mehdi Houshmand added a comment - <snip/> > 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? > > 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.
          Hide
          Vincent Hennebert added a comment -

          I uploaded my changes to the test cases in a new Bugzilla entry, see FOP-2034.

          I'm closing this one as the original problem has been fixed.

          Show
          Vincent Hennebert added a comment - I uploaded my changes to the test cases in a new Bugzilla entry, see FOP-2034 . I'm closing this one as the original problem has been fixed.

            People

            • Assignee:
              fop-dev
              Reporter:
              ilj
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development