Mailing-List: contact shiro-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: shiro-dev@incubator.apache.org
Received-SPF: neutral (nike.apache.org: local policy)
MIME-Version: 1.0
Sender: les.hazlewood@anjinllc.com
In-Reply-To: <20100517185939.C3DEE23889E5@eris.apache.org>
References: <20100517185939.C3DEE23889E5@eris.apache.org>
Date: Mon, 17 May 2010 12:17:48 -0700
Message-ID: <AANLkTinAyAa2z3xXYq8Gi85_O2nSMgLZjuQ6rqCK7CQH@mail.gmail.com>
Subject: Re: svn commit: r945310 -
 /incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/ThreadContext.java
From: Les Hazlewood <les@hazlewood.com>
To: shiro-dev@incubator.apache.org
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

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 sho=
rtly.

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=3D945310&view=3Drev
> Log:
> Incomplete - issue SHIRO-161: No SecurityManager accessible to the callin=
g code
> https://issues.apache.org/jira/browse/SHIRO-161
> The root cause of this issue was "resources =3D null;" in line 261 of rem=
ove() in r944585. The ThreadLocal attribute itself should *never* be nullif=
ied as that'll remove ThreadLocal variables for all threads. There's no nee=
d to create ThreadLocal lazily, so therefore there's no need for the create=
ThreadLocal() method either. Since the ThreadLocal is created at class load=
ing time, there's no need for "if (resources =3D=3D null)" checks either, s=
o 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 pro=
per way of accessing the threadlocal variable is always with ThreadLocal.ge=
t() so I changed all the occurrences to use that format. Finally, I don't s=
ee any benefit in doing clean() in remove() before get().remove() so I remo=
ved 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
> =C2=A0ater. Issue is fixed barring code review and possibly re-adding som=
e 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 te=
st for the case, so we need to rely on code review.
>
> Modified:
> =C2=A0 =C2=A0incubator/shiro/trunk/core/src/main/java/org/apache/shiro/ut=
il/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/jav=
a/org/apache/shiro/util/ThreadContext.java?rev=3D945310&r1=3D945309&r2=3D94=
5310&view=3Ddiff
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> --- incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/Thread=
Context.java (original)
> +++ incubator/shiro/trunk/core/src/main/java/org/apache/shiro/util/Thread=
Context.java Mon May 17 18:59:39 2010
> @@ -53,7 +53,7 @@ public abstract class ThreadContext {
> =C2=A0 =C2=A0 public static final String SECURITY_MANAGER_KEY =3D ThreadC=
ontext.class.getName() + "_SECURITY_MANAGER_KEY";
> =C2=A0 =C2=A0 public static final String SUBJECT_KEY =3D ThreadContext.cl=
ass.getName() + "_SUBJECT_KEY";
>
> - =C2=A0 =C2=A0protected static ThreadLocal<Map<Object, Object>> resource=
s;
> + =C2=A0 =C2=A0private static final ThreadLocal<Map<Object, Object>> reso=
urces =3D new InheritableThreadLocalMap<Map<Object, Object>>();
>
> =C2=A0 =C2=A0 /**
> =C2=A0 =C2=A0 =C2=A0* Default no-argument constructor.
> @@ -62,50 +62,6 @@ public abstract class ThreadContext {
> =C2=A0 =C2=A0 }
>
> =C2=A0 =C2=A0 /**
> - =C2=A0 =C2=A0 * Returns the {@link ThreadLocal} resource {@code Map}. =
=C2=A0If it does not yet exist, one is created,
> - =C2=A0 =C2=A0 * bound to the thread, and then returned.
> - =C2=A0 =C2=A0 *
> - =C2=A0 =C2=A0 * @return the ThreadLocal resource {@code Map}, possibly =
lazily-created.
> - =C2=A0 =C2=A0 * @since 1.0
> - =C2=A0 =C2=A0 */
> - =C2=A0 =C2=A0protected static Map<Object, Object> getResourcesLazy() {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if (resources =3D=3D null) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0resources =3D createThreadLoca=
l();
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0return resources.get();
> - =C2=A0 =C2=A0}
> -
> - =C2=A0 =C2=A0/**
> - =C2=A0 =C2=A0 * Creates a new {@link ThreadLocal} instance containing a=
 {@link Map} to hold arbitrary key-value pairs.
> - =C2=A0 =C2=A0 *
> - =C2=A0 =C2=A0 * @return a new {@link ThreadLocal} instance containing a=
 {@link Map} to hold arbitrary key-value pairs.
> - =C2=A0 =C2=A0 * @since 1.0
> - =C2=A0 =C2=A0 */
> - =C2=A0 =C2=A0private static ThreadLocal<Map<Object, Object>> createThre=
adLocal() {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0return new InheritableThreadLocal<Map<Object=
, Object>>() {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0protected Map<Object, Object> =
initialValue() {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return new HashM=
ap<Object, Object>();
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> -
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/**
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * This implementation was add=
ed to address a
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * <a href=3D"http://jsecurity=
.markmail.org/search/?q=3D#query:+page:1+mid:xqi2yxurwmrpqrvj+state:results=
">
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * user-reported issue</a>.
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * @param parentValue the pare=
nt value, a HashMap as defined in the {@link #initialValue()} method.
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * @return the HashMap to be u=
sed by any parent-spawned child threads (a clone of the parent HashMap).
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0@SuppressWarnings({"unchecked"=
})
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0protected Map<Object, Object> =
childValue(Map<Object, Object> parentValue) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (parentValue =
!=3D null) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0re=
turn (Map<Object, Object>) ((HashMap<Object, Object>) parentValue).clone();
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0re=
turn null;
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0};
> - =C2=A0 =C2=A0}
> -
> - =C2=A0 =C2=A0/**
> =C2=A0 =C2=A0 =C2=A0* Returns the ThreadLocal Map. This Map is used inter=
nally to bind objects
> =C2=A0 =C2=A0 =C2=A0* to the current thread by storing each object under =
a unique key.
> =C2=A0 =C2=A0 =C2=A0*
> @@ -123,13 +79,12 @@ public abstract class ThreadContext {
> =C2=A0 =C2=A0 =C2=A0* @param resources the resources to replace the exist=
ing {@link #getResources() resources}.
> =C2=A0 =C2=A0 =C2=A0* @since 1.0
> =C2=A0 =C2=A0 =C2=A0*/
> - =C2=A0 =C2=A0public static void setResources(Map<Object, Object> resour=
ces) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if (CollectionUtils.isEmpty(resources)) {
> + =C2=A0 =C2=A0public static void setResources(Map<Object, Object> newRes=
ources) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (CollectionUtils.isEmpty(newResources)) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0Map<Object, Object> existing =3D getResource=
sLazy();
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0existing.clear();
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0existing.putAll(resources);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0resources.get().clear();
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0resources.get().putAll(newResources);
> =C2=A0 =C2=A0 }
>
> =C2=A0 =C2=A0 /**
> @@ -142,9 +97,6 @@ public abstract class ThreadContext {
> =C2=A0 =C2=A0 =C2=A0* @since 1.0
> =C2=A0 =C2=A0 =C2=A0*/
> =C2=A0 =C2=A0 private static Object getValue(Object key) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if (resources =3D=3D null) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return null;
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return resources.get().get(key);
> =C2=A0 =C2=A0 }
>
> @@ -196,7 +148,7 @@ public abstract class ThreadContext {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0getResourcesLazy().put(key, value);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0resources.get().put(key, value);
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (log.isTraceEnabled()) {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 String msg =3D "Bound value of =
type [" + value.getClass().getName() + "] for key [" +
> @@ -214,9 +166,6 @@ public abstract class ThreadContext {
> =C2=A0 =C2=A0 =C2=A0* =C2=A0 =C2=A0 =C2=A0 =C2=A0 under the specified <tt=
>key</tt> name.
> =C2=A0 =C2=A0 =C2=A0*/
> =C2=A0 =C2=A0 public static Object remove(Object key) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if (resources =3D=3D null) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return null;
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 Object value =3D resources.get().remove(key);
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((value !=3D null) && log.isTraceEnabled()=
) {
> @@ -229,23 +178,6 @@ public abstract class ThreadContext {
> =C2=A0 =C2=A0 }
>
> =C2=A0 =C2=A0 /**
> - =C2=A0 =C2=A0 * Clears <em>all</em> values bound to this ThreadContext,=
 which includes any Subject, Session, or InetAddress
> - =C2=A0 =C2=A0 * that may be bound by these respective objects' convenie=
nce methods, as well as all values bound by your
> - =C2=A0 =C2=A0 * application code.
> - =C2=A0 =C2=A0 * <p/>
> - =C2=A0 =C2=A0 * <p>This operation is meant as a clean-up operation that=
 may be called at the end of
> - =C2=A0 =C2=A0 * thread execution to prevent data corruption in a pooled=
 thread environment.
> - =C2=A0 =C2=A0 */
> - =C2=A0 =C2=A0public static void clear() {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if (resources !=3D null) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0resources.get().clear();
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if (log.isTraceEnabled()) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0log.trace("Removed all ThreadC=
ontext values from thread [" + Thread.currentThread().getName() + "]");
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> - =C2=A0 =C2=A0}
> -
> - =C2=A0 =C2=A0/**
> =C2=A0 =C2=A0 =C2=A0* First {@link #clear clears} the {@code ThreadContex=
t} values and then
> =C2=A0 =C2=A0 =C2=A0* {@link ThreadLocal#remove removes} the underlying {=
@link ThreadLocal ThreadLocal} from the thread.
> =C2=A0 =C2=A0 =C2=A0* <p/>
> @@ -255,11 +187,7 @@ public abstract class ThreadContext {
> =C2=A0 =C2=A0 =C2=A0* @since 1.0
> =C2=A0 =C2=A0 =C2=A0*/
> =C2=A0 =C2=A0 public static void remove() {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if (resources !=3D null) {
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0clear();
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0resources.remove();
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0resources =3D null;
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0resources.remove();
> =C2=A0 =C2=A0 }
>
> =C2=A0 =C2=A0 /**
> @@ -379,5 +307,27 @@ public abstract class ThreadContext {
> =C2=A0 =C2=A0 public static Subject unbindSubject() {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (Subject) remove(SUBJECT_KEY);
> =C2=A0 =C2=A0 }
> +
> + =C2=A0 =C2=A0private static final class InheritableThreadLocalMap<T ext=
ends Map<Object, Object>> extends InheritableThreadLocal<Map<Object, Object=
>> {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0protected Map<Object, Object> initialValue()=
 {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return new HashMap<Object, Obj=
ect>();
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0/**
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 * This implementation was added to address =
a
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 * <a href=3D"http://jsecurity.markmail.org/=
search/?q=3D#query:+page:1+mid:xqi2yxurwmrpqrvj+state:results">
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 * user-reported issue</a>.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 * @param parentValue the parent value, a Ha=
shMap as defined in the {@link #initialValue()} method.
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 * @return the HashMap to be used by any par=
ent-spawned child threads (a clone of the parent HashMap).
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0@SuppressWarnings({"unchecked"})
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0protected Map<Object, Object> childValue(Map=
<Object, Object> parentValue) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (parentValue !=3D null) {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return (Map<Obje=
ct, Object>) ((HashMap<Object, Object>) parentValue).clone();
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return null;
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> + =C2=A0 =C2=A0}
> =C2=A0}
>
>
>
>