From f7e295fbff9bf8df34fc78b07623bbd16b2b718d Mon Sep 17 00:00:00 2001 From: markt Date: Fri, 18 Dec 2009 18:42:09 +0000 Subject: [PATCH] Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=47930 Make swapIn thread safe so parallel requests for the same session don't result in multiple session objects for one sesison. git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@892341 13f79535-47bb-0310-9956-ffa450edef68 --- .../catalina/session/PersistentManagerBase.java | 132 ++++++++++++++------- 1 file changed, 89 insertions(+), 43 deletions(-) diff --git a/java/org/apache/catalina/session/PersistentManagerBase.java b/java/org/apache/catalina/session/PersistentManagerBase.java index 93a68f7b0..072fca725 100644 --- a/java/org/apache/catalina/session/PersistentManagerBase.java +++ b/java/org/apache/catalina/session/PersistentManagerBase.java @@ -24,6 +24,9 @@ import java.io.IOException; import java.security.AccessController; import java.security.PrivilegedActionException; import java.security.PrivilegedExceptionAction; +import java.util.HashMap; +import java.util.Map; + import org.apache.catalina.Container; import org.apache.catalina.Context; import org.apache.catalina.Lifecycle; @@ -207,6 +210,13 @@ public abstract class PersistentManagerBase protected long processingTime = 0; + /** + * Sessions currently being swapped in and the associated locks + */ + private Map sessionSwapInLocks = + new HashMap(); + + // ------------------------------------------------------------- Properties @@ -779,53 +789,89 @@ public abstract class PersistentManagerBase if (store == null) return null; - Session session = null; - try { - if (SecurityUtil.isPackageProtectionEnabled()){ - try{ - session = AccessController.doPrivileged( - new PrivilegedStoreLoad(id)); - }catch(PrivilegedActionException ex){ - Exception exception = ex.getException(); - log.error("Exception in the Store during swapIn: " - + exception); - if (exception instanceof IOException){ - throw (IOException)exception; - } else if (exception instanceof ClassNotFoundException) { - throw (ClassNotFoundException)exception; - } - } - } else { - session = store.load(id); - } - } catch (ClassNotFoundException e) { - log.error(sm.getString("persistentManager.deserializeError", id, e)); - throw new IllegalStateException - (sm.getString("persistentManager.deserializeError", id, e)); - } + Object swapInLock = null; + + /* + * The purpose of this sync and these locks is to make sure that a + * session is only loaded once. It doesn't matter if the lock is removed + * and then another thread enters this method and trues to load the same + * session. That thread will re-creates a swapIn lock for that session, + * quickly find that the session is already in sessions, use it and + * carry on. + */ + synchronized (this) { + if (sessionSwapInLocks.containsKey(id)) { + swapInLock = sessionSwapInLocks.get(id); + } else { + swapInLock = new Object(); + sessionSwapInLocks.put(id, swapInLock); + } + } - if (session == null) - return (null); + Session session = null; - if (!session.isValid()) { - log.error("session swapped in is invalid or expired"); - session.expire(); - removeSession(id); - return (null); + synchronized (swapInLock) { + // First check to see if another thread has loaded the session into + // the manager + session = sessions.get(id); + + if (session == null) { + try { + if (SecurityUtil.isPackageProtectionEnabled()){ + try { + session = AccessController.doPrivileged( + new PrivilegedStoreLoad(id)); + } catch (PrivilegedActionException ex) { + Exception e = ex.getException(); + log.error(sm.getString( + "persistentManager.swapInException", id), + e); + if (e instanceof IOException){ + throw (IOException)e; + } else if (e instanceof ClassNotFoundException) { + throw (ClassNotFoundException)e; + } + } + } else { + session = store.load(id); + } + } catch (ClassNotFoundException e) { + String msg = sm.getString( + "persistentManager.deserializeError", id); + log.error(msg, e); + throw new IllegalStateException(msg, e); + } + + if (session != null && !session.isValid()) { + log.error(sm.getString( + "persistentManager.swapInInvalid", id)); + session.expire(); + removeSession(id); + session = null; + } + + if (session != null) { + if(log.isDebugEnabled()) + log.debug(sm.getString("persistentManager.swapIn", id)); + + session.setManager(this); + // make sure the listeners know about it. + ((StandardSession)session).tellNew(); + add(session); + ((StandardSession)session).activate(); + // endAccess() to ensure timeouts happen correctly. + // access() to keep access count correct or it will end up + // negative + session.access(); + session.endAccess(); + } + } } - if(log.isDebugEnabled()) - log.debug(sm.getString("persistentManager.swapIn", id)); - - session.setManager(this); - // make sure the listeners know about it. - ((StandardSession)session).tellNew(); - add(session); - ((StandardSession)session).activate(); - // endAccess() to ensure timeouts happen correctly. - // access() to keep access count correct or it will end up negative - session.access(); - session.endAccess(); + // Make sure the lock is removed + synchronized (this) { + sessionSwapInLocks.remove(id); + } return (session); -- 2.11.0