Fix memory leak in tomcat application server #1701

rPraml · 2019-05-12T23:03:52Z

This PR relates to that post
https://groups.google.com/forum/#!topic/ebean/pfrnwNlahOU
(the post seems to be corrupt in the google forum)

It is a first draft, how to solve the memory/transaction leak problem. I have to test that in my environment, if the problem is gone now.
@rbygrave maybe you can give me some feedback of this implementation

rPraml · 2019-05-12T23:04:28Z

src/main/java/io/ebeaninternal/api/SpiTransactionProxy.java

@@ -325,7 +325,7 @@ public Object getUserObject(String name) {

  @Override
  public void depth(int diff) {
-    transaction.depth();
+    transaction.depth(diff);


I think this is an error, that I have found while debugging

Logged as: #1705

rPraml · 2019-05-12T23:23:35Z

src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java

-    protected synchronized TransactionMap initialValue() {
-      return new TransactionMap();
-    }
-  };


Here are several misconcepts, that prevents an application server to unload ebean/webappclassloader

the bracket initalization constructs an anonymous class, which holds a reference to our classloader

the initalValue is also a class from ebean, which holds also a reference to the classloader.

If such a reference is stuck in an other thread, the GC cannot remove the classloader on redeploy.
A java.util.HashMap can be cleared by the leakDetector, so that all references can go away.

rPraml · 2019-05-12T23:28:17Z

src/main/java/io/ebeaninternal/server/transaction/TransactionManager.java

-    scopeManager.set(t);
-    return t;
+    SpiTransaction transaction = createTransaction(false, -1);
+    return wrapAsScopedTransaction(transaction);


To get erverything working, I had to put only scopedTransaction in the DefaultTransactionScopeManager, respectively to the ThreadLocal.
When a non scoped transaction is put to the ThreadLocal, it will not get removed. It will only get inactive, so that it can be replaced the next run.
Do you think the additional overhead we will get here, is a problem?

rPraml · 2019-05-12T23:29:18Z

src/test/java/org/tests/transaction/TestExecuteComplete.java

+              Customer customer = Ebean.getReference(Customer.class, 42424242L);
+              Order order = new Order();
+              order.setCustomer(customer);
+              Ebean.save(order);


these tests were all wrong! They have to save "order" and not "customer"

rPraml · 2019-05-12T23:42:12Z

src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java

+  // we also use only maps from java.util, as these object may not trigger a
+  // class leaking issue in an application server if the maps are cleared by the
+  // leak detector
+  private static final ThreadLocal<Map<String, SpiTransaction>> local = new ThreadLocal<>();


I deleted TransactionMap and replaced it with a HashMap, which can be cleared on shutdown.

What were your intentions for using the serverName as key?
I think it would be better, to hold a ThreadLocal per server.
This could easily passed as "TransactionHolder" from the serverConfig here:
https://github.com/ebean-orm/ebean/blob/master/src/main/java/io/ebeaninternal/server/core/InternalConfiguration.java#L503

I deleted TransactionMap and replaced it with a HashMap

To get a PR in this area we need to focus on the minimal change and not try to include refactor/tidy up changes until we have the core change lined up and agreed.

…ed leak detection

jnehlmeier · 2019-05-15T11:26:28Z

src/main/java/io/ebean/config/ServerConfig.java

+  /**
+   * The Transaction leak detection.
+   */
+  private TransactionLeakDetection transactionLeakDetection = TransactionLeakDetection.COUNTER;


IMHO should be OFF by default. Leak detection is something you enable in a debug or development environment, not in production. Default settings should be sensible default settings for production.

good argument, there are currently 3 modes

OFF = really off

COUNTER = count how many transactions are started and stopped. Sum should be 0 on shutdown. Maintaining a counter should not add any noticeable overhead.

DETAIL = record details for debugging (add

So I thought COUNTER would be a good default value, because it would notify, if there are any detected leaks, but I can also change thhis to OFF.

Sure, but always think at scale. LongAdder maintains an internal array to avoid locking and contention. The size of the array reflects the amount of parallel count operations, which roughly represents the count of parallel transactions. So while it does not cost you a lot of performance, it will cost you memory. Given that transaction management is pretty crucial I would expect that most apps will not have transaction issues at all in production. So I think I would still prefer OFF as a default.

Also as a second note: If you assume that transaction leaks should be found during development and we are talking about web applications, then IMHO it would be preferable to have a leak output (if detected) per request or shortly after a request and not when undeploying the app. Could very well be, that during development, you simply hard restart the server instead of undeploying the app. So something like a servlet filter (or similar for other types of servers) might be more appropriate.
A different approach could also be to let the developer activate leak detection and then also let the developer specify a timeout after which the developer thinks a leak must had happened. That approach is for example used by HikariCP, a pretty good JDBC connection pool. The timeout you choose would be a little more than the slowest transaction in your app.

Unfortunately I don't want the TransactionLeakDetection in here at all.

That is, we already have this as part of Ebean's connection pool - that is the much better place to have it and I don't understand why that is part of this PR. It means this PR can't be merged as it is.

@rbygrave I agree and I'm also not very happy with the changes, as they touch one of the guts of ebean and I cannot test if it will work properly with spring/jta/....

The main issue was, that every thread is left with an inactive transaction object that were stuck in the ThreadLocal. And this will prevent tomcat from removing the web-app-classloader

This is also what the TransactionLeakDetector does: it detects stuck inactive transactions in the threadlocal object(s), so I tried to fix the root cause, so that this will not happen any more.

As workaround I can write some servlet filter, as from @jnehlmeier suggessted, which gurantees, that the ThreadLocal is removed after each web request (and may notify of leaked active transactions)

So I need a solution for my Classloader leak, because the old implementation left inactive transactions in the ThreadLocal (writing a servlet filter would be possible, but I see this as workaround and would like a clean implementation in ebean)

What do you suggest, how we should continue here?

Interesting - https://github.com/codesinthedark/ImprovedThreadLocal/blob/master/ImprovedThreadLocal.java

There are three possible solutions for the class loader leak:

A servlet filter that cleans ThreadLocals containing Ebean classes

Don't put Ebean classes into ThreadLocals but only JDK classes

Extract TransactionMap storage and have a ThreadLocal based implementation for classic applications and a request attribute based implementation (might require a Filter as well to get access to the request) when using Ebean in a server environment.

Wondering if anyone has used Ebean with async servlets yet. Because in that environment different threads can execute the same request but the transaction might life for the entire request. So storing transaction information in ThreadLocal is problematic in that case (the thread that starts processing the request and starts the transaction must not be the thread that finishes the request processing and wants to commit the transaction).

every thread is left with an inactive transaction object that were stuck in the ThreadLocal.

I think the goal is that under normal/good operation with each transaction committed or rolledback then DefaultTransactionThreadLocal is "ok for garbage collection".

Now, I believe we think there is a problem here because ...

private static final ThreadLocal<TransactionMap> local = new ThreadLocal<TransactionMap>() { @Override protected synchronized TransactionMap initialValue() { return new TransactionMap(); } };

... the above means there is anonymous class than references TransactionMap which is a class loaded by the same classloader as the anonymous class. This results in DefaultTransactionThreadLocal not being garbage collected even when TransactionMap has been removed by the commit() or rollback().

Is that the thinking?

@rbygrave you will have a leak if the ThreadLocal holds an instance of TransactionMap (that map can be empty or not, does not matter). So whenever you call local.get() without local.remove() / local.set(null) you have a problem.

Thanks @jnehlmeier - that is what I was thinking.
This PR replaces TransactionMap with HashMap - https://github.com/ebean-orm/ebean/pull/1704/files (a cut down version of your change @rPraml )

rbygrave · 2019-05-16T08:53:49Z

The second commit has changes that can not be merged. Do you want to redo this ?

rbygrave · 2019-05-16T08:55:00Z

src/main/java/io/ebeaninternal/server/core/DefaultServer.java

@@ -810,14 +810,7 @@ public Transaction beginTransaction(TxScope txScope) {
  public Transaction beginTransaction(TxIsolation isolation) {
    // start an explicit transaction
    SpiTransaction t = transactionManager.createTransaction(true, isolation.getLevel());
-    try {


Why? This was here for a reason etc.

rbygrave · 2019-05-16T08:56:58Z

src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java

+  // we also use only maps from java.util, as these object may not trigger a
+  // class leaking issue in an application server if the maps are cleared by the
+  // leak detector
+  private static final ThreadLocal<Map<String, SpiTransaction>> local = new ThreadLocal<>();


I deleted TransactionMap and replaced it with a HashMap

To get a PR in this area we need to focus on the minimal change and not try to include refactor/tidy up changes until we have the core change lined up and agreed.

rbygrave · 2019-05-16T08:58:17Z

src/main/java/io/ebeaninternal/server/transaction/DefaultTransactionThreadLocal.java

-  private static TransactionMap.State getState(String serverName) {
-    return local.get().getStateWithCreate(serverName);
+  public static void shutdown() {
+    leakDetector.detectLeaks();


shutdown() depending on the leak detection that I'm not sure I'd want ... can this not work without leakDetector etc?

…nThreadLocal

…nThreadLocal - Use ThreadLocal.withInitial to always have a map. - Remove the calls to local.remove(); when the map is empty (with the expectation that the thread local will GC with an empty map.

#1701 (v2) - Replace TransactionMap with HashMap in DefaultTransactionThreadLocal

rPraml · 2019-05-18T17:21:30Z

This is obsolete now, all relevant changes to fix the memory leak are included in #1711, #1714 and #1715. I also tested these changes in my application-server and the memory leak is gone now.

Ebean clears the threadLocal now in all known cases, so the autonatic leak detector is not included in the other PRs.

(Transaction leak detection and prevention, if really required, has to be done in application code, for example in a ServletFilter - See https://github.com/ebean-orm/ebean/blob/master/src/test/java/io/ebean/BaseTestCase.java#L43 to get an idea)

Closing without merge...

rPraml added 2 commits May 13, 2019 00:58

corrected the test cases

af39470

FIX: ClassLoader leak in DefaultTransactionThreadLocal

dc8bea4

rPraml commented May 12, 2019

View reviewed changes

rPraml added 3 commits May 14, 2019 22:12

changed to threadlocal per server/transaction manager

990ba61

changed the set method

1c8445d

using property in ServerConfig to control leak detection and refactor…

c520efc

…ed leak detection

jnehlmeier reviewed May 15, 2019

View reviewed changes

rbygrave reviewed May 16, 2019

View reviewed changes

rPraml mentioned this pull request May 16, 2019

Provide test cases for transaction leak problem #1703

Closed

rbygrave added a commit that referenced this pull request May 16, 2019

#1701 (v2) - Replace TransactionMap with HashMap in DefaultTransactio…

73990e3

…nThreadLocal

rbygrave added a commit that referenced this pull request May 17, 2019

Merge pull request #1704 from ebean-orm/feature/1701-v2

205f1f5

#1701 (v2) - Replace TransactionMap with HashMap in DefaultTransactionThreadLocal

rPraml closed this May 18, 2019

rPraml deleted the fix-memory-leak branch November 22, 2019 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak in tomcat application server #1701

Fix memory leak in tomcat application server #1701

rPraml commented May 12, 2019

rPraml May 12, 2019

rbygrave May 16, 2019

rPraml May 12, 2019

rPraml May 12, 2019

rPraml May 12, 2019

rPraml May 12, 2019

rbygrave May 16, 2019

jnehlmeier May 15, 2019

rPraml May 15, 2019

jnehlmeier May 15, 2019

rbygrave May 16, 2019

rPraml May 16, 2019

rbygrave May 16, 2019

jnehlmeier May 16, 2019

rbygrave May 16, 2019

jnehlmeier May 16, 2019

rbygrave May 16, 2019

rbygrave commented May 16, 2019

rbygrave May 16, 2019

rbygrave May 16, 2019

rbygrave May 16, 2019

rPraml commented May 18, 2019

Fix memory leak in tomcat application server #1701

Fix memory leak in tomcat application server #1701

Conversation

rPraml commented May 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbygrave commented May 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rPraml commented May 18, 2019