DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT <http://issues.apache.org/bugzilla/show_bug.cgi?id=43343>. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=43343 Summary: Loss of data and concurrency issue with Catalina session persistent storage Product: Tomcat 6 Version: 6.0.9 Platform: Other OS/Version: other Status: NEW Severity: major Priority: P2 Component: Catalina AssignedTo: [EMAIL PROTECTED] ReportedBy: [EMAIL PROTECTED] A user was asking questions on the tomcat users mailing list about TC and how it handled concurrency in sessions and session passivation/object caching of sessions when they are inactive etc. I thought, surely this is using locks etc, so had responded as such. Then I got curious. I started looking in the code and found that indeed TC has a concurrency issue when the session is to pushed to disk if it is inactive or the max number of live sessions is too high. The problem is a user could have a session getting ready to become inactive. The server is beginning to write the session out to disk. The user then comes in on a request and gets the current session. They set an attribute. The web application and the user think all is OK. But in reality the data they just put into the session will be lost. The server just dumped their session to disk containing the old data as they updated the copy. So, the next time they come in the session will be pulled from disk with the old values and anything they put into it will have been lost. Depending on the application this could be very bad. To see the issue go to the file: java/org/apache/catalina/session/PersistentManagerBase.java methods: swapIn swapOut writeSession findSession (other related) then the different stores load and save methods. There is nothing keeping this from happening. If a lock is on a per session level this should keep it snappy for all other requests etc. Either the session could be used as the lock or an object instance variable on the session instance. I noticed there is a comment in the source code for PersistentManagerBase.processMaxIdleSwaps which reads: // Swap out all sessions idle longer than maxIdleSwap // FIXME: What's preventing us from mangling a session during // a request? So, apparently someone thought of this. Nothing is keeping it from mangling a session currently. What might be a fix is a session is given an instance variable which can be used for a synchronization lock at the session level. In PersistentManagerBase.findSession, this lock would be used before the session can be returned. After the session map is accessed it should then return null if the session had been passivating before it was asked to be found. The session would then be loaded from storage again before it was ever given back to be accessed had it been passivating. It would also be used in PersistentManagerBase.* which call swapOut. The code would look like this (might explain it better): StandardSession: /** Used for locking the session during persistence operations. **/ Integer persistence_locker = new Integer(0); PersistentManagerBase: /** * Return the active Session, associated with this Manager, with the * specified session id (if any); otherwise return <code>null</code>. * This method checks the persistence store if persistence is enabled, * otherwise just uses the functionality from ManagerBase. * * @param id The session id for the session to be returned * * @exception IllegalStateException if a new session cannot be * instantiated for any reason * @exception IOException if an input/output error occurs while * processing this request */ public Session findSession(String id) throws IOException { Session session = super.findSession(id); //OK, at this point, we're not sure if another thread is trying to //remove the session or not so the only way around this is to lock //it (or attempt to) and then try to get it by this session id again. //If the other code ran swapOut, then we should get a null back during //this run, and if not, then by doing this we lock it out and then can //access the session safely and will call access on it to update the //access time and hopefully keep the processes from running swapOut //so, we're adding two accesses to the hashmap instead of one for //each request. But, we know we get the session or null back after //the other process has had a chance to remove it or not. if(session!=null{ synchronized(session.persistence_locker){ session = super.findSession(session.getIdInternal()); if(session!=null){ //we need to do this here //to keep any external calling code from messing up the //concurrency. session.access(); } } } if (session != null) return (session); // See if the Session is in the Store session = swapIn(id); return (session); } /** * Swap idle sessions out to Store if they are idle too long. */ protected void processMaxIdleSwaps() { if (!isStarted() || maxIdleSwap < 0) return; Session sessions[] = findSessions(); long timeNow = System.currentTimeMillis(); // Swap out all sessions idle longer than maxIdleSwap // FIXME: What's preventing us from mangling a session during // a request? if (maxIdleSwap >= 0) { for (int i = 0; i < sessions.length; i++) { StandardSession session = (StandardSession) sessions[i]; synchronized(session.persistence_locker){ if (!session.isValid()) continue; int timeIdle = // Truncate, do not round up (int) ((timeNow - session.getLastAccessedTime()) / 1000L); if (timeIdle > maxIdleSwap && timeIdle > minIdleSwap) { if (log.isDebugEnabled()) log.debug(sm.getString ("persistentManager.swapMaxIdle", session.getIdInternal(), new Integer(timeIdle))); try { swapOut(session); } catch (IOException e) { ; // This is logged in writeSession() } } } } } } /** * Swap idle sessions out to Store if too many are active */ protected void processMaxActiveSwaps() { if (!isStarted() || getMaxActiveSessions() < 0) return; Session sessions[] = findSessions(); // FIXME: Smarter algorithm (LRU) if (getMaxActiveSessions() >= sessions.length) return; if(log.isDebugEnabled()) log.debug(sm.getString ("persistentManager.tooManyActive", new Integer(sessions.length))); int toswap = sessions.length - getMaxActiveSessions(); long timeNow = System.currentTimeMillis(); for (int i = 0; i < sessions.length && toswap > 0; i++) { synchronized(sessions[i].persistence_locker){ int timeIdle = // Truncate, do not round up (int) ((timeNow - sessions[i].getLastAccessedTime()) / 1000L); if (timeIdle > minIdleSwap) { if(log.isDebugEnabled()) log.debug(sm.getString ("persistentManager.swapTooManyActive", sessions[i].getIdInternal(), new Integer(timeIdle))); try { swapOut(sessions[i]); } catch (IOException e) { ; // This is logged in writeSession() } toswap--; } } } } /** * Back up idle sessions. */ protected void processMaxIdleBackups() { if (!isStarted() || maxIdleBackup < 0) return; Session sessions[] = findSessions(); long timeNow = System.currentTimeMillis(); // Back up all sessions idle longer than maxIdleBackup if (maxIdleBackup >= 0) { for (int i = 0; i < sessions.length; i++) { StandardSession session = (StandardSession) sessions[i]; synchronized(session.persistence_locker){ if (!session.isValid()) continue; int timeIdle = // Truncate, do not round up (int) ((timeNow - session.getLastAccessedTime()) / 1000L); if (timeIdle > maxIdleBackup) { if (log.isDebugEnabled()) log.debug(sm.getString ("persistentManager.backupMaxIdle", session.getIdInternal(), new Integer(timeIdle))); try { writeSession(session); } catch (IOException e) { ; // This is logged in writeSession() } } } } } } So, currently one could very easily have a session and have some issues. Your user would have a time when they thought they set something or your code could think this then immediately the changes are lost. Anyways, that is what looks to be the issue and a workable fix. The only place I saw other issues was inside of: java/org/apache/catalina/valves/PersistentValve.java where it incorrectly grabs the store from the PersistentManager and uses it directly instead of using the manager API. To me this is bad in that the manager is not able to be the manager and this other logic is accessing the store directly and should never happen...unless it is used only in test cases etc. The only way you could possibly *sort of* workaround this issue would be to use a HttpSessionActivationListener, but even then it would be hard to code up a 100% reliable solution. You can see how this would *sort of* be a way to work around the issue by looking at the class StandardSession method passivate , and see the events will be thrown to alert code the session will passivate, but it would be pretty complicated and ugly to protect the session completely. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee. --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
