tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Kolinko <knst.koli...@gmail.com>
Subject Re: svn commit: r892120 - /tomcat/tc6.0.x/trunk/STATUS.txt
Date Wed, 23 Dec 2009 03:08:30 GMT
2009/12/18 Mark Thomas <markt@apache.org>:
> On 18/12/2009 03:43, kkolinko@apache.org wrote:
>> Author: kkolinko
>> Date: Fri Dec 18 03:43:42 2009
>> New Revision: 892120
>>
>> URL: http://svn.apache.org/viewvc?rev=892120&view=rev
>> Log:
>> votes
>>
>> Modified:
>>     tomcat/tc6.0.x/trunk/STATUS.txt
>>

>>  * Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=47774
>>    Ensure web application class loader is used when calling session listeners
>>    http://svn.apache.org/viewvc?rev=890530&view=rev
>>    +1: markt
>>    -1:
>> +   0: kkolinko:
>> +       Re approach:
>> +         Request.isRequestedSessionIdValid() serves as implementation for
>> +         HttpServletRequest.isRequestedSessionIdValid().
>> +         I think that ClassLoader should already be set when calling this
>> +         method.
>> +
>> +         I mean, the problem is elsewhere. Supposedly in CoyoteAdapter.
>> +         parseSessionCookiesId(), but may be earlier in the call chain.
>> +
>> +         I won't oppose the patch. I have to think a bit more about it.
>
> I did a search of places where this is called from. Some set the tccl
> set, some don't. I decided to go for the safe option and implement this
> in Request.isRequestedSessionIdValid() to ensure the tccl was always
> set, regardless of what called the method.
>
> Yes, in some cases the tccl will be set unnecessarily. I thought about
> testing if the tccl has already been set to the webapp classloader but
> having looked at the implementation for
> Thread.setsetContextClassLoader() I decided to go for the simpler approach.
>

I would say that it is not so easy to review all places where
isValid() is called.

I see two more approaches available here:
A). Place fix in StandardSession.expire(boolean)
That is, wrap the call to  listener.sessionDestroyed(event);  with
setting and then clearing the TCCL.
The fireContainerEvent() calls above and below that line probably do
not need webapp's classLoader, though I am not sure.
The Context reference is available there, so Loader can be obtained.

B). Implement a wrapper for HttpSessionListener that sets TCCL before
calling the wrapped listener. Add wrappers when the listeners are
added to the webapp.

The if (!(listeners[j] instanceof HttpSessionListener)) check in
StandardSession.notify() code means that it has to be a wrapper that
specifically implements the HttpSessionListener interface.

Any thoughts?

Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message