You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@shiro.apache.org by ka...@apache.org on 2010/05/17 20:59:39 UTC
svn commit: r945310 -
/incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java
Author: kaosko
Date: Mon May 17 18:59:39 2010
New Revision: 945310
URL: http://svn.apache.org/viewvc?rev=945310&view=rev
Log:
Incomplete - issue SHIRO-161: No SecurityManager accessible to the calling code
https://issues.apache.org/jira/browse/SHIRO-161
The root cause of this issue was "resources = null;" in line 261 of remove() in r944585. The ThreadLocal attribute itself should *never* be nullified as that'll remove ThreadLocal variables for all threads. There's no need to create ThreadLocal lazily, so therefore there's no need for the createThreadLocal() method either. Since the ThreadLocal is created at class loading time, there's no need for "if (resources == null)" checks either, so I've removed them in order to simplify the code. The usage of ThreadLocal was somewhat odd with the cast to Map; while it technically works, the proper way of accessing the threadlocal variable is always with ThreadLocal.get() so I changed all the occurrences to use that format. Finally, I don't see any benefit in doing clean() in remove() before get().remove() so I removed the clean() call and I also removed the whole operation since it wasn't being used anymore and I don't see any use case where it could be used. We can always add it in l
ater. Issue is fixed barring code review and possibly re-adding some of the removed code if there's a validated need for it. There are no new test cases added because it'd be difficult to write a comprehensive unit test for the case, so we need to rely on code review.
Modified:
incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java
Modified: incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java
URL: http://svn.apache.org/viewvc/incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java?rev=945310&r1=945309&r2=945310&view=diff
==============================================================================
--- incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java (original)
+++ incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java Mon May 17 18:59:39 2010
@@ -53,7 +53,7 @@ public abstract class ThreadContext {
public static final String SECURITY_MANAGER_KEY = ThreadContext.class.getName() + "_SECURITY_MANAGER_KEY";
public static final String SUBJECT_KEY = ThreadContext.class.getName() + "_SUBJECT_KEY";
- protected static ThreadLocal<Map<Object, Object>> resources;
+ private static final ThreadLocal<Map<Object, Object>> resources = new InheritableThreadLocalMap<Map<Object, Object>>();
/**
* Default no-argument constructor.
@@ -62,50 +62,6 @@ public abstract class ThreadContext {
}
/**
- * Returns the {@link ThreadLocal} resource {@code Map}. If it does not yet exist, one is created,
- * bound to the thread, and then returned.
- *
- * @return the ThreadLocal resource {@code Map}, possibly lazily-created.
- * @since 1.0
- */
- protected static Map<Object, Object> getResourcesLazy() {
- if (resources == null) {
- resources = createThreadLocal();
- }
- return resources.get();
- }
-
- /**
- * Creates a new {@link ThreadLocal} instance containing a {@link Map} to hold arbitrary key-value pairs.
- *
- * @return a new {@link ThreadLocal} instance containing a {@link Map} to hold arbitrary key-value pairs.
- * @since 1.0
- */
- private static ThreadLocal<Map<Object, Object>> createThreadLocal() {
- return new InheritableThreadLocal<Map<Object, Object>>() {
- protected Map<Object, Object> initialValue() {
- return new HashMap<Object, Object>();
- }
-
- /**
- * This implementation was added to address a
- * <a href="http://jsecurity.markmail.org/search/?q=#query:+page:1+mid:xqi2yxurwmrpqrvj+state:results">
- * user-reported issue</a>.
- * @param parentValue the parent value, a HashMap as defined in the {@link #initialValue()} method.
- * @return the HashMap to be used by any parent-spawned child threads (a clone of the parent HashMap).
- */
- @SuppressWarnings({"unchecked"})
- protected Map<Object, Object> childValue(Map<Object, Object> parentValue) {
- if (parentValue != null) {
- return (Map<Object, Object>) ((HashMap<Object, Object>) parentValue).clone();
- } else {
- return null;
- }
- }
- };
- }
-
- /**
* Returns the ThreadLocal Map. This Map is used internally to bind objects
* to the current thread by storing each object under a unique key.
*
@@ -123,13 +79,12 @@ public abstract class ThreadContext {
* @param resources the resources to replace the existing {@link #getResources() resources}.
* @since 1.0
*/
- public static void setResources(Map<Object, Object> resources) {
- if (CollectionUtils.isEmpty(resources)) {
+ public static void setResources(Map<Object, Object> newResources) {
+ if (CollectionUtils.isEmpty(newResources)) {
return;
}
- Map<Object, Object> existing = getResourcesLazy();
- existing.clear();
- existing.putAll(resources);
+ resources.get().clear();
+ resources.get().putAll(newResources);
}
/**
@@ -142,9 +97,6 @@ public abstract class ThreadContext {
* @since 1.0
*/
private static Object getValue(Object key) {
- if (resources == null) {
- return null;
- }
return resources.get().get(key);
}
@@ -196,7 +148,7 @@ public abstract class ThreadContext {
return;
}
- getResourcesLazy().put(key, value);
+ resources.get().put(key, value);
if (log.isTraceEnabled()) {
String msg = "Bound value of type [" + value.getClass().getName() + "] for key [" +
@@ -214,9 +166,6 @@ public abstract class ThreadContext {
* under the specified <tt>key</tt> name.
*/
public static Object remove(Object key) {
- if (resources == null) {
- return null;
- }
Object value = resources.get().remove(key);
if ((value != null) && log.isTraceEnabled()) {
@@ -229,23 +178,6 @@ public abstract class ThreadContext {
}
/**
- * Clears <em>all</em> values bound to this ThreadContext, which includes any Subject, Session, or InetAddress
- * that may be bound by these respective objects' convenience methods, as well as all values bound by your
- * application code.
- * <p/>
- * <p>This operation is meant as a clean-up operation that may be called at the end of
- * thread execution to prevent data corruption in a pooled thread environment.
- */
- public static void clear() {
- if (resources != null) {
- resources.get().clear();
- }
- if (log.isTraceEnabled()) {
- log.trace("Removed all ThreadContext values from thread [" + Thread.currentThread().getName() + "]");
- }
- }
-
- /**
* First {@link #clear clears} the {@code ThreadContext} values and then
* {@link ThreadLocal#remove removes} the underlying {@link ThreadLocal ThreadLocal} from the thread.
* <p/>
@@ -255,11 +187,7 @@ public abstract class ThreadContext {
* @since 1.0
*/
public static void remove() {
- if (resources != null) {
- clear();
- resources.remove();
- resources = null;
- }
+ resources.remove();
}
/**
@@ -379,5 +307,27 @@ public abstract class ThreadContext {
public static Subject unbindSubject() {
return (Subject) remove(SUBJECT_KEY);
}
+
+ private static final class InheritableThreadLocalMap<T extends Map<Object, Object>> extends InheritableThreadLocal<Map<Object, Object>> {
+ protected Map<Object, Object> initialValue() {
+ return new HashMap<Object, Object>();
+ }
+
+ /**
+ * This implementation was added to address a
+ * <a href="http://jsecurity.markmail.org/search/?q=#query:+page:1+mid:xqi2yxurwmrpqrvj+state:results">
+ * user-reported issue</a>.
+ * @param parentValue the parent value, a HashMap as defined in the {@link #initialValue()} method.
+ * @return the HashMap to be used by any parent-spawned child threads (a clone of the parent HashMap).
+ */
+ @SuppressWarnings({"unchecked"})
+ protected Map<Object, Object> childValue(Map<Object, Object> parentValue) {
+ if (parentValue != null) {
+ return (Map<Object, Object>) ((HashMap<Object, Object>) parentValue).clone();
+ } else {
+ return null;
+ }
+ }
+ }
}
Re: svn commit: r945310 - /incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java
Posted by Les Hazlewood <le...@hazlewood.com>.
Looks good to me! (good catch on the null value!)
There are a few very minor JavaDoc issues that I can fix - I'll do that shortly.
Thanks!
Les
On Mon, May 17, 2010 at 11:59 AM, <ka...@apache.org> wrote:
> Author: kaosko
> Date: Mon May 17 18:59:39 2010
> New Revision: 945310
>
> URL: http://svn.apache.org/viewvc?rev=945310&view=rev
> Log:
> Incomplete - issue SHIRO-161: No SecurityManager accessible to the calling code
> https://issues.apache.org/jira/browse/SHIRO-161
> The root cause of this issue was "resources = null;" in line 261 of remove() in r944585. The ThreadLocal attribute itself should *never* be nullified as that'll remove ThreadLocal variables for all threads. There's no need to create ThreadLocal lazily, so therefore there's no need for the createThreadLocal() method either. Since the ThreadLocal is created at class loading time, there's no need for "if (resources == null)" checks either, so I've removed them in order to simplify the code. The usage of ThreadLocal was somewhat odd with the cast to Map; while it technically works, the proper way of accessing the threadlocal variable is always with ThreadLocal.get() so I changed all the occurrences to use that format. Finally, I don't see any benefit in doing clean() in remove() before get().remove() so I removed the clean() call and I also removed the whole operation since it wasn't being used anymore and I don't see any use case where it could be used. We can always add it in l
> ater. Issue is fixed barring code review and possibly re-adding some of the removed code if there's a validated need for it. There are no new test cases added because it'd be difficult to write a comprehensive unit test for the case, so we need to rely on code review.
>
> Modified:
> incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java
>
> Modified: incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java
> URL: http://svn.apache.org/viewvc/incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java?rev=945310&r1=945309&r2=945310&view=diff
> ==============================================================================
> --- incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java (original)
> +++ incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java Mon May 17 18:59:39 2010
> @@ -53,7 +53,7 @@ public abstract class ThreadContext {
> public static final String SECURITY_MANAGER_KEY = ThreadContext.class.getName() + "_SECURITY_MANAGER_KEY";
> public static final String SUBJECT_KEY = ThreadContext.class.getName() + "_SUBJECT_KEY";
>
> - protected static ThreadLocal<Map<Object, Object>> resources;
> + private static final ThreadLocal<Map<Object, Object>> resources = new InheritableThreadLocalMap<Map<Object, Object>>();
>
> /**
> * Default no-argument constructor.
> @@ -62,50 +62,6 @@ public abstract class ThreadContext {
> }
>
> /**
> - * Returns the {@link ThreadLocal} resource {@code Map}. If it does not yet exist, one is created,
> - * bound to the thread, and then returned.
> - *
> - * @return the ThreadLocal resource {@code Map}, possibly lazily-created.
> - * @since 1.0
> - */
> - protected static Map<Object, Object> getResourcesLazy() {
> - if (resources == null) {
> - resources = createThreadLocal();
> - }
> - return resources.get();
> - }
> -
> - /**
> - * Creates a new {@link ThreadLocal} instance containing a {@link Map} to hold arbitrary key-value pairs.
> - *
> - * @return a new {@link ThreadLocal} instance containing a {@link Map} to hold arbitrary key-value pairs.
> - * @since 1.0
> - */
> - private static ThreadLocal<Map<Object, Object>> createThreadLocal() {
> - return new InheritableThreadLocal<Map<Object, Object>>() {
> - protected Map<Object, Object> initialValue() {
> - return new HashMap<Object, Object>();
> - }
> -
> - /**
> - * This implementation was added to address a
> - * <a href="http://jsecurity.markmail.org/search/?q=#query:+page:1+mid:xqi2yxurwmrpqrvj+state:results">
> - * user-reported issue</a>.
> - * @param parentValue the parent value, a HashMap as defined in the {@link #initialValue()} method.
> - * @return the HashMap to be used by any parent-spawned child threads (a clone of the parent HashMap).
> - */
> - @SuppressWarnings({"unchecked"})
> - protected Map<Object, Object> childValue(Map<Object, Object> parentValue) {
> - if (parentValue != null) {
> - return (Map<Object, Object>) ((HashMap<Object, Object>) parentValue).clone();
> - } else {
> - return null;
> - }
> - }
> - };
> - }
> -
> - /**
> * Returns the ThreadLocal Map. This Map is used internally to bind objects
> * to the current thread by storing each object under a unique key.
> *
> @@ -123,13 +79,12 @@ public abstract class ThreadContext {
> * @param resources the resources to replace the existing {@link #getResources() resources}.
> * @since 1.0
> */
> - public static void setResources(Map<Object, Object> resources) {
> - if (CollectionUtils.isEmpty(resources)) {
> + public static void setResources(Map<Object, Object> newResources) {
> + if (CollectionUtils.isEmpty(newResources)) {
> return;
> }
> - Map<Object, Object> existing = getResourcesLazy();
> - existing.clear();
> - existing.putAll(resources);
> + resources.get().clear();
> + resources.get().putAll(newResources);
> }
>
> /**
> @@ -142,9 +97,6 @@ public abstract class ThreadContext {
> * @since 1.0
> */
> private static Object getValue(Object key) {
> - if (resources == null) {
> - return null;
> - }
> return resources.get().get(key);
> }
>
> @@ -196,7 +148,7 @@ public abstract class ThreadContext {
> return;
> }
>
> - getResourcesLazy().put(key, value);
> + resources.get().put(key, value);
>
> if (log.isTraceEnabled()) {
> String msg = "Bound value of type [" + value.getClass().getName() + "] for key [" +
> @@ -214,9 +166,6 @@ public abstract class ThreadContext {
> * under the specified <tt>key</tt> name.
> */
> public static Object remove(Object key) {
> - if (resources == null) {
> - return null;
> - }
> Object value = resources.get().remove(key);
>
> if ((value != null) && log.isTraceEnabled()) {
> @@ -229,23 +178,6 @@ public abstract class ThreadContext {
> }
>
> /**
> - * Clears <em>all</em> values bound to this ThreadContext, which includes any Subject, Session, or InetAddress
> - * that may be bound by these respective objects' convenience methods, as well as all values bound by your
> - * application code.
> - * <p/>
> - * <p>This operation is meant as a clean-up operation that may be called at the end of
> - * thread execution to prevent data corruption in a pooled thread environment.
> - */
> - public static void clear() {
> - if (resources != null) {
> - resources.get().clear();
> - }
> - if (log.isTraceEnabled()) {
> - log.trace("Removed all ThreadContext values from thread [" + Thread.currentThread().getName() + "]");
> - }
> - }
> -
> - /**
> * First {@link #clear clears} the {@code ThreadContext} values and then
> * {@link ThreadLocal#remove removes} the underlying {@link ThreadLocal ThreadLocal} from the thread.
> * <p/>
> @@ -255,11 +187,7 @@ public abstract class ThreadContext {
> * @since 1.0
> */
> public static void remove() {
> - if (resources != null) {
> - clear();
> - resources.remove();
> - resources = null;
> - }
> + resources.remove();
> }
>
> /**
> @@ -379,5 +307,27 @@ public abstract class ThreadContext {
> public static Subject unbindSubject() {
> return (Subject) remove(SUBJECT_KEY);
> }
> +
> + private static final class InheritableThreadLocalMap<T extends Map<Object, Object>> extends InheritableThreadLocal<Map<Object, Object>> {
> + protected Map<Object, Object> initialValue() {
> + return new HashMap<Object, Object>();
> + }
> +
> + /**
> + * This implementation was added to address a
> + * <a href="http://jsecurity.markmail.org/search/?q=#query:+page:1+mid:xqi2yxurwmrpqrvj+state:results">
> + * user-reported issue</a>.
> + * @param parentValue the parent value, a HashMap as defined in the {@link #initialValue()} method.
> + * @return the HashMap to be used by any parent-spawned child threads (a clone of the parent HashMap).
> + */
> + @SuppressWarnings({"unchecked"})
> + protected Map<Object, Object> childValue(Map<Object, Object> parentValue) {
> + if (parentValue != null) {
> + return (Map<Object, Object>) ((HashMap<Object, Object>) parentValue).clone();
> + } else {
> + return null;
> + }
> + }
> + }
> }
>
>
>
>