Fix PDF.close() closing the wrong Page instances (memory leak, #1339)

CHANGELOG.md

@@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file. The format
 ## [Unreleased]
 
 ### Fixed
+- Fix `PDF.close()` closing the wrong `Page` instances: it called `flush_cache()` (which clears the cached `_pages` list) before iterating `self.pages`, so that iteration re-parsed and closed a fresh, never-otherwise-referenced set of pages instead of the ones the caller actually used — leaving the real pages' `_objects`/`_layout`/`get_textmap` caches unreleased. This caused unbounded-seeming memory growth in long-lived processes that open many PDFs sequentially. ([#1339](https://github.com/jsvine/pdfplumber/issues/1339), related: [#1229](https://github.com/jsvine/pdfplumber/issues/1229), [#1189](https://github.com/jsvine/pdfplumber/issues/1189))
 - Initialize PDFium's form environment in `get_page_image` so that filled AcroForm field content is included when rendering pages via `Page.to_image()`. ([#1367](https://github.com/jsvine/pdfplumber/issues/1367))
 - Fix `make venv`, which created the virtual environment at `venv/` but then installed into `${VENV}` (default `.venv/`), causing the target to fail on a fresh checkout (h/t @soodoku). ([ec96f72](https://github.com/jsvine/pdfplumber/commit/ec96f72))
 - Include the wrapped exception's class name in `PdfminerException`'s message when `pdfminer.six` raises an exception without one (e.g. `PDFPasswordIncorrect`), which previously surfaced as a blank error message.

README.md

@@ -577,6 +577,7 @@ Many thanks to the following users who've contributed ideas, features, and fixes
 - [Sebastian Cao](https://github.com/cycsmail)
 - [Kaspar Naraghi](https://github.com/kaninaba94)
 - [Siddharth Gaur](https://github.com/siddharthgaur1)
+- [Timothy O'Hare](https://github.com/timothyohare)
 
 ## Contributing
 

pdfplumber/pdf.py

@@ -122,11 +122,16 @@ def open(
             raise
 
     def close(self) -> None:
-        self.flush_cache()
-
+        # Must close the pages *before* flush_cache() clears `_pages`; the
+        # `pages` property re-parses (and returns a fresh, never-otherwise-
+        # referenced set of pages) if `_pages` is missing, so flushing first
+        # means the pages actually used by the caller never get their heavy
+        # caches (`_objects`, `_layout`, `get_textmap`) released. See #1339.
         for page in self.pages:
             page.close()
 
+        self.flush_cache()
+
         if not self.stream_is_external:
             self.stream.close()
 

tests/test_issues.py

@@ -338,3 +338,26 @@ def test_pr_1195(self):
         ):
             for _ in pdf.annots:
                 pass
+
+    def test_issue_1339(self):
+        """
+        PDF.close() closed the wrong Page instances: flush_cache() ran
+        first and deleted the cached `_pages` list, so the subsequent
+        `for page in self.pages: page.close()` re-parsed and closed a
+        fresh, never-otherwise-referenced set of pages instead of the ones
+        the caller actually used -- leaving the real pages' `_objects` /
+        `_layout` / `get_textmap` caches unreleased after close().
+        https://github.com/jsvine/pdfplumber/issues/1339
+        """
+        path = os.path.join(HERE, "pdfs/issue-33-lorem-ipsum.pdf")
+        pdf = pdfplumber.open(path)
+        pages = pdf.pages
+        for page in pages:
+            page.extract_text()
+            assert hasattr(page, "_objects")
+
+        pdf.close()
+
+        for page in pages:
+            assert not hasattr(page, "_objects")
+            assert page.get_textmap.cache_info().currsize == 0