Bug 328988

Comment 1 David 2010-10-28 17:35:38 EDT

Created attachment 182001 [details]
Zip of Java PassivatingHashSessionManager

Comment 2 Greg Wilkins 2010-10-28 17:58:06 EDT

I'll have a look.  It sounds like a good idea.

Comment 3 Greg Wilkins 2010-10-29 00:30:05 EDT

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

Comment 4 David 2010-10-29 01:03:06 EDT

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

Comment 5 David 2010-10-31 17:38:14 EDT

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...

Comment 6 Greg Wilkins 2010-10-31 19:33:30 EDT

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!

Comment 7 David 2010-11-01 21:59:54 EDT

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!

Comment 8 Greg Wilkins 2010-11-02 01:39:49 EDT

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.

Comment 9 David 2010-11-02 15:32:13 EDT

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.

Comment 10 Greg Wilkins 2010-11-02 22:49:56 EDT

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.

Comment 11 David 2010-11-03 06:01:20 EDT

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.

Comment 12 David 2010-11-03 06:03:14 EDT

Created attachment 182277 [details]
Limit errors caused by non serializable sessions

Comment 13 Greg Wilkins 2010-12-02 10:12:23 EST

applied in jetty-7.2.1.v20101111

Comment 14 Jesse McConnell 2011-09-20 15:52:38 EDT

Resolved -> Closed