xmlgraphics-fop-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Craig Ringer <ring...@ringerc.id.au>
Subject Re: Problem that could lose me my job
Date Fri, 03 Feb 2012 13:59:56 GMT
On 3/02/2012 8:58 PM, Theresa Jayne Forster wrote:
>
> Sorry, I was under pressure to roll back the servers that we had just 
> upgraded to use the fop 1.0 system.
>
> One of the failing PDFs is at http://www.inbrand1.co.uk/failsinadobe.pdf
>
> The FO that generated it is at http://www.inbrand1.co.uk/failsinadobe.fo
>
"Adobe" (or even "Adobe Acrobat" or "Adobe Reader") can be a lot of 
different things. Versions are important.

That said, the PDF is clearly broken. Adobe Reader X fails with "There 
was an error opening this document. The file is damaged and could not be 
repaired". It isn't handled by Chrome either.

Re David's comment about the XSL-FO being non-confirming: If fop 
produces a broken PDF silently and without error output that is a fop 
bug, even if it's triggered by bad XSL-FO input. A warning should be 
emitted if valid PDF can be produced - though possibly not displaying 
how the author intended - but if no valid PDF can be produced that 
surely must be a fatal error. I know you're not actually saying it's 
*not* a fop bug, just being clear that IMO bad FO -> bad PDF *is* a fop bug.

However, looking at your code below, I see little reason to believe this 
is a fop bug. All you've shown so far is that your code ignores errors 
if fop does tell you about them. Have you looked at your application's 
error logs to see if fop is throwing an exception and if so, what? 
That's the only place you'll see errors since your code otherwise 
ignores them:

>         } catch (IOException ioe) {
>
>             LOGGER.error("IOException ",ioe);
>
>             ioe.printStackTrace();
>
>         } catch (SAXException se) {
>
>             LOGGER.error("Sax Exception ",se);
>
>             se.printStackTrace();
>
>         } catch (ConfigurationException ce) {
>
>             LOGGER.error("Configuration Exception ",ce);
>
>             ce.printStackTrace();
>
>         } catch (TransformerConfigurationException tce) {
>
>             LOGGER.error("Transformer Configuration Exception",tce);
>
>             tce.printStackTrace();
>
>         } catch (TransformerException te) {
>
>             LOGGER.error("Transformer Exception ",te);
>
>             te.printStackTrace();
>
>         }
>
>         return tempFile;
>
>

... that's some "interesting" error handling code. LOGGER clearly 
doesn't throw another (wrapping) exception since otherwise 
`te.printStackTrace()' would never be getting executed. That means your 
code is logging an error (two different ways) then IGNORING IT.

Not only is that error handling code rather skeezy, but you assign a 
value to tempFile early on and do not clear it when an error occurs, so 
you MAY RETURN A HALF-WRITTEN GARBAGE FILE TO THE CALLER if an error occurs.

I strongly advise you to fix your error handling code and re-test. There 
are at least a few bugs in that code:

- After some errors you may return a handle to an incompletely written 
temp file without any indication it's broken

- You don't close your file handle(s) or streams, so you're relying on 
the objects' finialisation to do it for you when the gc gets
   around to cleaning them up. This is not usually a good idea. You 
really need to be using try{}finally{} blocks to clean up
   resources like this.

Try correcting your error handling code and seeing if the problem changes.

Please also send any messages fop 1.0 logs with detailed logging enabled 
when it is producing the faulty PDF. I'm sure you've looked at the logs 
as part of basic first-step troubleshooting anyway, so you should have 
no problem bringing them to hand...

Here's a 30-second quickly hacked out version of how I'd want to tweak 
this. I haven't tested this code and don't claim I'm any expert, I'm 
just sure it's closer than what you're using right now:

     // Of course you'd use your app's real exception classes instead of 
this demo
     // scratch code....
     public class MyApplicationRuntimeException extends RuntimeException {
         public MyApplicationRuntimeException(Throwable err) { super(err); }
         public MyApplicationRuntimeException(String msg) { super(msg); }
         public MyApplicationRuntimeException(String msg, Throwable err) 
{ super(msg,err); }
     }

     public File generatePDFromFO(File foFile) {
         File tempFile = File.createTempFile("W2P", ".pdf");
         try {
             fopFactory.setStrictValidation(false);  // <--  This is a 
**HINT** you're doing something **WRONG**
             DefaultConfigurationBuilder cfgBuilder = new 
DefaultConfigurationBuilder();
             Configuration cfg = cfgBuilder.buildFromFile(new 
File("/fopconfig.xml"));  // <-- You're reparsing and re-applying the 
config for each file?!?
             fopFactory.setUserConfig(cfg);
             FOUserAgent useragent = fopFactory.newFOUserAgent();
             useragent.setTargetResolution(300);
             TransformerFactory factory = TransformerFactory.newInstance();
             BufferedOutputSteam out = new BufferedOutputStream(new 
FileOutputStream(tempFile));
             try {
                 Fop fop = 
fopFactory.newFop(MimeConstants.MIME_PDF,useragent,out);
                 Source src = new StreamSource(foFile);
                 Transformer transformer = factory.newTransformer();
                 Result res = new SAXResult(fop.getDefaultHandler());
                 transformer.transform(src, res);
                 return tempFile;
             } finally {
                 // Always close the output stream when we're done.
                 //
                 // See: Apache IOUtils. You can do the same thing more 
verbosely
                 // by hand with another try/catch block.
                 IOUtils.closeQuietly(out);
             }
         // For the following I'd use a Java 7 multi-catch to reduce the 
repetition
         // or if limited to Java <= 6 I'd assign the exception to a 
local variable
         // and fall out of the catch  in each clause, so I could use 
the same 3 lines
         // once at the end of the method to handle all error cases 
together. Up to you
         // though, that's just style preference so I've left it how you 
had it.
         } catch (IOException ioe) {
             tempFile.delete(); // Get rid of the failed tempfile if it 
was created, ignoring error
             LOGGER.error("IOException ",ioe);
             throw new MyApplicationRuntimeException("I/O error while 
producing PDF from XSL-FO", ioe);
         } catch (SAXException se) {
             tempFile.delete();
             LOGGER.error("Sax Exception ",se);
             throw new MyApplicationRuntimeException("SAX XML error 
while producing PDF from XSL-FO", ioe);
         } catch (ConfigurationException ce) {
             tempFile.delete();
             LOGGER.error("Configuration Exception ",ce);
             throw new MyApplicationRuntimeException("Fop configuration 
error while producing PDF from XSL-FO", ioe);
         } catch (TransformerConfigurationException tce) {
             tempFile.delete();
             LOGGER.error("Transformer Configuration Exception",tce);
             throw new MyApplicationRuntimeException("XML Tranformer 
configuration error while producing PDF from XSL-FO", ioe);
         } catch (TransformerException te) {
             tempFile.delete();
             LOGGER.error("Transformer Exception ",te);
             throw new MyApplicationRuntimeException("XML transformer 
error while producing PDF from XSL-FO", ioe);
         }
     }
   }

Mime
View raw message