Community
Participate
Working Groups
Build Identifier: All We run a very busy site that has quite large amounts of session data that is plagued by site scrapers around the world and our own users creating and abandoning sessions and thus consuming large amounts of RAM. We minimize this currently by having a short session timeout which we extend when a purchase looks likely. I would think this is common issue for many users which they resolve by adding hideous amounts of RAM to their servers. Have created a variation of the HashSessionManager called PassivatingHashSessionManager which when the session has been idle for a configured amount of time, passivates the session by saving and clearing the session attribute map. If the session is referenced again, the session is activated by reloading the attribute map. Will attach a zip of this new class shortly. Is there a chance that this class, or the feature it provides, being incorporated into Jetty? Reproducible: Always
Created attachment 182001 [details] Zip of Java PassivatingHashSessionManager
I'll have a look. It sounds like a good idea.
I think this functionality can be added to the HashSessionManager directly rather than creating a subclass. This will be able to reuse a lot of the load/save session machinery already in that class. David, would you be able to prepare a patch against that class? If not, we'll give this a go sometime in the next few weeks. cheers
I thought that too, but wanted to make a simpler class to demonstrate. I wasn't keen on the existing session serialization using two loops; keys then values rather then key value, key value in single loop. Also I didn't need to actually serialize all the data, just the attribute map and wasn't interested in saving sessions across restarts and could see some potential for confusion between the two. However, if providing a patch means inclusion sooner rather than later then I will revisit ! Will do so against 7.2 source (thanks for the new source bundle). (In reply to comment #3) > I think this functionality can be added to the HashSessionManager directly > rather than creating a subclass. This will be able to reuse a lot of the > load/save session machinery already in that class. > > David, would you be able to prepare a patch against that class? If not, we'll > give this a go sometime in the next few weeks. > > cheers
Created attachment 182122 [details] Updated HashSessionManager.java Have attached zip of updated HashSessionManager that reuses existing serialization to implement the "idling" of sessions. If is based off jetty-distribution-7.2.0.v20101020. I changed from calling this passivation/activation to idle/deidle because of the already existing use of the passivation/activation terms. I had to change the public method restoreSession to take an existing session. Guess this might break a unit test or two but they can be updated to pass in null for the existing session. It may be that more refactoring will be required because this functionality might be wanted by some on the JDBCSessionManager...
Created attachment 182125 [details] Updated HashSessionManager This is your updated hashsessionmanager converted to a patch on the project (in the form we'd normally handle them). I've added a little bit more javadoc (although probably not enough) and renamed/reformatted for our coding style. I also changed abstractSessionManager, so that if getAttribute is called on an idle session, then deIdle is called. Also changed a few cases of if (test) then synchronize {change test status } which could result in a race if a session is being idled just as a new request comes in. Finally, I think willPassivate and didActivate should be called before save and after restore. So this looks pretty good and almost ready to commit to mainline. I think we just need some test harnesses for that. If you have time to update the tests to check this new behaviour, that would be a great help. thanks again for the contribution!
Will see if I can get time, but can't commit to it at the moment... Give me a week and then review. Thank you very much for responsiveness. (In reply to comment #6) > Created an attachment (id=182125) [details] > Updated HashSessionManager > > This is your updated hashsessionmanager converted to a patch on the project (in > the form we'd normally handle them). > > I've added a little bit more javadoc (although probably not enough) and > renamed/reformatted for our coding style. > > I also changed abstractSessionManager, so that if getAttribute is called on an > idle session, then deIdle is called. > > Also changed a few cases of > if (test) then synchronize {change test status } > > which could result in a race if a session is being idled just as a new request > comes in. > > Finally, I think willPassivate and didActivate should be called before save and > after restore. > > So this looks pretty good and almost ready to commit to mainline. I think we > just need some test harnesses for that. If you have time to update the tests > to check this new behaviour, that would be a great help. > > thanks again for the contribution!
Created attachment 182187 [details] passivation without synchronization This is an updated patch that adds your passivation, but also removes dependence on the global session manager lock. The hash map used is a concurrent hashmap, so synchronization should mostly not be required and this should improve performance on multi-core machines.
Looks good. Load/stress testing here has highlighted that there was too much synchronization so its good that there has been some reduction made. You made getAttribute synchronize on the session in case the session had been idled. I don't see how(or why) anybody could call getAttribute on a session without having first got the session which would have already deidled the session. Yes, it is possible to spin off a thread that hangs on to a session and accessed it after the idle save period but it would be a bizarre thing to do. ...still, I don't believe this additional synchronization will be noticeable. ...You updated to use generics on collections in a couple of places but left the existing type casts in place. HashSessionManager lines 498,499, 658.
The synchronization in getAttribute is there mostly as a memory barrier rather than as mutual exclusion. It should be pretty low cost. With asynchronous servlets, it is more likely that references to sessions are kept and used outside of the scope of a request, so checking for idle session is also pretty low cost. I've done some further improvements to the synchronization and optimization of the map handling. This is now committed to trunk r2466 even though we lack a test harness for the idle save. I thought it best to start getting some exposure of the synchronization changes.
Will attach shorty an attempted patch which updates the HashSessionManager such that once there has been an issue saving/idling a session to disk, flags the session as having a failed save. If a save has failed, no more attempts will be made to persist that session. Was concerned that if there were issues persisting sessions, that the logs could be flooded with huge numbers of errors. This patch will at least limit the errors to one per session. Have been looking at the existing test code. Did not see any existing test for the HashSessionManager session saving. Had thought to find such tests and extend to cover session idling. So the issue on tests is larger than I thought.
Created attachment 182277 [details] Limit errors caused by non serializable sessions
applied in jetty-7.2.1.v20101111
Resolved -> Closed
