Sounds good.
On Jul 31, 2008, at 10:57 AM, Les Hazlewood wrote:
Yep, that's what I'm thinking.
I'll commit the adjustments in just a few minutes so you can see if
it works
for you or not...
On Thu, Jul 31, 2008 at 10:46 AM, Jeremy Haile <[EMAIL PROTECTED]>
wrote:
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
