jakartaee / expression-language

Jakarta Expression Language
https://eclipse.org/ee4j/el
Other
62 stars 49 forks source link

ELParser PermGen Memory Leak #136

Closed jasonex7 closed 3 years ago

jasonex7 commented 3 years ago

Class com.sun.el.parser.ELParser defines a static final java.lang.Error which could cause PermGen memory leaks (see reference: https://bugs.openjdk.java.net/browse/JDK-8146961 and https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8158237)

https://github.com/eclipse-ee4j/el-ri/blob/288f7cd30d26c8541d71e62ffa9fef222c1f9990/impl/src/main/java/com/sun/el/parser/ELParser.java#L2876

markt-asf commented 3 years ago

That is a JVM bug not an EL bug. I'm not at all convinced of the need to work-around a JVM bug that should be fixed in any recent Java version. Further, the code in question is generated code which complicates a work-around (if we wanted to go that route). I'm leaning heavily towards WONTFIX as a resolution for this.

jasonex7 commented 3 years ago

Thanks for your comment. The description of the JDK issue is pointing to 2 java classes (com.sun.org.apache.xerces.internal.dom.DOMNormalizer and com.sun.org.apache.xml.internal.serialize.DOMSerializerImpl) that also defines a static final java.lang.RuntimeException which could lead to a PermGen Memory Leak.

A DESCRIPTION OF THE PROBLEM : The classes com.sun.org.apache.xerces.internal.dom.DOMNormalizer and com.sun.org.apache.xml.internal.serialize.DOMSerializerImpl, both within rt.jar, each contain a static final field of type RuntimeException named 'abort'.

When each of these two classes is statically initialized, its 'abort' exception is created, and its stacktrace is filled in. If a class in a web application is unfortunate enough to be in the call stack when this happens, that class can no longer be garbage collected as the exception's stack trace now contains a reference to this class. Consequently, this class, and all of the other classes loaded by the web app's classloader, cannot be removed from PermGen space within the JVM. This causes a memory leak.

I would suggest the following fix: (1): Create a custom subclass of RuntimeException, which overrides fillInStackTrace to just return this. Use an instance of this subclass instead of a RuntimeException for DOMNormalizer.abort. (This appears to be the approach taken for two other static final fields which also contain exceptions: com.sun.org.apache.xerces.internal.impl.XMLEntityScanner.END_OF_DOCUMENT_ENTITY and com.sun.org.apache.xerces.internal.parsers.AbstractDOMParser$Abort.INSTANCE. Neither of these exceptions causes the same problem.) (2): Delete the field com.sun.org.apache.xml.internal.serialize.DOMSerializerImpl.abort, as it appears to be unused.

The JDK issue was resolved by removing the declaration of the static final Exception variable in both classes, so I think ELParser should align to that.

I'm using WildFly 20.0.1 with JDK 8u202 (the JDK issue I'm referring was solved in JDK 8u152) and I'm experiencing that PermGen memory leak

image

markt-asf commented 3 years ago

I thought the issue was fixed more generally but I must be thinking of one of the other memory leaks that were fixed around the same time. That this is generated code makes things a little more complicated. First step is to see if this is configurable and/or has been fixed upstream in the code generation tool.

jasonex7 commented 3 years ago

I think the problem is in these added lines:

https://github.com/eclipse-ee4j/el-ri/blob/288f7cd30d26c8541d71e62ffa9fef222c1f9990/impl/build.xml#L35-L41

The original project doesn't have those lines in build.xml (https://github.com/javaee/el-spec/blob/master/impl/build.xml), so the generated LookaheadSuccess jj_ls variable was only final instead of static final, as you can see in this line: https://github.com/javaee/el-spec/blob/28d07a5c64556866e925a55569b547a5a570588a/impl/src/main/java/com/sun/el/parser/ELParser.java#L2849

markt-asf commented 3 years ago

The generated code should create a static and the memory leak has been fixed in the code generator (JavaCC) for a number of years. Given the context, the replacement looks odd in several ways. A review of the history of that code may be enlightening.

jasonex7 commented 3 years ago

I've generated ELParser.java and the LookaheadSuccess variable was created as final only. The "static" modifier was added after the code generator (JavaCC) had been executed. I found the commit that add those build.xml lines: https://github.com/javaee/uel-ri/commit/d2f0cbf02ba29f28a21f5a1ec78836d1edcafa2c but I couldn't find details about WLS (seems stands for WebLogicServer) bug 26801105.

Also I've generated a custom build of EE4J_8 branch (jakarta.el-3.0.2-SNAPSHOT.jar) without the static modifier, put that jar in WildFly 20 and repeat my test and the memory leak has gone.

image

markt-asf commented 3 years ago

That looks like a very good reason to revert that WLS bug fix. Whatever the bug was, it will need to be fixed in WLS not in this EL implementation.

jasonex7 commented 3 years ago

I've created 2 pull requests for revert that commit: one on master and another one in 3.x branch.

lprimak commented 3 years ago

@markt-asf This bug appears on Payara as well. EL is the proper place to fix it. Please merge this fix. @arjantijms thank you