keeping page ref after close leads to crashes in 1.17.1 (but not in 1.16.xx)

[cbm755]

If I take a page ref, then close the doc, I eventually (after repeating a couple times) I get:

• a zero-len doc
• crashes
  This look related to Should be able to close document explicitly #16.

To Reproduce

Consider this code to crawls a directory of pdf files (their content seems unimportant).

from glob import glob
import fitz

for i, f in enumerate(glob("*.pdf")):
    print("i={}, file={}".format(i, f))

    doc = fitz.open(f)

    print("  {} pages".format(len(doc)))

    front_page = doc[0]   # remove this line and no crash

    doc.close()

I'm not sure if that is supposed to work or not but it reliably did work in 1.16.xx. Even if its invalid it probably shouldn't be segfaulting!

Sometimes I get crashes:

i=0, file=more4.pdf
  20 pages
i=1, file=more0.pdf
  0 pages
Traceback (most recent call last):
  File "foo.py", line 12, in <module>
    front_page = doc[0]   # remove this line and no crash
  File "/home/cbm/.local/lib/python3.8/site-packages/fitz/fitz.py", line 3577, in __getitem__
    raise IndexError("page not in document")
IndexError: page not in document
fish: Job 2, “python foo.py” terminated by signal SIGSEGV (Address boundary error)

other times just the IndexError (note: I'm using multiple copies of the same PDF file for this MWE).

Expected behavior

I'm not sure but the MWE runs without error on 1.16.18.

configuration

• Fedora 32

3.8.3 (default, May 15 2020, 00:00:00) 
[GCC 10.1.1 20200507 (Red Hat 10.1.1-1)] 
 linux 
 
PyMuPDF 1.17.1: Python bindings for the MuPDF 1.17.0 library.
Version date: 2020-06-13 07:54:31.
Built for Python 3.8 on linux (64-bit).

other info

Downstream issue: https://gitlab.com/plom/plom/-/issues/919

[cbm755]

update: the MWE seems ok (i.e., does not crash) on 1.17.0 as well

[JorjMcKie]

Thanks for submitting this! Weird.
No problems either if I do page = None before the doc.close().

[JorjMcKie]

Found the cause. My effort to improve inline documentation caused deletion of a critical piece of code ... 😡, which is responsible for keeping the parent-child relationship between document and its pages in sync.
Thanks again for spotting this.

[cbm755]

Happy to help!

May I ask if its ok to close an pdf after inserting it into a new pdf (but before saving that new PDF). I.e., something like:

bigdoc = fitz.open()

for f in ...
    doc = fitz.open(f)
    front_page = doc[0]
    # ... do some manipulation, adding text or images
    bigdoc.insertPDF(doc)
    del front_page    # (workaround https://github.com/pymupdf/PyMuPDF/issues/533)
    doc.close()
bigdoc.save(...)

[JorjMcKie]

The error you detected, only (fingers crossed) pertains to the close() method of the Dpcument class.

If you just don't close, everything should be fine: all resources will still be feed / dropped, just at some other point in time. E.g. the "close" (meaning dropping associated resources) will simply occur when you assign the next Document to doc. At that point, front_page-associated resources will also be freed, if you hadn't deleted it earlier.

The last "version" of variable doc will be freed at your script's end.

[cbm755]

Is close called on GC? If so, seems to me 1.17.1 would be still susceptible to this bug right?

Are you saying it would be a better pattern to not call .close() even in absence of this bug?

[JorjMcKie]

Is close called on GC?

No. But still all resource freeing will take place - no memory leakage will occur. Using it is completely optional and only needed if you want to control the point in time when resources are freed, or when you need to remove the OS lock on the filename or so.

This bug really only occurs if the method is invoked explicitely.

[cbm755]

The context manager version of this pattern also exhibits this bug:

bigdoc = fitz.open()
for f in ...
    with fitz.open(f) as doc:
        front_page = doc[0]
        # ... do some manipulation, adding text or images
        bigdoc.insertPDF(doc)
bigdoc.save(...)

I did that b/c I was worried about exhausting file handles without the explicit close and it seemed more pythonic.

[JorjMcKie]

That's true: the context manager (which I coded, obviously) does use close.
If you are concerned about resource consumption, you can use doc = None instead of close for the time being.

[JorjMcKie]

But actually there is no reason for concern:
The document's file handle is kept inside MuPDF. The release of resources including file handles is automatically executed if the Python variable doc is assigned to something else (or even deleted via del doc).
So the worst think happening in your example is the somewhat extended lifetime of the last doc if you don't actively close or re-assign it.
The Python-C-interface generator software, SWIG, provides a "deletion" slot for for Python classes which are based on some C constructs. This is the point where the resp. MuPDF destructor is invoked whenever Python deletes a document instance, i.e. invokes its __del__.
So in PyMuPDF there are several ways to achieve this: re-assigning the instance variable, deleting it or invoking close.

[cbm755]

Thanks, that's a weight off my mind: I shall just remove .close() for now.

(And eventually switch to the context manager approach, after 1.17.2).

[JorjMcKie]

Just uploaded v1.17.2 to PyPI. Hope this issue is fixed by it.