Introduce ObjectCleaner and use it in FastThreadLocal to ensure FastThreadLocal.onRemoval(...) is called by normanmaurer · Pull Request #7463 · netty/netty

normanmaurer · 2017-12-01T15:08:32Z

Motivation:

There is no guarantee that FastThreadLocal.onRemoval(...) is called if the FastThreadLocal is used by "non" FastThreacLocalThreads. This can lead to all sort of problems, like for example memory leaks as direct memory is not correctly cleaned up etc.

Beside this we use ThreadDeathWatcher to check if we need to release buffers back to the pool when thread local caches are collected. In the past ThreadDeathWatcher was used which will need to "wakeup" every second to check if the registered Threads are still alive. If we can ensure FastThreadLocal.onRemoval(...) is called we do not need this anymore.

Modifications:

- Introduce ObjectCleaner and use it to ensure FastThreadLocal.onRemoval(...) is always called when a Thread is collected.
- Deprecate ThreadDeathWatcher
- Add unit tests.

Result:

Consistent way of cleanup FastThreadLocals when a Thread is collected.

normanmaurer · 2017-12-01T15:09:44Z

There is still some TODO to fix ... Just wanted to show the idea

johnou · 2017-12-01T16:12:49Z

@normanmaurer could the same approach be used with Recycler instead of finalize?

normanmaurer · 2017-12-01T16:15:40Z

@johnou not sure what you refer to with finalize.... any pointers?

johnou · 2017-12-01T16:20:51Z

@normanmaurer

io.netty.util.Recycler.WeakOrderQueue#finalize

        @Override
        protected void finalize() throws Throwable {
            try {
                super.finalize();
            } finally {
                // We need to reclaim all space that was reserved by this WeakOrderQueue so we not run out of space in
                // the stack. This is needed as we not have a good life-time control over the queue as it is used in a
                // WeakHashMap which will drop it at any time.
                Link link = head;
                while (link != null) {
                    reclaimSpace(LINK_CAPACITY);
                    link = link.next;
                }
            }
        }

normanmaurer · 2017-12-01T16:25:05Z

Yes most likely

…

Am 01.12.2017 um 17:20 schrieb Johno Crawford ***@***.***>: @normanmaurer io.netty.util.Recycler.WeakOrderQueue#finalize @OverRide protected void finalize() throws Throwable { try { super.finalize(); } finally { // We need to reclaim all space that was reserved by this WeakOrderQueue so we not run out of space in // the stack. This is needed as we not have a good life-time control over the queue as it is used in a // WeakHashMap which will drop it at any time. Link link = head; while (link != null) { reclaimSpace(LINK_CAPACITY); link = link.next; } } } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ejona86

I don't see how this would avoid ClassLoader leaks. In fact, it seems strictly worse.

As I mentioned in #7446 (comment), I think there is a fundamental issue with cleaning anything retained from a static via a background-based cleaner.

ejona86 · 2017-12-01T17:44:17Z

common/src/main/java/io/netty/util/internal/AutomaticCleanupReference.java

