ClassLoader leak with Jetty and Karaf - static instances of java.lang.Throwable

[grgrzybek]

There's magical (not visible via reflection, only in memory dumps) field inside java.lang.Throwable class called backtrace (source code).

At runtime, in native code, this field is filled with array of Objects which are actually java.lang.Class instances - and we know how sensitive they are in dynamic classloader environments.

In Karaf, through pax-web-jetty, when pax-web bundles are restarted without touching jetty bundles we have a memory leak visible in visualvm:

this     - value: org.apache.felix.framework.BundleWiringImpl #73
 <- m_wiring     - class: org.apache.felix.framework.BundleWiringImpl$BundleClassLoaderJava5, value: org.apache.felix.framework.BundleWiringImpl #73
  <- <classLoader>     - class: org.ops4j.pax.web.service.internal.Activator, value: org.apache.felix.framework.BundleWiringImpl$BundleClassLoaderJava5 #44
   <- [9]     - class: java.lang.Object[], value: org.ops4j.pax.web.service.internal.Activator class Activator
    <- [2]     - class: java.lang.Object[], value: java.lang.Object[] #14374
     <- backtrace     - class: java.lang.Throwable, value: java.lang.Object[] #14373
      <- COMPLETED     - class: org.eclipse.jetty.util.FutureCallback, value: java.lang.Throwable #1
       <- [68]     - class: java.lang.Object[], value: org.eclipse.jetty.util.FutureCallback class FutureCallback
        <- elementData     - class: java.util.Vector, value: java.lang.Object[] #11011
         <- classes     - class: org.apache.felix.framework.BundleWiringImpl$BundleClassLoaderJava5, value: java.util.Vector #112
          <- <classLoader>     - class: org.eclipse.jetty.util.thread.NonBlockingThread, value: org.apache.felix.framework.BundleWiringImpl$BundleClassLoaderJava5 #16
           <- <class> (Java frame)     - class: org.eclipse.jetty.util.thread.NonBlockingThread, value: org.eclipse.jetty.util.thread.NonBlockingThread class NonBlockingThread

The problem is with static instances of Throwable classes. They keep backtraces filled with Classes+ClassLoaders from previous incarnations of other bundles - depending on the time when the static Throwable was created.

Here are static throwables found in jetty-9.2.17.v20160517:

11:40 $ grep -r --include='*.java' 'static.*new.*Exception'
jetty-util/src/test/java/org/eclipse/jetty/util/component/LifeCycleListenerTest.java:    static Exception cause = new Exception("expected test exception");

11:40 $ grep -r --include='*.java' 'static.*new.*Throwable'
jetty-continuation/src/main/java/org/eclipse/jetty/continuation/FauxContinuation.java:    private final static ContinuationThrowable __exception = new ContinuationThrowable();
jetty-continuation/src/main/java/org/eclipse/jetty/continuation/Servlet3Continuation.java:    private final static ContinuationThrowable __exception = new ContinuationThrowable();
jetty-util/src/main/java/org/eclipse/jetty/util/SharedBlockingCallback.java:    private static Throwable IDLE = new Throwable()
jetty-util/src/main/java/org/eclipse/jetty/util/SharedBlockingCallback.java:    private static Throwable SUCCEEDED = new Throwable()
jetty-util/src/main/java/org/eclipse/jetty/util/SharedBlockingCallback.java:    private static Throwable FAILED = new Throwable()
jetty-util/src/main/java/org/eclipse/jetty/util/FutureCallback.java:    private static Throwable COMPLETED=new Throwable();
jetty-util/src/main/java/org/eclipse/jetty/util/FuturePromise.java:    private static Throwable COMPLETED=new Throwable();
jetty-util/src/main/java/org/eclipse/jetty/util/BlockingCallback.java:    private static Throwable SUCCEEDED=new Throwable()

Actually, for JDK 7+ the fix is simple, PR in next comment.

[grgrzybek]

PR #869 uses subclasses of java.lang.Throwable that use 4 argument constructors, allowing to create exceptions without backtrace/stackTrace.

[sbordet]

@grgrzybek this is surprising. Actually, I am more inclined to think it's a JVM bug. Java application code should not bother about magic hidden fields, no matter what the application does.
For reference, see also: http://bugs.java.com/view_bug.do?bug_id=8033735

Even using the 4 arguments constructor, I don't see any guarantee that the backtrace field is not filled by some JVM code.

[grgrzybek]

@sbordet I checked before&after heap dumps and looks like it helps with ClassLoader actually being GCed - no references kept in Throwables created with writableStackTrace==false.

There's similar issue with JGit: https://github.com/eclipse/jgit/blob/master/org.eclipse.jgit/src/org/eclipse/jgit/errors/StopWalkException.java#L56 and same fix (4 argument Exception constructor) helps.

Of course I can't be sure some native code actually writes to this field, I'm also not sure how BacktraceBuilder class... But my fix looks promising for now.

Javadoc for 4 argument Throwable says:

If the writable stack trace is false, this constructor will not call fillInStackTrace(), a null will be written to the stackTrace field, and subsequent calls to fillInStackTrace and setStackTrace(StackTraceElement[]) will not set the stack trace. If the writable stack trace is false, getStackTrace will return a zero length array.

which is what we need - no Class instances in Throwable.

[sbordet]

@grgrzybek then rather than applying the 4 args constructor in all the places, I'd prefer to have a utility class, say ConstantThrowable that does the 4 args constructor, in module jetty-util, and then have all the other classes extend this one.

Can you comply with the required checks and update your PR ?

Thanks !

[grgrzybek]

@sbordet sure! I'll refactor private StaticThrowable classes from all required places and use single definition. Thanks for checking!

[grgrzybek]

@sbordet done - I've updated PR #869 for jetty-9.2.x branch, but also pushed 3 other to my fork - for 9.3.x, 9.4.x and master. Should I create PRs for these branches as well?

[sbordet]

@grgrzybek just do 9.2.x, we'll do the merge with the other branches.

[grgrzybek]

@sbordet done, PR #869 is being checked by Jenkins now and ip-validation already passed OK

[grgrzybek]

@janbartel thanks for merging!
I'll close this issue when fix is merged to 9.3.x, 9.4.x and master, ok?

[janbartel]

@grgrzybek I'll close it when I'm done merging. Thanks.

[janbartel]

Committed to jetty-9.2.x and merged to jetty-9.3.x jetty-9.4.x and master.

[grgrzybek]

@janbartel many many thanks!

[janbartel]

@grgrzybek thank you for the pull request.