GlenKPeterson / PdfLayoutManager

Adds line-breaking, page-breaking, tables, and styles to PDFBox
45 stars 20 forks source link

doc never closed in PdfLayoutMgr #7

Closed kbdguy closed 9 years ago

kbdguy commented 9 years ago

Thanks so much for making this project available. I was looking for a PdfBox front-end and this seems to go a long way to doing just about everything I'm looking for. Even if I don't end up using it directly, what I am learning by looking at the code is hugely speeding up my grasp of PdfBox.

One thing: I noticed when playing with pdfLayoutTest.testPdf that I was getting warnings from PdfBox about the document not getting closed. I had taken the code and just tweaked it a bit to serve the PDF from a servlet, and the warnings were output by Jetty whenever I restarted the server.

Is this a problem? If so, as an easy remedy, I think you can simply add a close method to PdfLayoutMgr that just does a doc.close(). Users of the class will need to remember to do it... just like they would if using PdfBox directly.

GlenKPeterson commented 9 years ago

Thank you for your kind words. Good catch! The best solution might be to make PdfLayoutMgr implement AutoCloseable and update the sample app/test appropriately to use the new try-with-resources syntax. Maybe save() should call close() as well. Is a PdfLayoutMgr even valid after a call to save()? I can't remember, but there is no point in making users call two methods if one method call will do. Maybe save() should be renamed close() and call doc.close() internally?

I was away for a few days, but can probably fix it this weekend, or you could submit a patch.

GlenKPeterson commented 9 years ago

OK, I added doc.close() inside the PdfLayoutMgr.save() method. The doc.close() method seems to just set a lot of things to null internally. I'm not sure that this change will make any difference, but it looks like a good idea. Let me know if this fixes the issue you were seeing: https://github.com/GlenKPeterson/PdfLayoutManager/commit/0f3cb2f973bbe9b68bf2cb3b072fcfd46a69f19b

As a side note, they updated the docs for how to deploy to Sonatype/Nexus and I think I might have deployed there correctly for the first time ever. If so, Maven should be able to auto-download the jar from Maven Central.

GlenKPeterson commented 9 years ago

I haven't heard from you, so I assume that this either resolved the issue, or did no harm. Feel free to reopen this issue if you need to.