xmlgraphics-fop-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Theresa Jayne Forster" <ther...@inbrand.co.uk>
Subject RE: Problem that could lose me my job
Date Fri, 03 Feb 2012 14:13:46 GMT
The stacktraces were added to allow me to quickly debug without trapesing
through the mega log that log4J is spewing out from the whole system (I know
its wrong hell I cringe whenever I see some of the code but it is based on
the old system just upgraded for Fop 1.0 as the old system was 0.23 with a
handful of custom addons like accepting font-weight="bold" and z-index="2"
to using a bare 1.0



Kindest regards


Theresa Forster

Senior Software Developer

From: Craig Ringer [mailto:ringerc@ringerc.id.au] 
Sent: 03 February 2012 14:00
To: fop-users@xmlgraphics.apache.org
Cc: Theresa Jayne Forster
Subject: Re: Problem that could lose me my job


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


        } catch (SAXException se) {

            LOGGER.error("Sax Exception ",se);


        } catch (ConfigurationException ce) {

            LOGGER.error("Configuration Exception ",ce);


        } catch (TransformerConfigurationException tce) {

            LOGGER.error("Transformer Configuration Exception",tce);


        } catch (TransformerException te) {

            LOGGER.error("Transformer Exception ",te);



        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

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
    // 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
            Configuration cfg = cfgBuilder.buildFromFile(new
File("/fopconfig.xml"));  // <-- You're reparsing and re-applying the config
for each file?!?
            FOUserAgent useragent = fopFactory.newFOUserAgent();
            TransformerFactory factory = TransformerFactory.newInstance();
            BufferedOutputSteam out = new BufferedOutputStream(new
            try {
                Fop fop =
                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
                // by hand with another try/catch block.
        // For the following I'd use a Java 7 multi-catch to reduce the
        // or if limited to Java <= 6 I'd assign the exception to a local
        // 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
        } 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) {
            LOGGER.error("Sax Exception ",se);
            throw new MyApplicationRuntimeException("SAX XML error while
producing PDF from XSL-FO", ioe);
        } catch (ConfigurationException ce) {
            LOGGER.error("Configuration Exception ",ce);
            throw new MyApplicationRuntimeException("Fop configuration error
while producing PDF from XSL-FO", ioe);
        } catch (TransformerConfigurationException tce) {
            LOGGER.error("Transformer Configuration Exception",tce);
            throw new MyApplicationRuntimeException("XML Tranformer
configuration error while producing PDF from XSL-FO", ioe);
        } catch (TransformerException te) {
            LOGGER.error("Transformer Exception ",te);
            throw new MyApplicationRuntimeException("XML transformer error
while producing PDF from XSL-FO", ioe);

View raw message