On Feb 13, 2013, at 4:26 AM, Keiichi Fujino wrote: > 2013/2/13 <ma...@apache.org>: >> Author: markt >> Date: Wed Feb 13 09:28:58 2013 >> New Revision: 1445517 >> >> URL: http://svn.apache.org/r1445517 >> Log: >> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54552 >> Servlet 3.1 >> Implement HttpSessionIdListener and HttpServletRequest#changeSessionId() >> Patch provided by Nick Williams. >> > I think that this code itself is not a problem at all. (I think prety good.)
Thanks! I was a bit nervous about my first code contribution. :-) > However I have a idea of code improvement. > > There seems to be a duplicate code on ManagerBase and DeltaManager. I struggled with that, too. There's a tellNew() method in StandardSession that is responsible for firing the Session.SESSION_CREATED_EVENT event and notifying HttpSessionListeners, and I thought about implementing a similar tellChangedSessionId method in StandardSession. However, since the code for firing the Context.CHANGE_SESSION_ID_EVENT internal event was also duplicated in ManagerBase and DeltaManager, I decided to stick with doing the same and keeping the code alongside those event firings. I would not disagree that the code should be centralized…in both cases. > e.g. > By introducing a following method to ManagerBase and DeltaManager, > we might be able to avoid code duplication. > > == > changeSessionId(Session session, String newId) > changeSessionId(Session session, String newId, > boolean notifySessionListeners, boolean notifyContainerListeners) > == If you are going to add a method to prevent code duplication, I would recommend it be something like StandardSession.tellChangedSessionId(String, String, boolean, boolean) (perhaps right below the tellNew() method) instead of in the ManagerBase class. > > And furthermore, we are changing sessionId in JvmRouteBinderValve. > Change sessionid of JvmRouteBinderValve is completely different from > Manager#changeSessionId. > By using new changeSessionId method, will be able to change sessionId > in a same way. > As a result, JvmRouteSessionIDBinderListener will be unnecessary. I took a look at JvmRouteSessionIDBinderListener and I do not believe it is changing the session ID in the same way (though I could certainly be wrong). It looks to me like it is involved in binding the JVMRoute to the end of the session ID in order to support clustering? I do not think that is the same thing as this. Other areas in the code that "change" the session ID (changeSessionIdOnAuthentication, change replicated from another cluster, HttpServletRequest.changeSessionId()) call the changeSessionId(Session) method on the Manager, which changes the sessionId, does NOT call tellNew(), fires Context.CHANGE_SESSION_ID_EVENT and (now) notifies HttpSessionIdListeners. However, JvmRouteSessionIDBinderListener does it differently: it calls setId() directly on the session, DOES call tellNew() (is this right?) and bypasses the Context.CHANGE_SESSION_ID_EVENT and listeners by not calling Manager.changeSessionId(Session). It could be that this was done this way intentionally, or not. I don't know. I'd like to understand it better. Someone more in-the-know than I can hopefully chime in. > > I'm going to fix these improvements If there is no objections from anyone. > Any objections and comment? If my code can be improved, I am always happy for someone to do it! Nick
smime.p7s
Description: S/MIME cryptographic signature
