Janino classloader leak

[chaokunyang]

Issue

When using org.codehaus.janino.Compiler to compiler a class which reference a class in URLClassLoader, and load the compiled byte code into the URLClassLoader, janino org.codehaus.janino.ClassLoaderIClassLoader will leak, which lead the generated classes and URLClassLoader can't be gc.

The class can be gc when I set classLoader/loadedIClasses in ClassLoaderIClassLoader to null. But I don't know why. Maybe we should make loadedIClasses value as weak ref, or add a clear method to ClassLoaderIClassLoader,

I can submmit a PR if needed, but why the classloader can't be gc when we don't clear ClassLoaderIClassLoader.

Version

• janino:3.0.7/3.1.6
• jdk: 1.8/11

Reproduction code

  @Test
  public void testJaninoCompiler() throws Exception {
    // WeakReference<? extends Class<?>> clsRef = compileClass2ByJaninoCompiler(compileClassByJaninoCompiler());
    WeakReference<? extends Class<?>> clsRef = compileClass2ByJaninoCompiler();
    while (clsRef.get() != null) {
      System.gc();
      Thread.sleep(1000);
      System.out.printf("Wait cls %s gc.\n", clsRef.get());
    }
  }

  public WeakReference<? extends Class<?>> compileClass2ByJaninoCompiler() throws Exception {
    Class<?> dep = createStructClass("A", 1)
    MapResourceFinder sourceFinder = new MapResourceFinder();
    String stubFileName = "B.java";
    String code =
        ""
            + "import A;\n"
            + "public class B {\n"
            + "  public A process(A o) {\n"
            + "    return o;\n"
            + "  }\n"
            + "}";
    sourceFinder.addResource(stubFileName, code);
    // Storage for generated bytecode
    final Map<String, byte[]> classes = new HashMap<>();
    // Set up the compiler.
    ClassLoaderIClassLoader classLoader = new ClassLoaderIClassLoader(dep.getClassLoader());
    Compiler compiler = new Compiler(sourceFinder, classLoader);
    compiler.setClassFileCreator(new MapResourceCreator(classes));
    compiler.setClassFileFinder(new MapResourceFinder(classes));

    // set debug flag to get source file names and line numbers for debug and stacktrace.
    // this is also the default behaviour for javac.
    compiler.setDebugSource(true);
    compiler.setDebugLines(true);

    // Compile all sources
    compiler.compile(sourceFinder.resources().toArray(new Resource[0]));
    byte[] byteCodes = classes.entrySet().iterator().next().getValue();
    ClassLoaderUtils.tryDefineClassesInClassLoader("B", dep, dep.getClassLoader(), byteCodes);
    Class<?> cls = dep.getClassLoader().loadClass("B");
    Assert.assertEquals(cls.getSimpleName(), "B");
    return new WeakReference<>(cls);
  }

  public static Class<?> createStructClass(String classname, int repeat) {
    if (StringUtils.isBlank(classname)) {
      classname = String.format("Struct%d", System.currentTimeMillis());
    }
    StringBuilder classCode =
        new StringBuilder(
            String.format(
                ""
                    + "import java.util.*;\n"
                    + "public final class %s implements java.io.Serializable {\n"
                    + "  public String toString() {\n"
                    + "   return io.fury.test.bean.Struct.toString(this);\n"
                    + "  }\n"
                    + "  public boolean equals(Object o) {\n"
                    + "   return io.fury.test.bean.Struct.equalsWith(this, o);\n"
                    + "  }\n",
                classname));

    String fields =
        ""
            + "  public int f%s;\n"
            + "  public Integer f%s;\n"
            + "  public String f%s;\n"
            + "  public int[] f%s;\n"
            + "  public Double[] f%s;\n"
            + "  public List<Integer> list_int%s;\n"
            + "  public List<String> list_str%s;\n"
            + "  public Map<String, String> map_ss%s;\n"
            + "  public Map<Long, Double> map_ld%s;\n"
            + "  public Object obj%s;\n";
    int numFields = 10;
    for (int i = 0; i < repeat; i++) {
      classCode.append(
          String.format(
              fields, IntStream.range(i * numFields, i * numFields + numFields).boxed().toArray()));
    }
    classCode.append("}");

    Path path = Paths.get(classname + ".java");
    try {
      Files.deleteIfExists(path);
      Files.write(path, classCode.toString().getBytes());
      // Use JavaCompiler because janino doesn't support generics.
      JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
      int result =
          compiler.run(
              null,
              new ByteArrayOutputStream(), // ignore output
              new ByteArrayOutputStream(), // ignore output
              "-classpath",
              System.getProperty("java.class.path"),
              path.toString());
      if (result != 0) {
        throw new RuntimeException(String.format("Couldn't compile code:\n %s.", classCode));
      }
      Class<?> clz =
          new URLClassLoader(
                  new URL[] {Paths.get(".").toUri().toURL()}, Struct.class.getClassLoader())
              .loadClass(classname);
      Files.delete(path);
      Files.delete(Paths.get(classname + ".class"));
      return clz;
    } catch (Exception e) {
      throw new RuntimeException(e);
    }
  }

