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 ==
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.
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.
(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!
Created attachment 22875 [details] Patch proposal 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?)
No further feedback received on fop-dev@, so changes committed to FOP Trunk in r718309. Thanks for reporting!
batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed