[
https://issues.apache.org/jira/browse/GROOVY-8113?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18141994#comment-18141994
]
ASF GitHub Bot commented on GROOVY-8113:
----------------------------------------
daniellansun commented on code in PR #3109:
URL: https://github.com/apache/groovy/pull/3109#discussion_r4525453541
##########
src/main/java/groovy/lang/MetaClassRegistry.java:
##########
@@ -161,6 +169,41 @@ private MetaClass createWithCustomLookup(Class theClass,
MetaClassRegistry regis
}
}
+ private static boolean customMetaClassVisible(String customName) {
+ ClassLoader loader = MetaClassRegistry.class.getClassLoader();
+ if (loader == null) {
+ return true;
+ }
+ for (ClassLoader current = loader; current != null; current =
current.getParent()) {
+ if (current instanceof GroovyClassLoader
+ && ((GroovyClassLoader)
current).getClassCacheEntry(customName) != null) {
+ return true;
+ }
+ }
+ return classResourcePresent(loader, customName);
+ }
+
+ /**
+ * True when {@code className} is visible as a class file. A native
image
+ * with exact reachability metadata throws if that resource was not
+ * registered; the class may still be loadable, so the caller falls
+ * through to {@code Class.forName}.
+ */
+ private static boolean classResourcePresent(ClassLoader loader, String
className) {
+ try {
+ return loader.getResource(className.replace('.', '/') +
".class") != null;
+ } catch (Error error) {
+ if (isMissingResourceRegistration(error)) {
Review Comment:
`MetaClassRegistry.class.getClassLoader() == null` means this class was
defined by the bootstrap loader. There is no `ClassLoader` object to walk, and
`getResource` on null would throw. `Class.forName(String)` then uses that same
bootstrap loader (it is `Class.forName(name, true, caller.getClassLoader())`).
Returning false would skip that call and drop a custom MetaClass that really is
on the boot classpath. Returning true means "not shown to be absent": we fall
through to `Class.forName`, and a miss is still the `ClassNotFoundException`
path that calls `createNormalMetaClass`. The system loader is a different
loader, so probing it would not match what `Class.forName` does from this
class. The cost is that this one configuration does not avoid the
`parallelLockMap` entry; Groovy on the boot classpath is the case we cannot
probe.
##########
src/main/java/groovy/lang/MetaClassRegistry.java:
##########
@@ -143,7 +143,15 @@ public final MetaClass create(Class theClass,
MetaClassRegistry registry) {
private MetaClass createWithCustomLookup(Class theClass,
MetaClassRegistry registry) {
try {
- final Class customMetaClass =
Class.forName("groovy.runtime.metaclass." + theClass.getName() + "MetaClass");
+ final String customName = "groovy.runtime.metaclass." +
theClass.getName() + "MetaClass";
+ // Class.forName records a missing name in every
parallel-capable
+ // loader it asks, and the entry is never removed. The lookup
can
+ // only succeed for a class already defined by this runtime's
+ // loader or present as a classpath resource.
+ if (!customMetaClassVisible(customName)) {
Review Comment:
The second point is a real hole. The comment says the lookup can only
succeed for a class already defined by this runtime's loader or present as a
`.class` resource. That is stronger than what `Class.forName` does.
`customMetaClassVisible` returns false, and we never call `Class.forName`, when
no ancestor `GroovyClassLoader` has the name in `getClassCacheEntry` and
`getResource(binaryName + ".class")` returns null. A class loader can define a
class with no resource behind it: `defineClass(byte[])`, a `findClass` that
synthesizes bytes, or a class already defined in a non-`GroovyClassLoader`
parent (`findLoadedClass` succeeds inside `loadClass` with no `.class` URL).
Those are missed, and `createNormalMetaClass` is used instead. A native image
has the same shape when the class is registered for reflection and the resource
is not in the image. `getResource` then returns null. We already treat
`MissingResourceRegistrationError` as "still try `Class.forName`" because the
class may be loadable without a registered resource; a null resource is that
situation without the exception, and it currently takes the opposite branch.
`GroovyClassLoader.hasClassResource` treats that error as a miss on purpose, so
the parent is not asked. This lookup should not copy that, because skipping
here changes which MetaClass the user gets.
The check is there because `Class.forName` on a parallel-capable loader
records every name in `parallelLockMap` from `getClassLoadingLock`, and that
entry is never removed. Each generated script otherwise leaves
`groovy.runtime.metaclass.<script>MetaClass` in the application loader and its
parents. A `.class` resource, a `GroovyClassLoader` cache hit, a null loader,
and the native-image registration error are reasons to call `Class.forName`. A
null resource is not a reason to skip it for an arbitrary class.
The skip should be limited to the names that grow without bound: the
`groovy.runtime.metaclass.<generated>MetaClass` companion of a script or
template class, where no parent can define that name and no `.class` resource
exists. A custom MetaClass that is a loaded class without a `.class` resource,
and any stable class whose companion a loader can still define, should still go
through `Class.forName`. `CustomMetaClassTest` only covers the classpath
`.class` case (`groovy.runtime.metaclass.bugs.CustomMetaClassTestMetaClass`),
so it does not catch this miss.
I will limit the skip to generated-script companion names that have no
resource and that no parent can define, and add a test for a custom MetaClass
defined without a `.class` resource.
> Groovy script/template engine produce memory leak
> --------------------------------------------------
>
> Key: GROOVY-8113
> URL: https://issues.apache.org/jira/browse/GROOVY-8113
> Project: Groovy
> Issue Type: Bug
> Components: groovy-templates, GroovyScriptEngine
> Affects Versions: 2.4.8
> Environment: Groovy 2.4.8, Oracle Java JDK 8u121
> Reporter: Artyom Kravchenko
> Priority: Critical
> Attachments: merged_path.png, object_explorer.png
>
>
> There is a simple way to produce OutOfMemoryException if run infinite
> numbers of groovy scripts or evaluate groovy templates.
> Simple test to reproduce a memory leak(run groovy script and make template
> inside the script):
> set max heap size: -Xmx7m
> {code:java}
> import javax.script.*;
> import java.util.HashMap;
> import java.util.Map;
> public class OutOfMemoryTest {
> private static String TEMPLATE = "Dear ${fullName} please go away at
> ${time}";
> private static String SCRIPT = "def run(args) {\n" +
> "def bindings = ['fullName' : 'Ali Baba', 'time' : new Date()]\n"
> +
> "println
> args.templateEngine.createTemplate(args.template).make(bindings).toString()\n"
> +
> "}";
> public static void main(String... args) {
> //
> GroovySystem.getMetaClassRegistry().getMetaClassCreationHandler().setDisableCustomMetaClassLookup(true);
> while(true) {
> ScriptEngineManager engineManager = new ScriptEngineManager();
> ScriptEngine engine = engineManager.getEngineByName("groovy");
> try {
> engine.eval(SCRIPT);
> } catch (ScriptException e) {
> throw new RuntimeException(e);
> }
> groovy.text.SimpleTemplateEngine templateEngine = new
> groovy.text.SimpleTemplateEngine();
> Invocable inv = (Invocable) engine;
> Map<String, Object> params = new HashMap<>();
> params.put("template", TEMPLATE);
> params.put("templateEngine", templateEngine);
> Object[] runArgs = { params };
> try {
> inv.invokeFunction("run", runArgs);
> } catch (ScriptException | NoSuchMethodException e) {
> e.printStackTrace();
> }
> }
> }
> }
> {code}
> The main cause of issue is Java class loader. Two instances:
> sun.misc.Launcher.AppClassLoader and sun.misc.Launcher.ExtClassLoader
> (parent for first one), each of them has a strong reference:
> {code:java}
> ConcurrentHashMap<String, Object> parallelLockMap
> {code}
> It collects all loaded classes (and never releases their) for application
> including newly created script/template classes.
> The main reason of introduction of this reference described here:
> https://blogs.oracle.com/dholmes/entry/parallel_classloading_revisited_fully_concurrent
> For each iteration of my test I see that parallelLockMap grows with 6 new
> entries:
> {code:java}
> groovy.runtime.metaclass.Script94MetaClass
> Script94BeanInfo
> Script94Customizer
> groovy.runtime.metaclass.SimpleTemplateScript94MetaClass
> SimpleTemplateScript94BeanInfo
> SimpleTemplateScript94Customizer
> {code}
> See memory snapshots(attached): objects_explorer, merged_path.
> I have found a partial solution of the issue, if I add this code line to my
> test:
> {code:java}
>
> GroovySystem.getMetaClassRegistry().getMetaClassCreationHandler().setDisableCustomMetaClassLookup(true);
> {code}
> I see that classes like:
> {code:java}
> groovy.runtime.metaclass.Script94MetaClass
> groovy.runtime.metaclass.SimpleTemplateScript94MetaClass
> {code}
> goes away (groovy engine don't ask ClassLoader to find this classes on class
> path therefore such classes are not collected by ClassLoader) but classes
> like
> {code:java}
> Script94BeanInfo
> Script94Customizer
> SimpleTemplateScript94BeanInfo
> SimpleTemplateScript94Customizer
> {code}
> still has affect on memory.
> Dear developers, could you please point me any advice/workarounds for this
> issue. It will be fine to get rig this issue in future releases.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
