> Should it be immutable? If so, we could provide a wrapper >> 'ImmutableSession' implementation that throws exceptions on any method >> call >> that tries to alter state and delegates to the wrapped Session when just >> reading state, like getAttribute(). The wrapper would only be used during >> listener notification, and the session would be 'unwrapped' for any >> remaining processing after listener notification.... >> > > My thoughts exactly. We'd need to look closely at the session to see what > should be mutable vs immutable. But it's something that security-conscious > framework should think about. Immutability is good in many cases. > > Is this something you think we should do now? Is it something that we >> should even enforce? >> > > Maybe for 1.0, not for 0.9.0.
Actually, I don't see any problems with adding this even today unless you have objections - it is very unintrusive and wouldn't take any time at all. The only methods that I can see that would throw an exception are: setTimeout(long); touch(); stop(); setAttribute(key,value); removeAttribute(key); Everything else is read-only already. In fact, I would prefer that it is in now, to minimize any surprise later. I'm not sure anyone is even using the SessionListener construct yet, so it'd be nice to get this in before that might happen.... > I have no idea what the best practice would be, but for some reason I >> really >> like the sound of the default not allowing modification. It seems more >> logical to me, because those notifications are supposed to indicate that >> the >> session is in an invalid state and probably shouldn't be messed with >> anymore. >> > > Yeah - I like the idea of certain parts of the session being immutable > always - and other parts of the session being wrapped in an immutable > implementation for listener events, etc. > > We do need to think about things like when someone wants to use a session > listener to set an attribute when a session is created. > That may be valid - so this does require some more thought as to what > should be immutable and when it should be immutable. I definitely think its ok for a listener to do whatever they want to the session upon creation. But I do think the wrapper concept should be in place today, and we can tweak exactly what the behavior may do in the future - just starting off by throwing exceptions for the above methods. That ok? > On Jul 30, 2008, at 11:32 AM, Les Hazlewood wrote: >> >>> >>> So in my review of the code in prep for RC1 last night, I found something >>> >>>> I'd never really thought about before: >>>> >>>> Should the Session *Interface* have the two methods getStopTimestamp() >>>> and >>>> isExpired() at all? >>>> >>>> I remember those existing in the implementation specifically to handle >>>> the >>>> case where Sessions were persistent beyond their user lifetime - e.g. in >>>> environments that do user activity reporting, they could organize user >>>> events based on sessions, etc. But the framework couldn't use stopped >>>> or >>>> expired sessions anymore. >>>> >>>> It seems as if implementation methods bubbled their way up into the >>>> interface. >>>> >>>> Would a user ever need to call getStopTimestamp() or isExpired()? >>>> >>>> In our web support for example, these methods would never return >>>> anything >>>> useful: getStopTimestamp() would _always_ return null and isExpired() >>>> would >>>> _always_ return false, because of our WebSessionManager logic: >>>> >>>> When a Subject is being constructed for the incoming Request, the >>>> WebSessionManager it tries to acquire the user's corresponding session >>>> (if >>>> it exists). If it does exist, but that session is invalid (that is, >>>> stopped >>>> or expired), it suppresses resulting InvalidSessionException (prints out >>>> a >>>> trace message really) and returns null to the caller. The caller views >>>> this >>>> as an indication that the session doesn't exist, and it must create a >>>> brand >>>> new one if it needs to use a Session. >>>> >>>> Because of this logic, those two methods are probably meaningless. I >>>> also >>>> think that our logic to ignore invalid sessions and create a brand new >>>> one >>>> when necessary is the right way to go - it is how web containers work, >>>> and >>>> everyone is pretty much happy with that behavior. >>>> >>>> Do you guys agree? Do you see a need to retain them in the interface? >>>> (They >>>> would stay in the implementation because the implementation needs to use >>>> them to determine if in fact it is invalid or not). >>>> >>>> I'm just looking for your thoughts if you have any... >>>> >>>> Les >>>> >>>> >>> >>> >
