Let PdfFileMerger open the files to be merged itself - #1151
Conversation
Sorry, something went wrong.
Before, the files were closed before actually being processed by PdfFileMerger
Codecov Report
@@ Coverage Diff @@
## master #1151 +/- ##
==========================================
+ Coverage 61.99% 63.49% +1.49%
==========================================
Files 230 232 +2
Lines 16604 17005 +401
==========================================
+ Hits 10294 10797 +503
+ Misses 6310 6208 -102
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Sorry, something went wrong.
|
Thanks, very good analysis, and I can also reproduce the problem. Looks like this broke after the Python 3 migration. See py-pdf/pypdf#293. Letting PdfFileMerger open the files sounds good, but there is a catch. PdfFileMerger keeps the files open until the .close() method is called. If an exception is raised before that, the method won't be called and the open files will be left dangling. This may have been the reason the original code was written this way. You could use contextlib.closing to manage the PdfFileMerger object in a with statement. Note that the PyPDF2 code itself could still leak the file objects, depending on where exactly an exception is raised. But as the current implementation doesn't work at all, I think it's an acceptable tradeoff. |
Sorry, something went wrong.
|
You're right -- I'll make a new commit implementing the change you proposed. |
Sorry, something went wrong.
|
Tested and merged. Thanks! |
Sorry, something went wrong.
PrintingService currently doesn't work. With the fix mentioned in PR #1150, a file is printed, but it only contains empty pages.
It seems that PdfFileMerger is used in a way that raises unspecified behavior: The input files are opened and appended to the pdfmerger, and then closed again before pdfmerger.write(...) is called. This way, pdfmerger.write probably tries to read the appended files, but is unable to. I suppose that in previous versions of PdfFileMerger the appended files were already read into memory when append was called, and that the implementation has changed.
To fix this, we could keep the files open until calling pdfmerger.write(...). But as pdfmerger is able to open files itself, the proposed change just gives it the paths to the files and calls pdfmerger.close() in the end, so pdfmerger should close all files again.
This change is