GROOVY-12142: Stop pinning container class loaders: remove PIC-Cleane… - #2798
GROOVY-12142: Stop pinning container class loaders: remove PIC-Cleane…#2798paulk-asert wants to merge 1 commit into
Conversation
…r and DFA-cache-cleaner threads, restore the ClassValue escape hatch A Groovy copy deployed per webapp (the common Tomcat/WEB-INF/lib topology) permanently pinned its class loader, growing metaspace on every parallel (re)deployment. Three independent pins: 1. PIC-Cleaner: linking any indy call site started a never-terminating daemon thread from CacheableCallSite's static initializer, pinning the defining loader and capturing the creating context's protection domains. Stale PIC entries are now swept inline — both callers already hold the lruCache monitor and the cache is bounded (8), so the sweep is trivial; the groovy.indy.callsite.cleaner.inline flag (GROOVY-12092) is gone along with the thread it toggled, which also removes the parked-thread false positive it existed to avoid. 2. DFA-cache-cleaner: each AtnManager started a never-terminating reference-queue thread whose catch-all swallowed even interrupts. The softly referenced AtnWrapper is a GC canary; its collection is now detected on the parse path when the soft reference reads null, clearing the shared DFA cache at the next parse instead of at GC time — no queue, no thread. 3. java.lang.ClassValue associations on immortal (bootstrap) classes never release their value's class loader (JDK-8136353), so one dynamic dispatch on e.g. String pinned the loader. The groovy.use.classvalue=false escape hatch (present through 4.x, lost in the GROOVY-11520 collections cleanup) is restored, with the fallback rebuilt on ManagedIdentityConcurrentMap (~40 lines) rather than the removed deprecated collections. Default stays ClassValue. With 1+2 fixed, a dropped loader is collectable either with the flag or with a shutdown sweep over ClassInfo.getAllClassInfo() calling ClassInfo.remove — both verified by classloader-churn harness against the rebuilt jar; out-of-the-box default config still requires one of the two (the JVM bug is not ours to fix).
8bba0f9 to
7442977
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2798 +/- ##
==================================================
+ Coverage 70.1064% 70.1412% +0.0348%
- Complexity 35774 35819 +45
==================================================
Files 1561 1563 +2
Lines 132373 132390 +17
Branches 24332 24353 +21
==================================================
+ Hits 92802 92860 +58
+ Misses 31170 31133 -37
+ Partials 8401 8397 -4
🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: 7442977 Learn more about TestLens at testlens.app. |
| long-lived classes from ever releasing their value — and with it the Groovy class loader the | ||
| value belongs to. In a container this shows up as metaspace growth on every redeployment, even | ||
| after the old application is undeployed. | ||
|
|
There was a problem hiding this comment.
This is fine for the integration guide, so I do not want to block this.
But for us developers I think that is a bit too short for the complexity of ClassValue. First of all the bug you mentioned has been closed. There are follow-ups, but they are documentation issues, not something to be fixed. in that manner it works as designed.
Think of it like this. With ClassValue we basically realize a chain: key class - class association -> class value instance -> value
Important to note here is that the class of the ClassValue implementation itself does not prevent any unloading. It is different from a static field in the key class for example. Now if we add the value, then there is a strong association between the key class and the value. If the value is for example an empty ArrayList, then it will not prevent the unloading of the classloader for the ClassValue. But if in the Tomcat scenario with Groovy the value class is from Groovy, then the loader for Groovy will be prevented from unloading as long as the key class exists. If the key class is an immortal class like String, then even unloading Tomcat and having had Groovy loaded in an isolated class loader, would still keep that class loader around.
The issue mentioned at openjdk.org is actually more complicated than this. Assume we store an ArrayList again, but the element is any instance of a class of that Groovy defining class loader. Then we avoided the class problem for the value, but again got it through one level of indirection. And the real problem in this scenario is that this can be a long chain. The sort formula for Groovy is then, key = Groovy class is fine. key == JDK class, bad. The issue itself was actually for the stored value referencing the ClassValue instance. I put emphasis on the class loader here, because that is actually the case we care about.
There is a strategy against that: use a SoftReference for the value of ClassValue. Since the chain is now always broken there would be no issue anymore. Of course then the usage of the ClassValue must fulfill some constraints. Only getting the value no longer works, you have to check the reference and if it did not retain the class value, remove the association and recompute it. That means it must be legal to recompute the value in the first place and the recomputing cost is to be considered.
Based on that I see some potentially problematic parts in Groovy:
- ClassInfo.globalClassValue -> static + unknown keys -> problem
- AwaitableAdapterRegistry.awaitableCache -> static + unknown keys -> problem
- Closure.CALL_OVERRIDES -> should be ok
I think we should investigate the two problems above further. So maybe to not loose this kind of context we create a followup issue?
3 participants
…r and DFA-cache-cleaner threads, restore the ClassValue escape hatch
A Groovy copy deployed per webapp (the common Tomcat/WEB-INF/lib topology) permanently pinned its class loader, growing metaspace on every parallel (re)deployment. Three independent pins:
PIC-Cleaner: linking any indy call site started a never-terminating daemon thread from CacheableCallSite's static initializer, pinning the defining loader and capturing the creating context's protection domains. Stale PIC entries are now swept inline — both callers already hold the lruCache monitor and the cache is bounded (8), so the sweep is trivial; the groovy.indy.callsite.cleaner.inline flag (GROOVY-12092) is gone along with the thread it toggled, which also removes the parked-thread false positive it existed to avoid.
DFA-cache-cleaner: each AtnManager started a never-terminating reference-queue thread whose catch-all swallowed even interrupts. The softly referenced AtnWrapper is a GC canary; its collection is now detected on the parse path when the soft reference reads null, clearing the shared DFA cache at the next parse instead of at GC time — no queue, no thread.
java.lang.ClassValue associations on immortal (bootstrap) classes never release their value's class loader (JDK-8136353), so one dynamic dispatch on e.g. String pinned the loader. The groovy.use.classvalue=false escape hatch (present through 4.x, lost in the GROOVY-11520 collections cleanup) is restored, with the fallback rebuilt on ManagedIdentityConcurrentMap (~40 lines) rather than the removed deprecated collections. Default stays ClassValue.
With 1+2 fixed, a dropped loader is collectable either with the flag or with a shutdown sweep over ClassInfo.getAllClassInfo() calling ClassInfo.remove — both verified by classloader-churn harness against the rebuilt jar; out-of-the-box default config still requires one of the two (the JVM bug is not ours to fix).