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 2014/05/14 02:44:34 UTC
svn commit: r1594436 - in /tomcat/trunk: java/org/apache/catalina/connector/
java/org/apache/coyote/ java/org/apache/coyote/ajp/
java/org/apache/coyote/http11/ webapps/docs/
Author: kkolinko
Date: Wed May 14 00:44:33 2014
New Revision: 1594436
URL: http://svn.apache.org/r1594436
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=56399
When recycling a Coyote request, ensure that Catalina request have been recycled.
Modified:
tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
tomcat/trunk/java/org/apache/catalina/connector/LocalStrings.properties
tomcat/trunk/java/org/apache/coyote/Adapter.java
tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java?rev=1594436&r1=1594435&r2=1594436&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java (original)
+++ tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java Wed May 14 00:44:33 2014
@@ -655,6 +655,41 @@ public class CoyoteAdapter implements Ad
}
+ private static class RecycleRequiredException extends Exception {
+ private static final long serialVersionUID = 1L;
+ }
+
+ @Override
+ public void checkRecycled(org.apache.coyote.Request req,
+ org.apache.coyote.Response res) {
+ Request request = (Request) req.getNote(ADAPTER_NOTES);
+ Response response = (Response) res.getNote(ADAPTER_NOTES);
+ try {
+ if (request != null) {
+ if (request.getContext() != null || request.getHost() != null)
+ throw new RecycleRequiredException();
+ }
+ if (response != null) {
+ if (response.getContentWritten() != 0)
+ throw new RecycleRequiredException();
+ }
+ } catch (RecycleRequiredException e) {
+ String message = sm.getString("coyoteAdapter.checkRecycled");
+ if (connector.getState().isAvailable()) {
+ log.warn(message, e);
+ } else {
+ // There may be some aborted requests.
+ // When connector shuts down, the request and response will not
+ // be reused, so there is no issue to warn about here.
+ log.debug(message, e);
+ }
+ // Log this request, as it has probably skipped the access log.
+ // The log() method will take care of recycling.
+ log(req, res, 0L);
+ }
+ }
+
+
@Override
public String getDomain() {
return connector.getDomain();
Modified: tomcat/trunk/java/org/apache/catalina/connector/LocalStrings.properties
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/connector/LocalStrings.properties?rev=1594436&r1=1594435&r2=1594436&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/connector/LocalStrings.properties (original)
+++ tomcat/trunk/java/org/apache/catalina/connector/LocalStrings.properties Wed May 14 00:44:33 2014
@@ -17,6 +17,7 @@ cometEvent.nullRequest=The event object
coyoteAdapter.accesslogFail=Exception while attempting to add an entry to the access log
coyoteAdapter.asyncDispatch=Exception while processing an asynchronous request
+coyoteAdapter.checkRecycled=A non-recycled request encountered. It will be recycled forcedly.
coyoteAdapter.debug=The variable [{0}] has value [{1}]
coyoteAdapter.parsePathParam=Unable to parse the path parameters using encoding [{0}]. The path parameters in the URL will be ignored.
Modified: tomcat/trunk/java/org/apache/coyote/Adapter.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/Adapter.java?rev=1594436&r1=1594435&r2=1594436&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/Adapter.java (original)
+++ tomcat/trunk/java/org/apache/coyote/Adapter.java Wed May 14 00:44:33 2014
@@ -56,6 +56,19 @@ public interface Adapter {
public void log(Request req, Response res, long time);
/**
+ * Assert that request and response have been recycled. If they have not
+ * then log a warning and force a recycle. This method is called as a safety
+ * check when a processor is being recycled and may be returned to a pool
+ * for reuse.
+ *
+ * @param req
+ * Request
+ * @param res
+ * Response
+ */
+ public void checkRecycled(Request req, Response res);
+
+ /**
* Provide the name of the domain to use to register MBeans for conponents
* associated with the connector.
*
Modified: tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java?rev=1594436&r1=1594435&r2=1594436&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java Wed May 14 00:44:33 2014
@@ -890,6 +890,8 @@ public abstract class AbstractAjpProcess
*/
@Override
public void recycle(boolean socketClosing) {
+ getAdapter().checkRecycled(request, response);
+
asyncStateMachine.recycle();
// Recycle Request object
Modified: tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java?rev=1594436&r1=1594435&r2=1594436&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java Wed May 14 00:44:33 2014
@@ -1744,6 +1744,8 @@ public abstract class AbstractHttp11Proc
@Override
public final void recycle(boolean isSocketClosing) {
+ getAdapter().checkRecycled(request, response);
+
if (getInputBuffer() != null) {
getInputBuffer().recycle();
}
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1594436&r1=1594435&r2=1594436&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Wed May 14 00:44:33 2014
@@ -225,6 +225,10 @@
<bug>56348</bug>: Fix slow asynchronous read when read was performed on
a non-container thread. (markt)
</fix>
+ <add>
+ <bug>56399</bug>: Assert that both Coyote and Catalina request objects
+ have been properly recycled. (kkolinko)
+ </add>
<fix>
<bug>56416</bug>: Correct documentation for default value of socket
linger for the AJP and HTTP connectors. (markt)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1594436 - in /tomcat/trunk: java/org/apache/catalina/connector/
java/org/apache/coyote/ java/org/apache/coyote/ajp/ java/org/apache/coyote/http11/
webapps/docs/
Posted by Mark Thomas <ma...@apache.org>.
On 14/05/2014 01:44, kkolinko@apache.org wrote:
> Author: kkolinko
> Date: Wed May 14 00:44:33 2014
> New Revision: 1594436
>
> URL: http://svn.apache.org/r1594436
> Log:
> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=56399
> When recycling a Coyote request, ensure that Catalina request have been recycled.
>
> Modified:
> tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
> tomcat/trunk/java/org/apache/catalina/connector/LocalStrings.properties
> tomcat/trunk/java/org/apache/coyote/Adapter.java
> tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
> tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
> tomcat/trunk/webapps/docs/changelog.xml
>
> Modified: tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java?rev=1594436&r1=1594435&r2=1594436&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java Wed May 14 00:44:33 2014
> @@ -655,6 +655,41 @@ public class CoyoteAdapter implements Ad
> }
>
>
> + private static class RecycleRequiredException extends Exception {
> + private static final long serialVersionUID = 1L;
> + }
> +
> + @Override
> + public void checkRecycled(org.apache.coyote.Request req,
> + org.apache.coyote.Response res) {
> + Request request = (Request) req.getNote(ADAPTER_NOTES);
> + Response response = (Response) res.getNote(ADAPTER_NOTES);
> + try {
> + if (request != null) {
> + if (request.getContext() != null || request.getHost() != null)
> + throw new RecycleRequiredException();
> + }
> + if (response != null) {
> + if (response.getContentWritten() != 0)
> + throw new RecycleRequiredException();
> + }
> + } catch (RecycleRequiredException e) {
> + String message = sm.getString("coyoteAdapter.checkRecycled");
> + if (connector.getState().isAvailable()) {
> + log.warn(message, e);
> + } else {
> + // There may be some aborted requests.
> + // When connector shuts down, the request and response will not
> + // be reused, so there is no issue to warn about here.
> + log.debug(message, e);
> + }
> + // Log this request, as it has probably skipped the access log.
> + // The log() method will take care of recycling.
> + log(req, res, 0L);
> + }
> + }
Why is an exception being used for flow control? Surely a boolean flag
would be better here especially as this is being called on every
request. I'm close to a -1 on this commit but I'd like to hear the
explanation for the use of an exception first.
<snip/>
> Modified: tomcat/trunk/webapps/docs/changelog.xml
> URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1594436&r1=1594435&r2=1594436&view=diff
> ==============================================================================
> --- tomcat/trunk/webapps/docs/changelog.xml (original)
> +++ tomcat/trunk/webapps/docs/changelog.xml Wed May 14 00:44:33 2014
> @@ -225,6 +225,10 @@
> <bug>56348</bug>: Fix slow asynchronous read when read was performed on
> a non-container thread. (markt)
> </fix>
> + <add>
> + <bug>56399</bug>: Assert that both Coyote and Catalina request objects
> + have been properly recycled. (kkolinko)
> + </add>
I remain of the view that a better solution would be to recycle both
pairs of request and response objects at the same time.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1594436 - in /tomcat/trunk: java/org/apache/catalina/connector/
java/org/apache/coyote/ java/org/apache/coyote/ajp/ java/org/apache/coyote/http11/
webapps/docs/
Posted by Rémy Maucherat <re...@apache.org>.
2014-05-15 14:57 GMT+02:00 Mark Thomas <ma...@apache.org>:
> Konstantin,
> (bcc'd directly as list mail still seems to be delayed by 8-12 hours)
>
> It looks like I am going to need to tag a new 8.0.x release as DBCP is
> currently broken. Please can you revert this commit as per Remy's veto
> ASAP so I can tag 8.0.7 without this change present.
>
> If r1594436 isn't reverted by the time I come to tag 8.0.7 I will revert
> it myself (but I'd rather not have to do that).
>
There's some effort to improve it, and it's not too visible (the
classloader log is far more problematic), so I'm withdrawing the veto.
Rémy
Re: svn commit: r1594436 - in /tomcat/trunk: java/org/apache/catalina/connector/
java/org/apache/coyote/ java/org/apache/coyote/ajp/ java/org/apache/coyote/http11/
webapps/docs/
Posted by Mark Thomas <ma...@apache.org>.
On 14/05/2014 07:48, Rémy Maucherat wrote:
> 2014-05-14 2:44 GMT+02:00 <kk...@apache.org>:
>
>> Author: kkolinko
>> Date: Wed May 14 00:44:33 2014
>> New Revision: 1594436
>>
>> URL: http://svn.apache.org/r1594436
>> Log:
>> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=56399
>> When recycling a Coyote request, ensure that Catalina request have been
>> recycled.
>>
>
> This shouldn't cause regressions (cosmetic or performance). So this needs
> to be refined, and shouldn't be committed for now.
>
> I'll have to -1 this commit until it doesn't cause issues. The code can be
> left in instead, but the calls to getAdapter().checkRecycled(request,
> response) could be disabled with a system property flag.
Konstantin,
(bcc'd directly as list mail still seems to be delayed by 8-12 hours)
It looks like I am going to need to tag a new 8.0.x release as DBCP is
currently broken. Please can you revert this commit as per Remy's veto
ASAP so I can tag 8.0.7 without this change present.
If r1594436 isn't reverted by the time I come to tag 8.0.7 I will revert
it myself (but I'd rather not have to do that).
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1594436 - in /tomcat/trunk: java/org/apache/catalina/connector/
java/org/apache/coyote/ java/org/apache/coyote/ajp/ java/org/apache/coyote/http11/
webapps/docs/
Posted by Rémy Maucherat <re...@apache.org>.
2014-05-14 2:44 GMT+02:00 <kk...@apache.org>:
> Author: kkolinko
> Date: Wed May 14 00:44:33 2014
> New Revision: 1594436
>
> URL: http://svn.apache.org/r1594436
> Log:
> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=56399
> When recycling a Coyote request, ensure that Catalina request have been
> recycled.
>
This shouldn't cause regressions (cosmetic or performance). So this needs
to be refined, and shouldn't be committed for now.
I'll have to -1 this commit until it doesn't cause issues. The code can be
left in instead, but the calls to getAdapter().checkRecycled(request,
response) could be disabled with a system property flag.
Rémy