Repository navigation
TIMX 739 - lxml memory pointer bug - #286
Merged
Merged
Conversation
Why these changes are being introduced: * Fixes a regression from 4149dbf: alma runs that span more than one write batch wrote all output and logged completion, then aborted on exit with a memory fault, e.g. "free(): invalid pointer" or "pointer being freed was not allocated". * Parsing Marc records with lxml overrode XMLTransformer.parse_source_record(), which bypassed parse_bs4_in_isolated_thread(). That thread was what prevented this same fault, introduced in 6772c18, and it is not specific to bs4. * Arrow reads the record generator using a new thread state per batch, and lxml has a string dictionary per thread state. If the thread state that finishes iterparse has already parsed something else, lxml replaces the iterparse document's dictionary, and freeing that document on exit then frees strings it does not own. How this addresses that need: * Marc.parse_source_record() parses in a short-lived thread, as the bs4 helper does. Exceptions from parsing are re-raised to the caller, so errors are still logged with their original cause. * The fix is nearly identical to the original solution to this problem, duplicating the logic and code for a short-term fix. Side effects of this change: * Output is unchanged: 19,052 alma records from the file that failed are identical before and after, ignoring run provenance. * A thread per record is a small cost in one process; the --workers path also pays it, though it was not affected by the fault. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-739
Why these changes are being introduced: * The previous commit avoids the memory fault on exit by keeping Marc parsing off the thread states that pyarrow uses to read records, but any other lxml parsing on those thread states could cause it again. * The cause is iterparse finishing under a different thread state than it started, with a different string dictionary, so that is the better place to fix it. How this addresses that need: * XMLTransformer.read_source_file() runs iterparse on one dedicated thread for the whole file, passing records back in small chunks, so it starts and finishes under the same thread state regardless of how the generator is resumed. * Marc.parse_source_record() no longer needs a thread per record, and goes back to parsing directly. Side effects of this change: * Output is unchanged: 19,052 alma records are identical before and after, ignoring run provenance, and with and without workers. * No measurable change in speed for alma, and a thread per record is no longer used. * Up to 100 records are read ahead, as bytes, while reading a source file. * parse_bs4_in_isolated_thread() remains for other XML sources. A test run with it bypassed did not fault with this change, and did without it, so it could be removed in a future change. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-739
Why these changes are being introduced: * The isolated thread prevented a memory fault on exit by keeping bs4 parsing off the thread states that pyarrow uses to read records. Now that iterparse runs on a dedicated thread, parsing on those thread states no longer causes the fault, so a thread per record is an unneeded cost for XML sources. How this addresses that need: * XMLTransformer.parse_source_record() calls BeautifulSoup directly. * The memory fault tests no longer describe parsing as threaded, and continue to cover both bs4 and lxml parsing across multiple write batches. Side effects of this change: * Output is unchanged: 6,006 researchdatabases records are identical before and after, ignoring run provenance, and with and without workers. * Non-Marc XML sources should be somewhat faster in one process. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-739
ghukill
marked this pull request as ready for review
November 6, 2026 16:55
ghukill
commented
Nov 6, 2026
Comment on lines
+36
to
+41
| records = cls._iterparse_source_file(source_file) | ||
| with ThreadPoolExecutor(max_workers=1) as executor: | ||
| while chunk := executor.submit( | ||
| lambda: list(itertools.islice(records, READ_CHUNK_SIZE)) | ||
| ).result(): | ||
| yield from chunk |
Contributor
Author
There was a problem hiding this comment.
This is ultimately the root level fix.
First, we assigns records as the lazy iterator coming from lxml.iterparse().
Then, under a single, throwaway thread, we yield chunks from this iterator.
It's eventually cleanly cleaned up because the entirety of the iterparse() calls completes in a single thread; the explicit one we create here.
ghukill
commented
Nov 6, 2026
| return cls.parse_bs4_in_isolated_thread(source_record) | ||
|
|
||
| @classmethod | ||
| def parse_bs4_in_isolated_thread(cls, source_record: bytes) -> Tag: |
Contributor
Author
There was a problem hiding this comment.
This previous workaround is no longer needed given the more root level fix at XMLTransformer.read_source_file().
ehanson8
approved these changes
Nov 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose and background context
This PR addresses a regression that was introduced with the conversion of the Alma MARC transformer from BeautifulSoup4 (
bs4) tolxml.In short, this transformer stopped using the custom method
parse_bs4_in_isolated_thread()from when we previously encountered this edge case. It was missed during testing because only runs > 1,000 records and those with a single worker would trigger it (confusingly, multiple workers which are new would sidestep the issue).A detailed explanation can be found here: https://mitlibraries.atlassian.net/browse/TIMX-739?focusedCommentId=206671.
This PR addresses the issue in 3 phases / commits:
parse_bs4_in_isolated_thread()to Alma MARC transformer. Confirmed addressed regression. (commit)XMLTransformer.read_source_file(). Confirmed regression still fixed, even after rolling back of # 1. (commit)parse_bs4_in_isolated_thread()entirely as the # 2 commit should suffice. Confirmed successful runs across multiple sources. (commit)How can a reviewer manually see the effects of these changes?
Complicated to reproduce, and the final outcome is just successful runs. However, can confirm that the recent failure in production for input file
s3://timdex-extract-prod-300442551476/alma/alma-2026-11-06-daily-extracted-records-to-index.xmlnow completes successfully. This was a run of > 1,000 records and was called via a single worker, triggering this edge case scenario.Includes new or updated dependencies?
NO
Changes expectations for external applications?
YES: runs are successful for all sources, single or multiple workers
What are the relevant tickets?
Code review