fix: executor shutdown race, dropped kwargs, chunk revalidation, unknown-class cache - #1699
Draft
ariostas wants to merge 1 commit into
Draft
fix: executor shutdown race, dropped kwargs, chunk revalidation, unknown-class cache#1699ariostas wants to merge 1 commit into
ariostas wants to merge 1 commit into
Conversation
…own-class cache Four independent correctness fixes in the source and model layers. ThreadPoolExecutor.shutdown queued one sentinel per worker and only then joined them, while ResourceThreadPoolExecutor set its closed flag after that. A submit racing with shutdown passed the guard, landed behind a sentinel, and was never run, so its Future blocked forever. Set the flag before queuing the sentinels and reject submits from that point on; ThreadPoolExecutor now has the same guarantee, which it previously lacked entirely. ThreadPoolExecutor.submit accepted **kwargs and dropped them on the floor: submit(task, 1, x=3) silently ran task(1). Carry them on the Future. Chunk.wait assigned self._raw_data before checking its length, so a chunk that failed validation raised on first access and handed back the short buffer on every access after that. Publish the data only once it has passed. Unknown model classes were cached in uproot.unknown_classes under the bare classname, so version 2 of a class got the class built for version 1, and a versionless UnknownClass and a versioned UnknownClassVersion evicted each other. Key on the encoded classname, which already carries both distinctions. Assisted-by: claude-code:claude-opus-5[1m]
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
|
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.
🤖 AI text below 🤖
Four independent, low-risk correctness fixes from #1688 (findings 1–3 of "PR 3" and finding 3 of "PR 5"). Grouped because each is a few lines with no API surface; happy to split if you'd rather review them separately.
Note this deliberately does not include finding 4 of "PR 3" (
with uproot.open(...)not shutting down attached executors). That one changes what__exit__closes and would shut down a user-supplied executor reused across files, so it needs its own discussion.1. Submit racing with shutdown was orphaned
ThreadPoolExecutor.shutdownqueues one sentinel per worker and then joins them;ResourceThreadPoolExecutoronly set_closedafter that returned. Asubmitin that window passed the guard, landed behind a sentinel, and was never run — itsFutureblocked forever. Reproduced by occupying the single worker sojoin()blocks:Sets the flag before queuing the sentinels.
ThreadPoolExecutornow gets the same guarantee, which it previously lacked entirely.2.
submitaccepted keyword arguments and discarded themThreadPoolExecutor.submit(task, /, *args, **kwargs)builtFuture(task, args), sosubmit(task, 1, x=3)silently rantask(1). Now carried on theFuture.3. A failed chunk length check only failed once
Chunk.waitassignedself._raw_databefore validating its length, so a chunk declared as 5 bytes but backed by 3 raisedOSErroron first access and returned the truncated buffer on every access after that. The data is now published only once it has passed.4. Unknown model classes were cached by classname alone
uproot.unknown_classeswas keyed on the bare classname, so version 2 of a class got back the class built for version 1 (Unknown_MyClass_v1), and a versionlessUnknownClassand a versionedUnknownClassVersionfor the same classname evicted each other. Keyed on the encoded classname instead, which already carries both distinctions. The dict staysstr -> class.Tests
tests/test_1688_source_hygiene.py, 9 tests; 6 fail onmain.Full suite passes locally (1032 passed, 90 skipped).