This is an automated email from the ASF dual-hosted git repository. kwin pushed a commit to branch bugfix/SLING-7432 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-commons-threads.git
commit 30ea237a5c2a2e7d77cf950721762aa0beb26880 Author: Konrad Windszus <[email protected]> AuthorDate: Wed Jan 24 17:02:06 2018 +0100 SLING-7432 no longer rely on thread locals themselves to store old thread local state add dump method for debugging purposes --- .../commons/threads/impl/ThreadLocalCleaner.java | 73 ++++++++++++++-------- .../ThreadPoolExecutorCleaningThreadLocals.java | 6 ++ ...ThreadPoolExecutorCleaningThreadLocalsTest.java | 7 ++- 3 files changed, 58 insertions(+), 28 deletions(-) diff --git a/src/main/java/org/apache/sling/commons/threads/impl/ThreadLocalCleaner.java b/src/main/java/org/apache/sling/commons/threads/impl/ThreadLocalCleaner.java index b755034..ab72c9f 100644 --- a/src/main/java/org/apache/sling/commons/threads/impl/ThreadLocalCleaner.java +++ b/src/main/java/org/apache/sling/commons/threads/impl/ThreadLocalCleaner.java @@ -21,6 +21,7 @@ import java.lang.reflect.Field; import java.util.Arrays; import org.apache.sling.commons.threads.impl.ThreadLocalChangeListener.Mode; +import org.slf4j.Logger; /** Notifies a {@link ThreadLocalChangeListener} about changes on a thread local storage. In addition it removes all references to variables * being added to the thread local storage while the cleaner was running with its {@link cleanup} method. @@ -47,7 +48,6 @@ public class ThreadLocalCleaner { private static Field threadLocalMapThresholdField; private static volatile IllegalStateException reflectionException; - public ThreadLocalCleaner(ThreadLocalChangeListener listener) { if (threadLocalsField == null || reflectionException != null) { initReflectionFields(); @@ -75,16 +75,40 @@ public class ThreadLocalCleaner { } catch (NoSuchFieldException e) { reflectionException = new IllegalStateException( "Could not locate threadLocals field in class Thread. " + - "Will not be able to clear thread locals: " + e, e); + "Will not be able to clear thread locals: " + e, + e); throw reflectionException; } } } + /** This is only for debugging purposes. Gives out all thread locals bound to the current thread to the logger. + * + * @param log + * @throws IllegalArgumentException + * @throws IllegalAccessException */ + void dump(Logger log) { + Thread thread = Thread.currentThread(); + Object threadLocals; + try { + threadLocals = threadLocalsField.get(thread); + Reference<?>[] currentReferences = (Reference<?>[]) tableField.get(threadLocals); + int size = (int) threadLocalMapSizeField.get(threadLocals); + log.info("Found {} thread locals bound to thread {}", size, thread); + for (Reference<?> curRef : currentReferences) { + if (curRef != null) { + log.info("Found reference {} with value {}", (ThreadLocal<?>) curRef.get(), threadLocalEntryValueField.get(curRef)); + } + } + } catch (IllegalArgumentException | IllegalAccessException e) { + log.error("Can not dump thread locals for thread {}: {}", thread, e, e); + } + } + public void cleanup() { // the first two diff calls are only to notify the listener, the actual cleanup is done by restoreOldThreadLocals - diff(threadLocalsField, copyOfThreadLocals.get()); - diff(inheritableThreadLocalsField, copyOfInheritableThreadLocals.get()); + diff(threadLocalsField, copyOfThreadLocals); + diff(inheritableThreadLocalsField, copyOfInheritableThreadLocals); restoreOldThreadLocals(); } @@ -175,20 +199,20 @@ public class ThreadLocalCleaner { "Could not find inner class " + name + " in " + clazz); } - private static final ThreadLocal<Reference<?>[]> copyOfThreadLocals = new ThreadLocal<>(); - private static final ThreadLocal<Integer> copyOfThreadLocalsSize = new ThreadLocal<>(); - private static final ThreadLocal<Integer> copyOfThreadLocalsThreshold = new ThreadLocal<>(); - private static final ThreadLocal<Reference<?>[]> copyOfInheritableThreadLocals = new ThreadLocal<>(); - private static final ThreadLocal<Integer> copyOfInheritableThreadLocalsSize = new ThreadLocal<>(); - private static final ThreadLocal<Integer> copyOfInheritableThreadLocalsThreshold = new ThreadLocal<>(); - - private static void saveOldThreadLocals() { - copyOfThreadLocals.set(copy(threadLocalsField)); - copyOfThreadLocalsSize.set(size(threadLocalsField, threadLocalMapSizeField)); - copyOfThreadLocalsThreshold.set(size(threadLocalsField, threadLocalMapThresholdField)); - copyOfInheritableThreadLocals.set(copy(inheritableThreadLocalsField)); - copyOfInheritableThreadLocalsSize.set(size(inheritableThreadLocalsField, threadLocalMapSizeField)); - copyOfInheritableThreadLocalsThreshold.set(size(inheritableThreadLocalsField, threadLocalMapThresholdField)); + private Reference<?>[] copyOfThreadLocals; + private Integer copyOfThreadLocalsSize; + private Integer copyOfThreadLocalsThreshold; + private Reference<?>[] copyOfInheritableThreadLocals; + private Integer copyOfInheritableThreadLocalsSize; + private Integer copyOfInheritableThreadLocalsThreshold; + + private void saveOldThreadLocals() { + copyOfThreadLocals = copy(threadLocalsField); + copyOfThreadLocalsSize = size(threadLocalsField, threadLocalMapSizeField); + copyOfThreadLocalsThreshold = size(threadLocalsField, threadLocalMapThresholdField); + copyOfInheritableThreadLocals = copy(inheritableThreadLocalsField); + copyOfInheritableThreadLocalsSize = size(inheritableThreadLocalsField, threadLocalMapSizeField); + copyOfInheritableThreadLocalsThreshold = size(inheritableThreadLocalsField, threadLocalMapThresholdField); } private static Reference<?>[] copy(Field field) { @@ -216,15 +240,14 @@ public class ThreadLocalCleaner { } } - private static void restoreOldThreadLocals() { + private void restoreOldThreadLocals() { try { - restore(inheritableThreadLocalsField, copyOfInheritableThreadLocals.get(), - copyOfInheritableThreadLocalsSize.get(), copyOfInheritableThreadLocalsThreshold.get()); - restore(threadLocalsField, copyOfThreadLocals.get(), - copyOfThreadLocalsSize.get(), copyOfThreadLocalsThreshold.get()); + restore(inheritableThreadLocalsField, copyOfInheritableThreadLocals, + copyOfInheritableThreadLocalsSize, copyOfInheritableThreadLocalsThreshold); + restore(threadLocalsField, copyOfThreadLocals, + copyOfThreadLocalsSize, copyOfThreadLocalsThreshold); } finally { - copyOfThreadLocals.remove(); - copyOfInheritableThreadLocals.remove(); + } } diff --git a/src/main/java/org/apache/sling/commons/threads/impl/ThreadPoolExecutorCleaningThreadLocals.java b/src/main/java/org/apache/sling/commons/threads/impl/ThreadPoolExecutorCleaningThreadLocals.java index 922f18b..29dfde4 100644 --- a/src/main/java/org/apache/sling/commons/threads/impl/ThreadPoolExecutorCleaningThreadLocals.java +++ b/src/main/java/org/apache/sling/commons/threads/impl/ThreadPoolExecutorCleaningThreadLocals.java @@ -54,6 +54,8 @@ public class ThreadPoolExecutorCleaningThreadLocals extends ThreadPoolExecutor { LOGGER.debug("Collecting changes to ThreadLocal for thread {} from now on...", t); try { ThreadLocalCleaner cleaner = new ThreadLocalCleaner(listener); + LOGGER.info("Before thread execution"); + cleaner.dump(LOGGER); local.set(cleaner); } catch (Throwable e) { LOGGER.warn("Could not set up thread local cleaner (most probably not a compliant JRE): {}", e, e); @@ -64,7 +66,11 @@ public class ThreadPoolExecutorCleaningThreadLocals extends ThreadPoolExecutor { LOGGER.debug("Cleaning up thread locals for thread {}...", Thread.currentThread()); ThreadLocalCleaner cleaner = local.get(); if (cleaner != null) { + LOGGER.info("After thread execution before cleanup"); + cleaner.dump(LOGGER); cleaner.cleanup(); + LOGGER.info("After thread execution after cleanup"); + cleaner.dump(LOGGER); } else { LOGGER.warn("Could not clean up thread locals in thread {} as the cleaner was not set up correctly", Thread.currentThread()); } diff --git a/src/test/java/org/apache/sling/commons/threads/impl/ThreadPoolExecutorCleaningThreadLocalsTest.java b/src/test/java/org/apache/sling/commons/threads/impl/ThreadPoolExecutorCleaningThreadLocalsTest.java index 0160e64..f56456e 100644 --- a/src/test/java/org/apache/sling/commons/threads/impl/ThreadPoolExecutorCleaningThreadLocalsTest.java +++ b/src/test/java/org/apache/sling/commons/threads/impl/ThreadPoolExecutorCleaningThreadLocalsTest.java @@ -24,13 +24,12 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.RejectedExecutionHandler; - import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import org.apache.sling.commons.threads.impl.ThreadLocalChangeListener.Mode; import org.junit.Assert; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentMatchers; @@ -55,7 +54,7 @@ public class ThreadPoolExecutorCleaningThreadLocalsTest { 1, 1, 100, TimeUnit.MILLISECONDS, queue, Executors.defaultThreadFactory(), rejectionHandler, listener); } - + @Test(timeout = 10000) public void threadLocalCleanupWorksWithResize() throws Exception { @@ -82,6 +81,8 @@ public class ThreadPoolExecutorCleaningThreadLocalsTest { private void assertTaskDoesNotSeeOldThreadLocals(String value) throws InterruptedException, ExecutionException { ThreadLocalTask task = new ThreadLocalTask(value); pool.submit(task).get(); + // when get returns the task may not have been cleaned up (i.e. afterExecute() was no necessarily executed), therefore wait a littlebit here + Thread.sleep(500); Assert.assertNull(task.getOldValue()); } -- To stop receiving notification emails like this one, please contact [email protected].
