shiro-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Les Hazlewood <...@hazlewood.com>
Subject Re: svn commit: r945310 - /incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java
Date Mon, 17 May 2010 19:17:48 GMT
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,  <kaosko@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;
> +            }
> +        }
> +    }
>  }
>
>
>
>

Mime
View raw message