You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by kk...@apache.org on 2009/12/18 04:43:43 UTC
svn commit: r892120 - /tomcat/tc6.0.x/trunk/STATUS.txt
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
Modified: tomcat/tc6.0.x/trunk/STATUS.txt
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=892120&r1=892119&r2=892120&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/STATUS.txt (original)
+++ tomcat/tc6.0.x/trunk/STATUS.txt Fri Dec 18 03:43:42 2009
@@ -362,7 +362,7 @@
* Prevent lost log messages on shutdown
http://svn.apache.org/viewvc?rev=888072&view=rev
- +1: markt, jim
+ +1: markt, jim, kkolinko
-1:
* Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=47537
@@ -405,6 +405,21 @@
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.
+
+ Re implementation:
+ if (session == null) return false; // return early and do not bother with thread class loader
+
* Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=47836
Don't keep TLD/listener info between reloads
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r892120 - /tomcat/tc6.0.x/trunk/STATUS.txt
Posted by Mark Thomas <ma...@apache.org>.
On 23/12/2009 03:08, Konstantin Kolinko wrote:
> 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?
Moving the fix to StandardSession.expire() works for me.
Given that in normal circumstances the TCCL is set for the entire
method, I'd set the TCCL outside the loop that iterates over the
listeners and reset it just after the loop.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r892120 - /tomcat/tc6.0.x/trunk/STATUS.txt
Posted by Konstantin Kolinko <kn...@gmail.com>.
2009/12/18 Mark Thomas <ma...@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
Re: svn commit: r892120 - /tomcat/tc6.0.x/trunk/STATUS.txt
Posted by Mark Thomas <ma...@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
>
> Modified: tomcat/tc6.0.x/trunk/STATUS.txt
> URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=892120&r1=892119&r2=892120&view=diff
> ==============================================================================
> --- tomcat/tc6.0.x/trunk/STATUS.txt (original)
> +++ tomcat/tc6.0.x/trunk/STATUS.txt Fri Dec 18 03:43:42 2009
> @@ -362,7 +362,7 @@
>
> * Prevent lost log messages on shutdown
> http://svn.apache.org/viewvc?rev=888072&view=rev
> - +1: markt, jim
> + +1: markt, jim, kkolinko
> -1:
>
> * Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=47537
> @@ -405,6 +405,21 @@
> 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 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 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.
> +
> + Re implementation:
> + if (session == null) return false; // return early and do not bother with thread class loader
Yep. That makes sense. I'll add that now.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org