Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let PdfFileMerger open the files to be merged itself #1151

Merged
merged 2 commits into from
Dec 26, 2020

Conversation

magula
Copy link
Contributor

@magula magula commented Mar 27, 2020

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 Reviewable

Before, the files were closed before actually being processed by PdfFileMerger
@codecov
Copy link

codecov bot commented Mar 27, 2020

Codecov Report

Merging #1151 (109ee96) into master (d4c9e92) will increase coverage by 1.49%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            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     
Flag Coverage Δ
functionaltests 47.36% <0.00%> (+1.76%) ⬆️
unittests 44.16% <0.00%> (+0.87%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cms/service/PrintingService.py 0.00% <0.00%> (ø)
cms/db/fsobject.py 75.75% <0.00%> (-4.72%) ⬇️
cms/io/PsycoGevent.py 61.11% <0.00%> (-4.61%) ⬇️
cms/grading/tasktypes/util.py 83.72% <0.00%> (-3.49%) ⬇️
cms/grading/languages/python2_cpython.py 96.66% <0.00%> (-3.34%) ⬇️
cmscontrib/DumpExporter.py 72.68% <0.00%> (-2.07%) ⬇️
cms/grading/tasktypes/abc.py 78.84% <0.00%> (-1.16%) ⬇️
cms/server/contest/handlers/contest.py 86.40% <0.00%> (-0.60%) ⬇️
cms/db/types.py 100.00% <0.00%> (ø)
cmscommon/binary.py 100.00% <0.00%> (ø)
... and 95 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4c9e92...109ee96. Read the comment docs.

@andreyv
Copy link
Member

andreyv commented Nov 22, 2020

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.

@magula
Copy link
Contributor Author

magula commented Dec 16, 2020

You're right -- I'll make a new commit implementing the change you proposed.

@andreyv andreyv force-pushed the fix_printingservice3 branch from f4e3936 to 109ee96 Compare December 26, 2020 15:35
@andreyv andreyv merged commit c5935e0 into cms-dev:master Dec 26, 2020
@andreyv
Copy link
Member

andreyv commented Dec 26, 2020

Tested and merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants