CleanerThread is never cleaned up and causes memory leak

[carlosame]

As explained in BATIK-939, CleanerThread does not self-terminate and messes with garbage collection.

@jjYBdx4IL has provided a pull request to Apache Batik to fix BATIK-939 and I have been experimenting with a patch for EchoSVG based on it. Perhaps @jjYBdx4IL may want to provide his own PR but meanwhile my adapted patch looks like this:

In echosvg-util/src/main/java/io/sf/carte/echosvg/util/CleanerThread.java:

@@ -21,10 +21,12 @@ package io.sf.carte.echosvg.util;
 import java.lang.ref.PhantomReference;
 import java.lang.ref.Reference;
 import java.lang.ref.ReferenceQueue;
 import java.lang.ref.SoftReference;
 import java.lang.ref.WeakReference;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;

 /**
  * One line Class Desc
  *
  * Complete Class Desc
@@ -35,15 +37,16 @@ import java.lang.ref.WeakReference;
  */
 public class CleanerThread extends Thread {

    static volatile ReferenceQueue<Object> queue = null;
    static CleanerThread thread = null;
+   private static CountDownLatch shutdownLatch = new CountDownLatch(1);

    public static ReferenceQueue<Object> getReferenceQueue() {

-       if (queue == null) {
-           synchronized (CleanerThread.class) {
+       synchronized (CleanerThread.class) {
+           if (queue == null) {
                queue = new ReferenceQueue<>();
                thread = new CleanerThread();
            }
        }
        return queue;
@@ -95,27 +98,42 @@ public class CleanerThread extends Thread {
        start();
    }

    @Override
    public void run() {
-       while (true) {
-           try {
+       try {
+           /*
+            * Give a big enough timeout to account for busy VMs.
+            */
+           while(!shutdownLatch.await(4000, TimeUnit.MILLISECONDS)) {
                Reference<?> ref;
-               try {
-                   ref = queue.remove();
-                   // System.err.println("Cleaned: " + ref);
-               } catch (InterruptedException ie) {
-                   continue;
-               }
-
-               if (ref instanceof ReferenceCleared) {
-                   ReferenceCleared rc = (ReferenceCleared) ref;
-                   rc.cleared();
-               }
-           } catch (ThreadDeath td) {
-               throw td;
-           } catch (Throwable t) {
-               t.printStackTrace();
+               do {
+                   ref = queue.poll();
+                   if (ref instanceof ReferenceCleared) {
+                       ReferenceCleared rc = (ReferenceCleared) ref;
+                       rc.cleared();
+                   }
+               } while (ref != null);
+           }
+       } catch (ThreadDeath td) {
+           throw td;
+       } catch (InterruptedException ie) {
+           // Expected in this context, we do not want to print the
+           // stack trace
+       } catch (Throwable t) {
+           // Unexpected: print the stack trace
+           t.printStackTrace();
+       } finally {
+           // Release the latch in case it wasn't.
+           shutdownLatch.countDown();
+           // Synchronize to avoid a race with getReferenceQueue()
+           synchronized (CleanerThread.class) {
+               queue = null;
            }
        }
    }
+
+   public static void shutdown() {
+       shutdownLatch.countDown();
+   }
+
 }

and in echosvg-util/src/main/java/io/sf/carte/echosvg/util/HaltingThread.java:

@@ -30,11 +30,11 @@ package io.sf.carte.echosvg.util;
  */
 public class HaltingThread extends Thread {
    /**
     * Boolean indicating if this thread has ever been 'halted'.
     */
-   protected boolean beenHalted = false;
+   protected volatile boolean beenHalted = false;

    public HaltingThread() {
    }

    public HaltingThread(Runnable r) {

[jjYBdx4IL]

I don't think the InterruptedException should be caught here, let alone suppressed. It shouldn't be thrown in the first place if the thread shutdown is handled properly. Maybe we need to let shutdown() wait for the thread to be gone?

[carlosame]

I don't think the InterruptedException should be caught here, let alone suppressed.

Well that's what your patch does ;-) I'm just adding it explicitly. And the current implementation of CleanerThread catches that exception and does not rethrow it (it's the current "behavior contract" of the class).

It shouldn't be thrown in the first place if the thread shutdown is handled properly.

Not sure what's exactly "handling properly". According to the CountDownLatch.await documentation:

If the current thread: •has its interrupted status set on entry to this method; or •is interrupted while waiting, then InterruptedException is thrown and the current thread's interrupted status is cleared.

A thread being [interrupted](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Thread.html#interrupt()) is something that could happen. I don't think that you can do anything to avoid a thread (from a class that you do not control and that could be used in a way that you do not control) being interrupted.

[carlosame]

One of the differences between this project and Batik is that EchoSVG has a functioning and easy-to-run test suite based on JUnit. For example there is a RunnableQueueTest testing RunnableQueue but no test for CleanerThread.

If you can figure out a meaningful test that checks the behavior of CleanerThread, that would be awesome.

[jjYBdx4IL]

You can use any ttf2svg or rasterizer test I guess (esp the main classes), run it twice, and call shutdown() after each run (primarily the first). Additionally, you could manually check if the thread is gone,. though that might require a newer Java API.

I tried to import the gradle project into eclipse, but it doesn't seem to work properly. I see only the top project and Eclipse does not find any classes when I hit ctrl-shift-t.

[carlosame]

I tried to import the gradle project into eclipse, but it doesn't seem to work properly. I see only the top project and Eclipse does not find any classes when I hit ctrl-shift-t.

Weird, looks like it isn't finding the submodules. Did you generate a 7.0.2 wrapper and told Eclipse to use it? Are you using the latest software versions (JDK 16.0.1, latest Eclipse)?

I plan to put a wrapper once Gradle 7.1 is out, meanwhile it has to be generated manually (as explained in the README).

I use Eclipse myself, just have to close the echosvg-test-scripts module because it does not compile (could fix that but I do not care much about that module :-)). Here is a screenshot of how my Eclipse's left pane looks now:

The error at the top-level echosvg is caused by echosvg-test-scripts and is harmless.

You can use any ttf2svg or rasterizer test

Neither EchoSVG nor Batik have ttf2svg tests, and any test using current classes that implement ReferenceCleared is unlikely to be stressful enough. A proper unit test is needed so it can identify issues in the patch: the initial patch —as well as the above one— had issues, and the latest patch that you pushed to Batik still has problems.

I've come up with a patch that —in principle— has no obvious issues, but I'd rather not commit it without a test that I still don't have.

[carlosame]

Eclipse issues are generally related to the dual Java 8/11 compilation, which is based on my Gist:

Compile module-info separately in Gradle

You may want to open a separate issue if you encounter problems with Eclipse.

[jjYBdx4IL]

I was able to make stuff work (build) in Eclipse, except for that scripts test project, too.

How does io.sf.carte.echosvg.apps.rasterizer.MainTest interfere with gradle? When I try to run it in Eclipse, it gives

java.security.AccessControlException: access denied ("java.util.PropertyPermission" "java.security.policy" "write") at java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:472) at java.base/java.security.AccessController.checkPermission(AccessController.java:1036) at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:408) at java.base/java.lang.System.setProperty(System.java:911) at io.sf.carte.echosvg.util.ApplicationSecurityEnforcer.installSecurityManager(ApplicationSecurityEnforcer.java:247) at io.sf.carte.echosvg.util.ApplicationSecurityEnforcer.enforceSecurity(ApplicationSecurityEnforcer.java:165) at io.sf.carte.echosvg.apps.rasterizer.Main.execute(Main.java:895) at io.sf.carte.echosvg.apps.rasterizer.AbstractMainTest.runTest(MainTest.java:509) at io.sf.carte.echosvg.apps.rasterizer.MainTest.testMain(MainTest.java:95) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:567) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306) at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63) at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329) at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293) at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306) at org.junit.runners.ParentRunner.run(ParentRunner.java:413) at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:89) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:541) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:768) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:464) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:210)

[carlosame]

How does io.sf.carte.echosvg.apps.rasterizer.MainTest interfere with gradle? When I try to run it in Eclipse, it gives

Which operating system are you using? On Windows 10, I run MainTest without issues from Eclipse (does not work from Gradle).

You may want to continue the conversation at issue #19, or open a new one.

[carlosame]

Just added this to the README:

Note: when running tests from the Eclipse IDE, it is recommended to run them as
a "JUnit Test" in the "Run As" menu option. If you run them as "Gradle Test" you
may encounter well-known security-related issues (issue #19).

[carlosame]

I'm committing my patch, no specific test yet but appears to work well in practice.

Although feedback is always welcome, I suggest @jjYBdx4IL to not look at it if you want to pursue your Batik PR (moreover my patch is quite different to yours). Keep in mind that nothing from EchoSVG can be contributed to Batik, due to the ASF requirement that all the copyrights be assigned to them.

In this project, contributors keep their own copyright unless they previously assigned it to somebody else (like the ASF). There are more details in the CONTRIBUTING document.