Fop
  1. Fop
  2. FOP-1594

Synchronization fault in FontCache

    Details

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

      Description

      I have an application which renders a lot of PDFs using several threads. We had an issue recently, concerning font loading. While investigating this stack trace:

      java.lang.NullPointerException
      at org.apache.fop.fonts.FontCache.isFailedFont(FontCache.java:294)
      at org.apache.fop.fonts.autodetect.FontInfoFinder.find(FontInfoFinder.java:179)
      at org.apache.fop.render.PrintRendererConfigurator.addFontInfoListFromFileList(PrintRendererConfigurator.java:233)
      at org.apache.fop.render.PrintRendererConfigurator.buildFontListFromConfiguration(PrintRendererConfigurator.java:140)
      at org.apache.fop.render.PrintRendererConfigurator.configure(PrintRendererConfigurator.java:95)
      at org.apache.fop.render.pdf.PDFRendererConfigurator.configure(PDFRendererConfigurator.java:71)
      at org.apache.fop.render.RendererFactory.createRenderer(RendererFactory.java:187)
      at org.apache.fop.area.RenderPagesModel.<init>(RenderPagesModel.java:68)
      at org.apache.fop.area.AreaTreeHandler.setupModel(AreaTreeHandler.java:127)
      at org.apache.fop.area.AreaTreeHandler.<init>(AreaTreeHandler.java:102)
      at org.apache.fop.render.RendererFactory.createFOEventHandler(RendererFactory.java:224)
      at org.apache.fop.fo.FOTreeBuilder.<init>(FOTreeBuilder.java:100)
      at org.apache.fop.apps.Fop.createDefaultHandler(Fop.java:100)
      at org.apache.fop.apps.Fop.<init>(Fop.java:78)
      at org.apache.fop.apps.FopFactory.newFop(FopFactory.java:247)

      I have found a possible synchronization fault in FontCache method:

      == Java code ==
      public boolean isFailedFont(String embedUrl, long lastModified) {
      if (failedFontMap.containsKey(embedUrl)) {
      synchronized (changeLock) {
      long failedLastModified = ((Long)failedFontMap.get(embedUrl)).longValue();
      if (lastModified != failedLastModified)

      { // this font has been changed so lets remove it // from failed font map for now failedFontMap.remove(embedUrl); changed = true; }
      }
      return true;
      }
      return false;
      }
      == end Java code ==

      to my opinion, it shall be like this:

      == Java code ==
      public boolean isFailedFont(String embedUrl, long lastModified) {
      synchronized (changeLock) {
      if (failedFontMap.containsKey(embedUrl)) {

      long failedLastModified = ((Long)failedFontMap.get(embedUrl)).longValue();
      if (lastModified != failedLastModified) { // this font has been changed so lets remove it // from failed font map for now failedFontMap.remove(embedUrl); changed = true; }

      return true;
      }
      return false;
      }
      }
      == end Java code ==

      1. bugzilla46211.diff
        3 kB
        Andreas L. Delmelle

        Activity

        Hide
        Andreas L. Delmelle added a comment -

        I think the gist is correct. Have you tried changing it? Does it resolve the issue if you do?
        Reason I'm asking is that there seems to be another problem: changeLock is not a 'final' variable, nor is it declared 'volatile'.
        As a consequence:
        a) since it is neither final nor volatile, it is not guaranteed to be properly initialized (some threads may see 'null' instead of the Object instance)
        b) since it is not final, it is theoretically possible to re-assign the changeLock member to a different instance, which would lead to unpredictable behavior. It is possible for two threads to enter the synchronized block, since they have each locked a separate instance.

        Show
        Andreas L. Delmelle added a comment - I think the gist is correct. Have you tried changing it? Does it resolve the issue if you do? Reason I'm asking is that there seems to be another problem: changeLock is not a 'final' variable, nor is it declared 'volatile'. As a consequence: a) since it is neither final nor volatile, it is not guaranteed to be properly initialized (some threads may see 'null' instead of the Object instance) b) since it is not final, it is theoretically possible to re-assign the changeLock member to a different instance, which would lead to unpredictable behavior. It is possible for two threads to enter the synchronized block, since they have each locked a separate instance.
        Hide
        ilj added a comment -

        no, i didn't tried it - but it seems quite obvious.
        and anyway - i will not be able to reproduce that easily for two reasons:

        1. it happens only when two threads have failed to load the font properly, which is seldom enough by itself. and this got to happen to those threads in this special order - one thread faile, tried to call isFailedFont, but got outrun by another thread which grabs the changeLock ...

        2. the font loading issue, which causeв this bug in my case was quickly fixed. and i really don't want to break that again

        so, i suggest changing the "if" and "synchronized" order as in my previous comment AND making changeLock final and initializing it along with that.

        Show
        ilj added a comment - no, i didn't tried it - but it seems quite obvious. and anyway - i will not be able to reproduce that easily for two reasons: 1. it happens only when two threads have failed to load the font properly, which is seldom enough by itself. and this got to happen to those threads in this special order - one thread faile, tried to call isFailedFont, but got outrun by another thread which grabs the changeLock ... 2. the font loading issue, which causeв this bug in my case was quickly fixed. and i really don't want to break that again so, i suggest changing the "if" and "synchronized" order as in my previous comment AND making changeLock final and initializing it along with that.
        Hide
        Andreas L. Delmelle added a comment -

        (In reply to comment #2)

        > no, i didn't tried it - but it seems quite obvious.

        OK, thanks for the feedback.

        > 1. it happens only when two threads have failed to load the font properly,
        > which is seldom enough by itself. and this got to happen to those threads in
        > this special order - one thread faile, tried to call isFailedFont, but got
        > outrun by another thread which grabs the changeLock ...

        Yep. A classic example of what is known as a 'race condition'. Unless the check is moved into the synchronized block as you suggest, this is bound to lead to trouble in some exceptional cases.

        > so, i suggest changing the "if" and "synchronized" order as in my previous
        > comment AND making changeLock final and initializing it along with that.
        >

        OK, will do. Just waiting for some feedback on fop-dev@ to see if I've overlooked anything. If not, then the changes will be committed in a few days.

        Thanks for tracking this and reporting the bug!

        Show
        Andreas L. Delmelle added a comment - (In reply to comment #2) > no, i didn't tried it - but it seems quite obvious. OK, thanks for the feedback. > 1. it happens only when two threads have failed to load the font properly, > which is seldom enough by itself. and this got to happen to those threads in > this special order - one thread faile, tried to call isFailedFont, but got > outrun by another thread which grabs the changeLock ... Yep. A classic example of what is known as a 'race condition'. Unless the check is moved into the synchronized block as you suggest, this is bound to lead to trouble in some exceptional cases. > so, i suggest changing the "if" and "synchronized" order as in my previous > comment AND making changeLock final and initializing it along with that. > OK, will do. Just waiting for some feedback on fop-dev@ to see if I've overlooked anything. If not, then the changes will be committed in a few days. Thanks for tracking this and reporting the bug!
        Hide
        Andreas L. Delmelle added a comment -

        Added the proposed changes (including some other minor details, like simplification of conditionals)

        The one thing I'm not sure about: we cannot combine 'final' and 'transient' as modifiers, since this would mean that the variable would always be null, apart from the very first time the cache is instantiated. When the cache is serialized once, changeLock is not written to the stream (transient), but is also never initialized again upon deserialization... (weird that this combination is actually allowed in Java)

        In the patch, I've restricted it to 'final', since I don't really see why we would not serialize the lock together with the cache. Alternative would be to perform the assignment in yet another synchronized block (synchronized on the FontCache itself?)

        Show
        Andreas L. Delmelle added a comment - Added the proposed changes (including some other minor details, like simplification of conditionals) The one thing I'm not sure about: we cannot combine 'final' and 'transient' as modifiers, since this would mean that the variable would always be null, apart from the very first time the cache is instantiated. When the cache is serialized once, changeLock is not written to the stream (transient), but is also never initialized again upon deserialization... (weird that this combination is actually allowed in Java) In the patch, I've restricted it to 'final', since I don't really see why we would not serialize the lock together with the cache. Alternative would be to perform the assignment in yet another synchronized block (synchronized on the FontCache itself?)
        Hide
        Andreas L. Delmelle added a comment -

        Attachment bugzilla46211.diff has been added with description: Patch proposal

        Show
        Andreas L. Delmelle added a comment - Attachment bugzilla46211.diff has been added with description: Patch proposal
        Hide
        Andreas L. Delmelle added a comment -

        No further feedback received on fop-dev@, so changes committed to FOP Trunk in r718309.

        Thanks for reporting!

        Show
        Andreas L. Delmelle added a comment - No further feedback received on fop-dev@, so changes committed to FOP Trunk in r718309. Thanks for reporting!
        Hide
        Glenn Adams added a comment -

        batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed

        Show
        Glenn Adams added a comment - batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development