Yes, there was considerable discussion about this.
Yes, the version in svn is wrong. I was hoping it would get fixed after our discussion, but that hasn't happened, so I'll try to commit something tonight (it is still Sunday night here in California).
Ralph
Jochen Kuhnle wrote:
Am I not getting it, or is the implementation broken (see below)?
/** The map to assure 1:1-mapping of server sessions and Cocoon session wrappers */
private static final Map sessions = Collections.synchronizedMap(new WeakHashMap());
public Session getSession(boolean create) {
javax.servlet.http.HttpSession serverSession = this.req.getSession(create);
HttpSession session;
if (serverSession != null) {
// no need to lock the map - it is synchronized
// synch on server session assures only one wrapper per session synchronized (serverSession) {
// retrieve existing wrapper
session = (HttpSession)((WeakReference)sessions.get(serverSession)).get();
//----> What if the map does not contain the session? sessions.get(serverSessions) returns null, resulting in a NullPointerException in the second get().
//----> Why not synchronize on the map, and not the serverSession. Right now, we run through 3 synchronize blocks: synchronized(serverSession), session.get(...) and sessions.put(...). Wouldn't replacing synchronized(serverSession) with synchronized(sessions) and using an unsynchronized map suffice? Resulting in only one synchronized block?
if (session == null) {
// create new wrapper
session = new HttpSession(serverSession);
sessions.put(serverSession, new WeakReference(session));
}
}
} else {
// invalidate
session = null;
}
return session; }
Regards,
Jochen
