You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by pe...@apache.org on 2007/06/07 11:34:01 UTC
svn commit: r545127 - in /tomcat/container/tc5.5.x:
catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java
webapps/docs/changelog.xml
Author: pero
Date: Thu Jun 7 02:33:59 2007
New Revision: 545127
URL: http://svn.apache.org/viewvc?view=rev&rev=545127
Log:
Fix correct ApplicationDispatcher forward/include handling after an exception is thrown. This patch fix a memory leak
as <code>STRICT_SERVLET_COMPLIANCE</code> system property is enabled and that cluster crossContext session replication
working correct after an exception is thrown at a RequestDispatcher.forward/include call.
But I don't really like this double try/catch implementation ;-(
Modified:
tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java
tomcat/container/tc5.5.x/webapps/docs/changelog.xml
Modified: tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java
URL: http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java?view=diff&rev=545127&r1=545126&r2=545127
==============================================================================
--- tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java (original)
+++ tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java Thu Jun 7 02:33:59 2007
@@ -336,7 +336,11 @@
HttpServletResponse hresponse = null;
if (response instanceof HttpServletResponse)
hresponse = (HttpServletResponse) response;
-
+
+ IOException ioException = null;
+ ServletException servletException = null;
+ RuntimeException runtimeException = null;
+
// Handle a non-HTTP forward by passing the existing request/response
if ((hrequest == null) || (hresponse == null)) {
@@ -361,11 +365,19 @@
wrequest.setPathInfo(hrequest.getPathInfo());
wrequest.setQueryString(hrequest.getQueryString());
- processRequest(request,response,state);
-
- wrequest.recycle();
- unwrapRequest(state);
-
+ try {
+ processRequest(request,response,state);
+ } catch ( ServletException sexp) {
+ servletException = sexp ;
+ } catch ( IOException iexp) {
+ ioException = iexp ;
+ } catch ( RuntimeException rexp) {
+ runtimeException = rexp ;
+ } finally {
+ wrequest.recycle();
+ // is called at invoke
+ // unwrapRequest(state);
+ }
}
// Handle an HTTP path-based forward
@@ -399,11 +411,19 @@
wrequest.setQueryString(queryString);
wrequest.setQueryParams(queryString);
}
-
- processRequest(request,response,state);
-
- wrequest.recycle();
- unwrapRequest(state);
+ try {
+ processRequest(request,response,state);
+ } catch ( ServletException sexp) {
+ servletException = sexp ;
+ } catch ( IOException iexp) {
+ ioException = iexp ;
+ } catch ( RuntimeException rexp) {
+ runtimeException = rexp ;
+ } finally {
+ wrequest.recycle();
+ // is called at invoke
+ // unwrapRequest(state);
+ }
}
@@ -439,6 +459,14 @@
}
}
+ // Rethrow an exception if one was thrown by the invoked servlet
+ if (ioException != null)
+ throw ioException;
+ if (servletException != null)
+ throw servletException;
+ if (runtimeException != null)
+ throw runtimeException;
+
}
@@ -553,9 +581,12 @@
wrequest.setAttribute(
ApplicationFilterFactory.DISPATCHER_REQUEST_PATH_ATTR,
servletPath);
- invoke(state.outerRequest, state.outerResponse, state);
-
- wrequest.recycle();
+ try {
+ invoke(state.outerRequest, state.outerResponse, state);
+ } finally {
+ wrequest.recycle();
+ }
+
}
// Handle an HTTP path based include
@@ -591,9 +622,11 @@
wrequest.setAttribute(
ApplicationFilterFactory.DISPATCHER_REQUEST_PATH_ATTR,
servletPath);
- invoke(state.outerRequest, state.outerResponse, state);
-
- wrequest.recycle();
+ try {
+ invoke(state.outerRequest, state.outerResponse, state);
+ } finally {
+ wrequest.recycle();
+ }
}
}
Modified: tomcat/container/tc5.5.x/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/webapps/docs/changelog.xml?view=diff&rev=545127&r1=545126&r2=545127
==============================================================================
--- tomcat/container/tc5.5.x/webapps/docs/changelog.xml (original)
+++ tomcat/container/tc5.5.x/webapps/docs/changelog.xml Thu Jun 7 02:33:59 2007
@@ -102,7 +102,12 @@
</add>
<add>
And getSession() operation to StandardManager and DeltaManager JMX Interface (pero)
- </add>
+ </add>
+ <fix>
+ <bug>30949</bug>: Fix correct ApplicationDispatcher forward/include handling after an exception is thrown.
+ This patch fix a memory leak as <code>STRICT_SERVLET_COMPLIANCE</code> system property is enabled and that cluster
+ crossContext session replication working correct after an exception is thrown at a RequestDispatcher.forward/include() call. (pero)
+ </fix>
</changelog>
</subsection>
<subsection name="Webapps">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r545127 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java webapps/docs/changelog.xml
Posted by Peter Rossbach <pr...@objektpark.de>.
Hi Filip and Remy,
Sorry for the very late checkin, but I am little bit busy last two
weeks. I have some trobule with today ApplicationDispatcher
implementation :-(
Thanks, Filip that you revert my patch, we can discuss the change
after tomcat release 5.5.24 is build.
> I see this particular patch is meant to call endAccess when there's
> an exception invoking the request dispatcher, which could cause
> problem in cross context. Well, maybe, because I don't see why a
> single try/finally wouldn't do the same thing, and it does some
> funky stuff like move some method calls.
Yes, this is the problem!
Remy, I very happy to see a simpler fix.
Peter
Am 07.06.2007 um 14:46 schrieb Remy Maucherat:
> Filip Hanik - Dev Lists wrote:
>> Remy Maucherat wrote:
>>> pero@apache.org wrote:
>>>> Author: pero
>>>> Date: Thu Jun 7 02:33:59 2007
>>>> New Revision: 545127
>>>>
>>>> URL: http://svn.apache.org/viewvc?view=rev&rev=545127
>>>> Log:
>>>> Fix correct ApplicationDispatcher forward/include handling after
>>>> an exception is thrown. This patch fix a memory leak
>>>> as <code>STRICT_SERVLET_COMPLIANCE</code> system property is
>>>> enabled and that cluster crossContext session replication
>>>> working correct after an exception is thrown at a
>>>> RequestDispatcher.forward/include call.
>>>> But I don't really like this double try/catch implementation ;-(
>>>
>>> Yes, whatever you are trying to fix, this sort of last minute
>>> patch is not reasonable to me. -1.
>> Peter, I need a response/action from you, as I am supposed to tag
>> and generate our candidate binaries today
>
> I see this particular patch is meant to call endAccess when there's
> an exception invoking the request dispatcher, which could cause
> problem in cross context. Well, maybe, because I don't see why a
> single try/finally wouldn't do the same thing, and it does some
> funky stuff like move some method calls.
>
> This sort of last minute patch is bad practice :( I did it in the
> past, and you usually have "good" reasons for it, but it should be
> explicitly forbidden in favor of proposing delaying the release
> (which may be accepted or not).
>
> Rémy
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r545127 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java
webapps/docs/changelog.xml
Posted by Remy Maucherat <re...@apache.org>.
Filip Hanik - Dev Lists wrote:
> Remy Maucherat wrote:
>> pero@apache.org wrote:
>>> Author: pero
>>> Date: Thu Jun 7 02:33:59 2007
>>> New Revision: 545127
>>>
>>> URL: http://svn.apache.org/viewvc?view=rev&rev=545127
>>> Log:
>>> Fix correct ApplicationDispatcher forward/include handling after an
>>> exception is thrown. This patch fix a memory leak
>>> as <code>STRICT_SERVLET_COMPLIANCE</code> system property is enabled
>>> and that cluster crossContext session replication
>>> working correct after an exception is thrown at a
>>> RequestDispatcher.forward/include call.
>>> But I don't really like this double try/catch implementation ;-(
>>
>> Yes, whatever you are trying to fix, this sort of last minute patch is
>> not reasonable to me. -1.
> Peter, I need a response/action from you, as I am supposed to tag and
> generate our candidate binaries today
I see this particular patch is meant to call endAccess when there's an
exception invoking the request dispatcher, which could cause problem in
cross context. Well, maybe, because I don't see why a single try/finally
wouldn't do the same thing, and it does some funky stuff like move some
method calls.
This sort of last minute patch is bad practice :( I did it in the past,
and you usually have "good" reasons for it, but it should be explicitly
forbidden in favor of proposing delaying the release (which may be
accepted or not).
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r545127 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java
webapps/docs/changelog.xml
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
Remy Maucherat wrote:
> pero@apache.org wrote:
>> Author: pero
>> Date: Thu Jun 7 02:33:59 2007
>> New Revision: 545127
>>
>> URL: http://svn.apache.org/viewvc?view=rev&rev=545127
>> Log:
>> Fix correct ApplicationDispatcher forward/include handling after an
>> exception is thrown. This patch fix a memory leak
>> as <code>STRICT_SERVLET_COMPLIANCE</code> system property is enabled
>> and that cluster crossContext session replication
>> working correct after an exception is thrown at a
>> RequestDispatcher.forward/include call.
>> But I don't really like this double try/catch implementation ;-(
>
> Yes, whatever you are trying to fix, this sort of last minute patch is
> not reasonable to me. -1.
Peter, I need a response/action from you, as I am supposed to tag and
generate our candidate binaries today
Filip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r545127 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/core/ApplicationDispatcher.java
webapps/docs/changelog.xml
Posted by Remy Maucherat <re...@apache.org>.
pero@apache.org wrote:
> Author: pero
> Date: Thu Jun 7 02:33:59 2007
> New Revision: 545127
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=545127
> Log:
> Fix correct ApplicationDispatcher forward/include handling after an exception is thrown. This patch fix a memory leak
> as <code>STRICT_SERVLET_COMPLIANCE</code> system property is enabled and that cluster crossContext session replication
> working correct after an exception is thrown at a RequestDispatcher.forward/include call.
> But I don't really like this double try/catch implementation ;-(
Yes, whatever you are trying to fix, this sort of last minute patch is
not reasonable to me. -1.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org