I don't have a problem adding it in now. So would onStart() receive a
mutable copy, but onStop/onExpiration receives an immutable copy?
On Jul 31, 2008, at 10:42 AM, Les Hazlewood wrote:
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