+                Thread cleanupThread = new Thread(new Runnable() {
+                    @Override
+                    public void run() {
+                        for (;;) {


So this runs forever? Isn't that guaranteed to be a classloader leak via this Runnable?

@ejona86 yes... I will fix this soon :)

ejona86

This seems fine to me, other than the ThreadCleaner looping forever. And actually, starting a thread from a static initializer also seems a bit worrisome.

ejona86 · 2017-12-12T19:20:07Z

buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java

+                }
+                return cache;
            }
            // No caching for non FastThreadLocalThreads.


This comment looks out-of-date. Remove?

ejona86 · 2017-12-12T19:50:32Z

common/src/main/java/io/netty/util/internal/ThreadCleaner.java

s/htraded/threaded/

ejona86 · 2017-12-12T19:56:30Z

common/src/main/java/io/netty/util/internal/ThreadCleaner.java

nit: s/its/it's/

ejona86 · 2017-12-12T19:56:48Z

common/src/main/java/io/netty/util/internal/ThreadCleaner.java

nit: there's a double space

ejona86 · 2017-12-12T19:57:12Z

common/src/main/java/io/netty/util/internal/ThreadCleaner.java

s/arleady/already/

ejona86 · 2017-12-12T20:02:57Z

I guess the failure case is the ThreadLocal instance could be thrown away, and let the storage evaporate (since Thread uses WeakReference for the mapping), but the Thread itself remain running.

So, this turned out to be semi-wrong. Or rather, if you leave a Thread running, "you're gonna have a bad time." I realized that even using finalize() you can't necessarily clean up resources if the Thread lives because ThreadLocal is only lazily cleaned up in the ThreadLocalMap. Any values held are strong references, and so they might be retained for the rest of the Thread's lifetime.

So I'm not trying to solve that any more :-). And that makes multiple solutions we've considered solvable. The only thing we need to be wary of is clearing the Thread.contextClassLoader.

johnou · 2017-12-12T22:09:17Z

buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java

@@ -440,14 +441,26 @@ protected synchronized PoolThreadCache initialValue() {
            if (useCacheForAllThreads || current instanceof FastThreadLocalThread) {
                // If our FastThreadLocalThread will call FastThreadLocal.removeAll() we not need to use
                // the ThreadDeathWatcher to release memory from the PoolThreadCache once the Thread dies.


update javadoc?

+1 ... Let me do this

johnou · 2017-12-12T22:10:54Z

common/src/main/java/io/netty/util/Recycler.java

+
+        // We store the Thread in a WeakReference as otherwise we may be the only ones that still hold a strong
+        // Reference to the Thread itself after it died.
+        final WeakReference<Thread> threadRef;


should this be in the same pull request?

I did it in the same as I noticed it here because the Thread was never GCed. Maybe I should do it in a separate PR tho..

@johnou #7499 ... Even added a test-case :)

johnou · 2017-12-12T22:12:49Z

common/src/main/java/io/netty/util/internal/ThreadCleaner.java

+ * Allows a way to register some {@link Runnable} that will executed once there are no references to the {@link Thread}
+ * anymore. This typically happens once the {@link Thread} dies / completes.
+ */
+public final class ThreadCleaner {


no phantom in the end? :)

nope... WeakReference was good enough.

carl-mastrangelo · 2017-12-13T00:06:52Z

Out of curiosity: why not spawn the cleaner thread when the original thread goes away, rather than when it is first attached? That would keep the number of active threads down.

Also, why not do the cleanup in a finalize() method? This is actually garbage collection, so it seems reasonable to do it there and not spawn another thread.

normanmaurer · 2017-12-13T06:22:14Z

@carl-mastrangelo answers / comments below....

Out of curiosity: why not spawn the cleaner thread when the original thread goes away, rather than when it is first attached? That would keep the number of active threads down.

How would you detect that the original Thread goes away ? Also I think its not so bad to have one extra thread that is doing the cleanup for everything. I will most likely use it in other places as well at some point.

Also, why not do the cleanup in a finalize() method? This is actually garbage collection, so it seems reasonable to do it there and not spawn another thread.

I actually had a PR (#7446) with a finalize() at some point but @ejona86 asked me to not do this as finalize() is marked as @deprecated in Java9 and so should not be used anymore.

johnou · 2017-12-13T06:55:02Z

@carl-mastrangelo Object.finalize is deprecated, the recommended replacements are PhantomReference and the new Cleaner class.

normanmaurer · 2017-12-13T10:05:01Z

@johnou @ejona86 @carl-mastrangelo PTAL again as I changed the approach a bit in a followup commit that I pushed. So the "real problem" why we needed all of this is because of the user uses a non FastThreadLocalThread there is no guarantee that we will call FastThreadLocal.removeAll() at all once the Thread goes away. The cleanup of the PoolThreadCache is done in FastThreadLocal.onRemove(...) that will be triggered by the removeAll().

Because of this I now moved the usage of ThreadCleaner.register(...) into the FastThreadLocal itself which has the affect that we are guaranteed that onRemoval(...) is always called (even if we use a non FastThreadLocal. I think this is a more general fix to the problem and also fixes a possible memory leak that could happen NativeDatagramPacketArray if non FastThreadLocal threads are used to access it as onRemoval would never been called before for these:

https://github.com/netty/netty/blob/4.1/transport-native-epoll/src/main/java/io/netty/channel/epoll/NativeDatagramPacketArray.java#L43

WDYT ?

ejona86 · 2017-12-13T18:47:21Z

common/src/main/java/io/netty/util/concurrent/FastThreadLocal.java

        }
    }

+    private static void removeAll(InternalThreadLocalMap threadLocalMap) {


Is there a reason this was split out into its own function?

I had expected the Runnable passed to ThreadCleaner would use it, but it uses remove(...) instead. It's a bit hard to determine what's the difference between the two methods.

Actually sorry it should use removeAll and I messed up :(

Sorry what I wanted to say was that we not need to factor out removeAll and can just call remove... will fix it.

ejona86 · 2017-12-13T18:53:24Z

Because of this I now moved the usage of ThreadCleaner.register(...) into the FastThreadLocal itself which has the affect that we are guaranteed that onRemoval(...) is always called (even if we use a non FastThreadLocal

It also means that you'll use the ThreadCleaner even if nothing stored in the FastThreadLocal needs cleaning. Normally that'd be rare, but if caching is disabled for non-FastThreadLocalThreads, then it'd be more frequent. Not sure it matters at all.

normanmaurer · 2017-12-13T18:57:57Z

@ejona86 yeah I agree... That said I think it may be still the preferred way to fix it and ensure onRemoval is always called and so can depend on it... WDYT ?

carl-mastrangelo · 2017-12-15T04:57:17Z

common/src/main/java/io/netty/util/concurrent/FastThreadLocalThread.java

+     */
+    @UnstableApi
+    public static boolean willCleanupFastThreadLocals(Thread thread) {
+        return thread instanceof FastThreadLocalThread && ((FastThreadLocalThread) thread)


might be clearer to break on &&

carl-mastrangelo · 2017-12-15T04:58:23Z

common/src/main/java/io/netty/util/internal/ThreadCleaner.java

+    // This will hold a reference to the ThreadCleanerReference which will be removed once we called cleanup()
+    private static final Set<ThreadCleanerReference> LIVE_SET = new ConcurrentSet<ThreadCleanerReference>();
+    private static final ReferenceQueue<Object> REFERENCE_QUEUE = new ReferenceQueue<Object>();
+    private static final AtomicBoolean CLEANER_RUNNING = new AtomicBoolean(false);


nit: caps are usually for constants.

I think CAPS are for things that are static final so I think this is fine. Sure the content can be changed but this is ok IMHO.

carl-mastrangelo · 2017-12-15T05:00:32Z

common/src/main/java/io/netty/util/internal/ThreadCleaner.java

+            cleanupThread.setName("ThreadCleanerReaper");
+
+            // This Thread is not a daemon as it will die once all references to the registered Threads will go away.
+            cleanupThread.setDaemon(false);


Daemon is for not blocking shutdown. In a shutdown scenario, how important is it to clean up refs?

actually I think we could also just use a non-daemon thread as the thread will go away once there is no references anymore.

ejona86 · 2017-12-15T07:16:15Z

That said I think it may be still the preferred way to fix it and ensure onRemoval is always called and so can depend on it... WDYT ?

I'm fine with it. It might be unnecessary in some cases, but doesn't seem like it hurts much and does remove a type of bug-prone behavior.

normanmaurer · 2017-12-15T14:38:26Z

@carl-mastrangelo PTAL again

Scottmitch

few questions ... GC sensitive tests make me 😢 but not sure of a better alternative.

Scottmitch · 2017-12-19T00:59:08Z

common/src/main/java/io/netty/util/internal/ThreadCleaner.java

+        LIVE_SET.add(reference);
+
+        // Check if there is already
+        if (CLEANER_RUNNING.compareAndSet(false, true)) {


we should able to avoid the CAS operation most of the time by doing a read first because I assume should be true more often than not?

again I think we should not care as this should not happen very frequently and the CAS is the thing that I would be concerned last about (as the other stuff will be the "expensive")

fair enough ... I guess even if folks are doing get/remove this may not buy much.

Scottmitch · 2017-12-19T01:03:58Z

common/src/main/java/io/netty/util/internal/ThreadCleaner.java

+     * This should only be used if there are no other ways to execute some cleanup once the {@link Thread} dies as
+     * it is not a cheap way to handle the cleanup.
+     */
+    public static void register(Thread thread, Runnable cleanupTask) {


this class is internal ... consider:

exposing a method where the user can directly create a ThreadCleanerReference to avoid allocating a Runnable and ThreadCleanerReference

Make the class package private and you can use the same technique as above to reduce object allocation.

I am not concerned about this as this should be something that is done very frequently and I think this design is cleaner.

Agreed the current APIs are nicer. However it is internal and doesn't need to be generic right? We could potentially save 1 object allocation per thread local. Since it is internal we can always change it later but maybe nice to keep thread local overhead down if we don't have to be generic.

@Scottmitch as discussed offline I will merge it as it is for now and we can address this if we find out its an issue later on. The current design is nicer :)

sgtm ... internal == change later if necessary

exactly 👍

Scottmitch · 2017-12-19T01:06:05Z

common/src/main/java/io/netty/util/internal/ThreadCleaner.java

+                }
+            }
+            if (interrupted) {
+                // As we catched the InterruptedException above we should mark the Thread as interrupted.


catched -> caught

normanmaurer · 2017-12-19T13:30:30Z

@Scottmitch yeah I think how we test here is the best we can do for now...

Scottmitch

1 suggestion (#7463 (comment)) but lgtm

carl-mastrangelo · 2017-12-20T01:37:19Z

cc: @zpencer

…hreadLocal.onRemoval(...) is called Motivation: There is no guarantee that FastThreadLocal.onRemoval(...) is called if the FastThreadLocal is used by "non" FastThreacLocalThreads. This can lead to all sort of problems, like for example memory leaks as direct memory is not correctly cleaned up etc. Beside this we use ThreadDeathWatcher to check if we need to release buffers back to the pool when thread local caches are collected. In the past ThreadDeathWatcher was used which will need to "wakeup" every second to check if the registered Threads are still alive. If we can ensure FastThreadLocal.onRemoval(...) is called we do not need this anymore. Modifications: - Introduce ObjectCleaner and use it to ensure FastThreadLocal.onRemoval(...) is always called when a Thread is collected. - Deprecate ThreadDeathWatcher - Add unit tests. Result: Consistent way of cleanup FastThreadLocals when a Thread is collected.

normanmaurer · 2017-12-20T13:25:48Z

@mosesn you may be interested as well.

normanmaurer · 2017-12-21T06:39:15Z

Cherry-picked into 4.1 (e329ca1) and 4.0 (00ae627)

zpencer · 2017-12-21T18:02:13Z

common/src/test/java/io/netty/util/concurrent/FastThreadLocalTest.java

+        Runnable runnable = new Runnable() {
+            @Override
+            public void run() {
+                assertEquals(Thread.currentThread().getName(), threadLocal.get());


A bit late to this PR, but what happens if code sets a FastThreadLocal and the thread exits before it's read? It looks like the cleaner code is only registered on the get path.

@zpencer good point. Let me fix this.

@zpencer #7534 PTAL

normanmaurer mentioned this pull request Dec 1, 2017

Dont use the ThreadDeathWatcher to release memory back to PoolArena a… #7446

Closed

normanmaurer force-pushed the cleanup_phantom branch from 9b1ca75 to 15a1c67 Compare December 1, 2017 15:37

ejona86 requested changes Dec 1, 2017

View reviewed changes

normanmaurer force-pushed the cleanup_phantom branch from 15a1c67 to afc3aa0 Compare December 12, 2017 19:03

ejona86 reviewed Dec 12, 2017

View reviewed changes

ejona86 approved these changes Dec 12, 2017

View reviewed changes

normanmaurer force-pushed the cleanup_phantom branch from fa6d1d7 to 6e971bc Compare December 12, 2017 20:08

normanmaurer changed the title ~~Introduce AutomaticCleanupReference and use it PooledByteBufAllocator…~~ Introduce ThreadCleaner and use it in PooledByteBufAllocator to cleanup ThreadLocal caches. Dec 12, 2017

normanmaurer requested review from Scottmitch and carl-mastrangelo December 12, 2017 20:10

johnou suggested changes Dec 12, 2017

View reviewed changes

johnou reviewed Dec 12, 2017

View reviewed changes

ejona86 approved these changes Dec 13, 2017

View reviewed changes

normanmaurer mentioned this pull request Dec 13, 2017

Ensure Thread can be collected in a timely manner if Recycler.Stack h… #7499

Closed

normanmaurer force-pushed the cleanup_phantom branch from 9a536e4 to f560ac8 Compare December 14, 2017 05:51

carl-mastrangelo reviewed Dec 15, 2017

View reviewed changes

normanmaurer force-pushed the cleanup_phantom branch 2 times, most recently from a232792 to 18eedc6 Compare December 15, 2017 14:37

johnou approved these changes Dec 15, 2017

View reviewed changes

normanmaurer requested a review from trustin December 18, 2017 08:42

Scottmitch reviewed Dec 19, 2017

View reviewed changes

johnou approved these changes Dec 19, 2017

View reviewed changes

normanmaurer force-pushed the cleanup_phantom branch from 4a997ed to c756364 Compare December 19, 2017 14:42

Scottmitch approved these changes Dec 19, 2017

View reviewed changes

normanmaurer force-pushed the cleanup_phantom branch from c756364 to f14cfdc Compare December 20, 2017 10:16

normanmaurer changed the title ~~Introduce ThreadCleaner and use it in PooledByteBufAllocator to cleanup ThreadLocal caches.~~ Introduce ObjectCleaner and use it in FastThreadLocal to ensure FastThreadLocal.onRemoval(...) is called Dec 20, 2017

normanmaurer closed this Dec 21, 2017

normanmaurer deleted the cleanup_phantom branch December 21, 2017 06:39

zpencer reviewed Dec 21, 2017

View reviewed changes

jasontedor mentioned this pull request Jun 10, 2018

Upgrade to Netty 4.1.25.Final elastic/elasticsearch#31232

Merged

Uh oh!

Conversation

normanmaurer commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

normanmaurer commented Dec 1, 2017

Uh oh!

johnou commented Dec 1, 2017

Uh oh!

normanmaurer commented Dec 1, 2017

Uh oh!

johnou commented Dec 1, 2017

Uh oh!

normanmaurer commented Dec 1, 2017 via email

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Dec 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carl-mastrangelo commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

normanmaurer commented Dec 13, 2017

Uh oh!

johnou commented Dec 13, 2017

Uh oh!

normanmaurer commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Dec 13, 2017

Uh oh!

normanmaurer commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Dec 1, 2017 •

edited

Loading

carl-mastrangelo commented Dec 13, 2017 •

edited

Loading