Repository: netty/netty
Issue: ObjectCleanerThread not be closed
Labels: defect

### Netty version
4.1.20.Final

### JVM version (e.g. `java -version`)
9.0.1

### OS version (e.g. `uname -a`)
Windows 10

Non-finished threads:

Thread[DestroyJavaVM,5,main]

Thread[ObjectCleanerThread,1,main]
  at java.base@9.0.1/java.lang.Object.wait(Native Method)
  at java.base@9.0.1/java.lang.ref.ReferenceQueue.remove(ReferenceQueue.java:151)
  at app//io.netty.util.internal.ObjectCleaner$1.run(ObjectCleaner.java:52)
  at app//io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
  at java.base@9.0.1/java.lang.Thread.run(Thread.java:844)


Issue comments:
@hank-whu it should be after 10 seconds maximum if there are no more references: https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/ObjectCleaner.java#L52

Are you sure there are no more references hold ?
@normanmaurer why isn't there a break in the loop if the thread is interrupted? wouldn't that indicate the application is shutting down?
@johnou I think it should still cary on cleanup until there are no more references as otherwise we may leak direct memory etc. It should only exit once the `LIVE_SET` is empty imho. 
@normanmaurer then how about a daemon thread? Allowing the JVM process to terminate in a timely manner.
@johnou we decided against a daemon thread to make harder guarantees about cleanup. If it would be a deamon thread we may leak direct memory for example when someone hot-deploy stuff. 

Also /cc @Scottmitch , as both of us had the discussion :)
Yes we considered daemon but decided against it. Although I would expect most OSes we run on will clean up process resources after the process dies we can’t be sure of this. Since we allocate native resources outside the scope of the JVM the more conservative approach is to go non daemon. The mitigation factor was to have a timeout on the poll. This may cause issues if some of the jvm shutdown processing contains references to items registered in the cleaner but this seemed unlikely when the discussion took place.
@normanmaurer 
shoud i wait 10 seconds after shutdown the EventLoopGroup ?
it doesn't seem like a good way.
can add a "shutdown(long mills)" method to ObjectCleanerThread ?
@hank-whu it may take up to 10 seconds... That said you can tune this with a system property:

https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/ObjectCleaner.java#L36
@normanmaurer it can actually stop the jvm from terminating, I'll post more details shortly. 
Thx... 

> Am 25.01.2018 um 09:27 schrieb Johno Crawford <notifications@github.com>:
> 
> @normanmaurer it can actually stop the jvm from terminating, I'll post more details shortly.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
> 

Ensure all threads are closed, "wait some time" is the only way.
I think this is not very good. 
I need a method to ensure all threads are closed.
@normanmaurer I would expect something like this to exit gracefully without an explicit System.exit call.

```java
public class Main {

    public static void main(String[] args) {
        ByteBuf byteBuf = PooledByteBufAllocator.DEFAULT.buffer();
        byteBuf.release();
        System.gc();
        System.runFinalization();
        // hangs because ObjectCleanerThread is not a daemon thread / LIVE_SET is not empty..
    }
}
```
@johnou @hank-whu after reading more up on all of this I think you are right and @Scottmitch and myself were wrong .... This must be a deamon thread to ensure we will be able to shutdown in all cases.  OpenJDK itself also uses a deamon thread and the GC threads are also deamon threads:
https://github.com/dmlloyd/openjdk/blob/342a565a2da8abd69c4ab85e285bb5f03b48b2c9/src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java#L117

