Make WrapDynaClassTest tolerate gc eviction of weak cache entries (1.X)#425
Merged
garydgregory merged 1 commit intoJul 21, 2026
Merged
Conversation
The per-classloader cache holds its CacheKey weakly and nothing else references it, so any GC that runs between two createDynaClass calls can evict the entry and the next caller legitimately builds a second instance. testConcurrentCreateDynaClassReturnsSameInstance asserted exactly one instance per round regardless, so a GC landing inside a round failed it on CI even though the synchronized get/create/put from PR 419 is correct. Reproducible without any concurrency: create, call System.gc(), create again, and the second call returns a new instance. Guard each round with a WeakReference canary: only assert the single-instance property when the canary shows no GC ran during the round, and skip rounds a GC invalidated. Apply the same guard to testCreateDynaClassIsCached, which had the same sensitivity in a smaller window. The test still fails on the unsynchronized pre-419 code and now passes under a System.gc() hammer thread that previously made it fail immediately with the same error as the CI run. Signed-off-by: Naveed Khan <dxbnaveed.k@gmail.com>
7 tasks
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.
Follow-up to #419, where
WrapDynaClassTest.testConcurrentCreateDynaClassReturnsSameInstancefailed on 1.X CI (run).The failure is the test asserting more than the cache guarantees, not the synchronization from #419. The per-classloader cache is weak-keyed (
WeakFastHashMapdelegates toWeakHashMap) and theCacheKeyis referenced by nothing but the map, so any GC evicts the entry and the nextcreateDynaClasscall legitimately builds a new instance. No concurrency is needed to see it: on unmodified 1.X,createDynaClass,System.gc(),createDynaClassreturns a distinct second instance. When a GC lands inside one of the test's rounds, a late thread misses the cache and the round observes two instances — exactly the CI failure (expected: <1> but was: <2>). Master is unaffected because the beanutils2 cache (ConcurrentReferenceHashMap) holds soft references, which survive ordinary GCs.The fix allocates a
WeakReferencecanary at the start of each round and only asserts the single-instance property when the canary shows no GC ran during the round; rounds a GC invalidated are skipped.testCreateDynaClassIsCachedgets the same guard since it has the same sensitivity in a smaller window.Verified locally:
System.gc()hammer thread makes the unguarded test fail immediately with the CI error; the guarded test passes under the same hammerSame caveat as #419 and #416: the full default goal hits the one pre-existing failure in
LocaleBeanificationTest.testContextClassloaderIndependenceon my machine, which fails identically on unmodified1.X; everything else passes.mvn; that'smvnon the command line by itself.