Nathaniel Alfred wrote:
One must synchonize the put and get operation on the map itself
in order to protect its internal consistency.
Well, yeah. A synchronized block that synchronizes on the map
accomplishes that.
Map map = Collections.synchronizedMap(new HashMap());
...
map.put(key, value);
...
value = map.get(key);
is just easier to read than
Map map = new HashMap();
...
synchronized(map) { map.put(key, value); }
...
synchronized(map) { value = map.get(key); }
Except that in this case it is:
synchronized (sessions) {
// retrieve existing wrapper
session =
(HttpSession)((WeakReference)sessions.get(serverSession)).get();
if (session == null) {
// create new wrapper
session = new HttpSession(serverSession);
sessions.put(serverSession, new WeakReference(session));
}
}
although with just one get and one put the significance may be argued.
You don't want to replace the outer synchronized(serverSession) by
synchronized(sessions) because that blocks all threads without being
necessary (although the effect will be immeasurable).
Yes, I do. The alternative is to synchronize on the servlet container's
session object, which could have far more horrifying results. Do you
have any idea how that will impact the container? I don't because I
cannot know. For all I know it is conceivable that doing that could
cause a deadlock in some wierd scenario. I also fail to see how locking
the map causes any more of a performance hit than locking the session.
Since the map is only used by this one method its effect should be far
less of an impact that locking the session. Besides, you ARE blocking
all threads on the get and put anyway, since it has been declared a
synchronized hash map. In fact, it is being done twice inside of a
synchronized block.
You must have synchronized(serverSession) (or block all threads) to
avoid calling sessions.put twice for the same serverSession.
I don't see two calls to put in the code above....
Cheers, Alfred.
Ralph
