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, <[email protected]> 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; > + } > + } > + } > } > > > >