[aunkrig]

Hey Shawn,

your handling of the class loaders is a bit unorthodox and may the reason for the resource leak: If this code

byte[] byteCodes = classes.entrySet().iterator().next().getValue();
ClassLoaderUtils.tryDefineClassesInClassLoader("B", dep, dep.getClassLoader(), byteCodes);
Class<?> cls = dep.getClassLoader().loadClass("B");

does what its name implies, the you define the Janino-generated class "B" in the class loader that loaded class "A", which is a no-no. You should instead create another class loader

Class<?> cls = new ByteArrayClassLoader(classes).loadClass("B");

, and, as far as I can see, there is no resource leak:

Wait cls null gc.

BTW: The Eclipse compiler issues a warning "Resource leak: '' is never closed" about this fragment of your code:

Class<?> clz = new URLClassLoader(
    new URL[] { Paths.get(".").toUri().toURL() },
    Struct.class.getClassLoader()
).loadClass(classname);

That smells like a resource leak, too...

Can you please check whether the proposed code change fixes the problem?

[chaokunyang]

no-no

Hi @aunkrig , thanks for your reply. Creating a new classloader won't leak resource. But here the generated class may access package-level API in dep class, which is only possible when defining the generated class in dep.getClassLoader(), that's why we define the generated class in dep.getClassLoader().

In our system we may have some classloader hot switch support. In such case the old classsloader and classes should be garbage collected, otherwise the metaspace will OOM. But when the generated class and original class are not used and not referenced by caller, those classes and their classloader are not garbage collected if I don't set classLoader/loadedIClasses in ClassLoaderIClassLoader to null

[aunkrig]

Are you sure that package-level access is forbidden across class loaders? I don't think so!

Again, I believe that the leak problem originates from that you call defineClass() on an existing class loader. defineClass() should only be invoked by findClass()!

Please check.

[chaokunyang]

Package-level access is forbidden across class loaders. For example, following code throws IllegalAccessError

@Test
  public void testJaninoClass()
      throws CompileException, ClassNotFoundException, IllegalAccessException,
          InstantiationException {
    SimpleCompiler compiler = new SimpleCompiler();
    String code =
        ""
            + "import java.util.function.Function;\n"
            + " class A implements Function {\n"
            + "  @Override\n"
            + "  public Object apply(Object o) {\n"
            + "    return o;\n"
            + "  }\n"
            + "}";
    // default to context class loader. set if only use another class loader
    compiler.setParentClassLoader(Thread.currentThread().getContextClassLoader());
    compiler.cook(code);
    Class<? extends Function> aCLass =
        compiler.getClassLoader().loadClass("A").asSubclass(Function.class);
    // @SuppressWarnings("unchecked")
    // Function<Object, Object> function = aCLass.newInstance();
    // Assert.assertEquals(function.apply("test class"), "test class");

    compiler = new SimpleCompiler();
     code =
        ""
            + "import java.util.function.Function;\n"
            + " class B extends A {\n"
            + "}";
    // default to context class loader. set if only use another class loader
    compiler.setParentClassLoader(aCLass.getClassLoader());
    compiler.cook(code);
    Class<? extends Function> cls2 =
        compiler.getClassLoader().loadClass("B").asSubclass(Function.class);
    System.out.println(aCLass.getClassLoader());
    System.out.println(cls2.getClassLoader());

java.lang.IllegalAccessError: class B cannot access its superclass A (B is in unnamed module of loader org.codehaus.janino.ByteArrayClassLoader @14dd7b39; A is in unnamed module of loader org.codehaus.janino.ByteArrayClassLoader @46d59067)

    at java.base/java.lang.ClassLoader.defineClass1(Native Method)
    at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
    at org.codehaus.janino.ByteArrayClassLoader.findClass(ByteArrayClassLoader.java:77)
    at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:589)
    at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
    at io.fury.codegen.JaninoUtilsTest.testJaninoClass(JaninoUtilsTest.java:159)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
    at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599)
    at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
    at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
    at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822)
    at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
    at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
    at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)

[aunkrig]

Again, I believe that the leak problem originates from that you call defineClass() on an existing class loader. defineClass() should only be invoked by findClass()!

[chaokunyang]

The existing class loader is an URLClasssLoader, and not referenced by other classes, after compileClass2ByJaninoCompiler method return, there should be any strong references to all those generated class, and they are all eligible for gc?

[aunkrig]

I am afraid that I cannot help you with this one. If you find a workaround, please let me know and I would happily integrate it.

[chaokunyang]

Set classLoader&loadedIClasses in ClassLoaderIClassLoader to null will work, I'll check it again and submit a PR if it does fix the issue.