You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by kw...@apache.org on 2018/01/24 16:02:11 UTC
[sling-org-apache-sling-commons-threads] branch bugfix/SLING-7432
updated: SLING-7432 no longer rely on thread locals themselves to store old
thread local state
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
The following commit(s) were added to refs/heads/bugfix/SLING-7432 by this push:
new 6020992 SLING-7432 no longer rely on thread locals themselves to store old thread local state
6020992 is described below
commit 60209922e77383f71318d5249fba32bd6b21fd3a
Author: Konrad Windszus <kw...@apache.org>
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 | 76 +++++++++++++++-------
.../ThreadPoolExecutorCleaningThreadLocals.java | 6 ++
...ThreadPoolExecutorCleaningThreadLocalsTest.java | 7 +-
3 files changed, 61 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..b3ecb9e 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,23 @@ 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);
+
+ System.out.println(
+ "saveOldThreadLocals: Found " + copyOfThreadLocalsSize + " thread locals bound to thread " + Thread.currentThread());
}
private static Reference<?>[] copy(Field field) {
@@ -216,15 +243,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
kwin@apache.org.