Let me come up with a PR
@hank-whu @johnou https://github.com/netty/netty/pull/7627 PTAL
@johnou - interesting example! interesting implication of having the main thread running the `main` method is objects referenced from here may not be GC'ed, or at least not detectable by the application.
Fixed by https://github.com/netty/netty/pull/7627
Hi @normanmaurer Any ETA for the next release? This issue is hurting us pretty badly :(
Hopefully tomorrow, latest Friday. 

> On 31. Jan 2018, at 10:37, Stephane Landelle <notifications@github.com> wrote:
> 
> Hi @normanmaurer <https://github.com/normanmaurer> Any ETA for the next release? This issue is hurting us pretty badly :(
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/netty/netty/issues/7617#issuecomment-361876251>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa0Qs7Ok-9ASZLNkWHepi64goFZNXf_ks5tQDRxgaJpZM4RrB4W>.
> 


Wow, great, you rock!
Hi, is this fix included in any 4.1.x release yet?
@mcfongtw yep. since 4.1.21.
Thanks. @fenik17 

Pull request review comments:
never ending loop?
Yeah this needs to be removed, done..
@jaymode question... does this also need to be in a privileged block ?
This is a good question. `Thread#setDaemon` does check permissions by calling `Thread#checkAccess`, but I think it is important to look at what happens there before wrapping in a doPrivileged block. In the default [SecurityManager](https://docs.oracle.com/javase/7/docs/api/java/lang/SecurityManager.html#checkAccess(java.lang.Thread)), the access control is only enforced for system threads. In that case, I think this should not be wrapped since we are not modifying a system thread. In elasticsearch we use [SecureSM](https://github.com/elastic/securesm/blob/master/src/main/java/org/elasticsearch/SecureSM.java), which has a stricter policy but even then I don't think you'd want to elevate privileges here since modifying threads within your own threadgroup or one of its descendants is allowed.

@jaymode thanks a lot for the answer and sorry for bothering you but after your last PR I was wondering if I am doing something stupid ;)

Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/internal/ObjectCleaner.java b/common/src/main/java/io/netty/util/internal/ObjectCleaner.java
index 96d04439563..991f30d0dc0 100644
--- a/common/src/main/java/io/netty/util/internal/ObjectCleaner.java
+++ b/common/src/main/java/io/netty/util/internal/ObjectCleaner.java
@@ -16,8 +16,6 @@
 package io.netty.util.internal;
 
 import io.netty.util.concurrent.FastThreadLocalThread;
-import io.netty.util.internal.logging.InternalLogger;
-import io.netty.util.internal.logging.InternalLoggerFactory;
 
 import java.lang.ref.ReferenceQueue;
 import java.lang.ref.WeakReference;
@@ -36,11 +34,13 @@
 public final class ObjectCleaner {
     private static final int REFERENCE_QUEUE_POLL_TIMEOUT_MS =
             max(500, getInt("io.netty.util.internal.ObjectCleaner.refQueuePollTimeout", 10000));
+
+    // Package-private for testing
+    static final String CLEANER_THREAD_NAME = ObjectCleaner.class.getSimpleName() + "Thread";
     // This will hold a reference to the AutomaticCleanerReference which will be removed once we called cleanup()
     private static final Set<AutomaticCleanerReference> LIVE_SET = new ConcurrentSet<AutomaticCleanerReference>();
     private static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<Object>();
     private static final AtomicBoolean CLEANER_RUNNING = new AtomicBoolean(false);
-    private static final String CLEANER_THREAD_NAME = ObjectCleaner.class.getSimpleName() + "Thread";
     private static final Runnable CLEANER_TASK = new Runnable() {
         @Override
         public void run() {
@@ -116,9 +116,9 @@ public Void run() {
             });
             cleanupThread.setName(CLEANER_THREAD_NAME);
 
-            // This Thread is not a daemon as it will die once all references to the registered Objects will go away
-            // and its important to always invoke all cleanup tasks as these may free up direct memory etc.
-            cleanupThread.setDaemon(false);
+            // Mark this as a daemon thread to ensure that we the JVM can exit if this is the only thread that is
+            // running.
+            cleanupThread.setDaemon(true);
             cleanupThread.start();
         }
     }


Test patch:
diff --git a/common/src/test/java/io/netty/util/internal/ObjectCleanerTest.java b/common/src/test/java/io/netty/util/internal/ObjectCleanerTest.java
index 0db3e269456..7e755040211 100644
--- a/common/src/test/java/io/netty/util/internal/ObjectCleanerTest.java
+++ b/common/src/test/java/io/netty/util/internal/ObjectCleanerTest.java
@@ -23,6 +23,8 @@
 import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 
 public class ObjectCleanerTest {
 
@@ -110,4 +112,26 @@ public void run() {
             Thread.sleep(100);
         }
     }
+
+    @Test(timeout = 5000)
+    public void testCleanerThreadIsDaemon() throws Exception {
+        temporaryObject = new Object();
+        ObjectCleaner.register(temporaryObject, new Runnable() {
+            @Override
+            public void run() {
+                // NOOP
+            }
+        });
+
+        Thread cleanerThread = null;
+
+        for (Thread thread : Thread.getAllStackTraces().keySet()) {
+            if (thread.getName().equals(ObjectCleaner.CLEANER_THREAD_NAME)) {
+                cleanerThread = thread;
+                break;
+            }
+        }
+        assertNotNull(cleanerThread);
+        assertTrue(cleanerThread.isDaemon());
+    }
 }