Re: svn commit: r1600109 - in /tomcat/trunk/java/org/apache/coyote: ./ ajp/ http11/ spdy/
2014-06-04 15:25 GMT+04:00 ma...@apache.org:
Author: markt
Date: Wed Jun 4 11:25:51 2014
New Revision: 1600109
URL: http://svn.apache.org/r1600109
Log:
Refactoring.
Switch from a boolean to an Enum for error state so we can differentiate
between an error that requires the connection is closed after the current
response is completed and an error that requires that the connection is
closed immediately.
This commit should be a NO-OP. While the different error states are set, the
only the presence of an error (or not) is tested - i.e. no change from the
implementation prior to this commit.
Try to be consistent when an error occurs. Set the status code first (if
required), then set the error state and finally log (if required).
Added:
tomcat/trunk/java/org/apache/coyote/ErrorState.java (with props)
Modified:
tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java
tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
tomcat/trunk/java/org/apache/coyote/ajp/AjpNio2Processor.java
tomcat/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java
tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Processor.java
tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java
Modified: tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java?rev=1600109r1=1600108r2=1600109view=diff
==
--- tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java Wed Jun 4
11:25:51 2014
@@ -40,9 +40,9 @@ public abstract class AbstractProcessor
protected SocketWrapperS socketWrapper = null;
/**
- * Error flag.
+ * Error state for the request/response currently being processed.
*/
-protected boolean error;
+private ErrorState errorState;
You have to assign ErrorState.NONE here by default.
Otherwise I expect setErrorState to fail with NPE.
+protected void setErrorState(ErrorState errorState) {
+this.errorState = this.errorState.getMostSevere(errorState);
+}
+
+
+protected void resetErrorState() {
+errorState = ErrorState.NONE;
+}
+
+
+protected ErrorState getErrorState() {
+return errorState;
+}
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1600109 - in /tomcat/trunk/java/org/apache/coyote: ./ ajp/ http11/ spdy/
On 04/06/2014 12:36, Konstantin Kolinko wrote: 2014-06-04 15:25 GMT+04:00 ma...@apache.org: Author: markt Date: Wed Jun 4 11:25:51 2014 New Revision: 1600109 URL: http://svn.apache.org/r1600109 Log: Refactoring. Switch from a boolean to an Enum for error state so we can differentiate between an error that requires the connection is closed after the current response is completed and an error that requires that the connection is closed immediately. This commit should be a NO-OP. While the different error states are set, the only the presence of an error (or not) is tested - i.e. no change from the implementation prior to this commit. Try to be consistent when an error occurs. Set the status code first (if required), then set the error state and finally log (if required). Added: tomcat/trunk/java/org/apache/coyote/ErrorState.java (with props) Modified: tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java tomcat/trunk/java/org/apache/coyote/ajp/AjpNio2Processor.java tomcat/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Processor.java tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java Modified: tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java?rev=1600109r1=1600108r2=1600109view=diff == --- tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java (original) +++ tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java Wed Jun 4 11:25:51 2014 @@ -40,9 +40,9 @@ public abstract class AbstractProcessor protected SocketWrapperS socketWrapper = null; /** - * Error flag. + * Error state for the request/response currently being processed. */ -protected boolean error; +private ErrorState errorState; You have to assign ErrorState.NONE here by default. Otherwise I expect setErrorState to fail with NPE. Currently not required as resetErrorState() is always called before any call to setErrorState(). Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1600109 - in /tomcat/trunk/java/org/apache/coyote: ./ ajp/ http11/ spdy/
2014-06-04 15:47 GMT+04:00 Mark Thomas ma...@apache.org: On 04/06/2014 12:36, Konstantin Kolinko wrote: 2014-06-04 15:25 GMT+04:00 ma...@apache.org: Author: markt Date: Wed Jun 4 11:25:51 2014 New Revision: 1600109 URL: http://svn.apache.org/r1600109 Log: Refactoring. Switch from a boolean to an Enum for error state so we can differentiate between an error that requires the connection is closed after the current response is completed and an error that requires that the connection is closed immediately. This commit should be a NO-OP. While the different error states are set, the only the presence of an error (or not) is tested - i.e. no change from the implementation prior to this commit. Try to be consistent when an error occurs. Set the status code first (if required), then set the error state and finally log (if required). Added: tomcat/trunk/java/org/apache/coyote/ErrorState.java (with props) Modified: tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java tomcat/trunk/java/org/apache/coyote/ajp/AjpNio2Processor.java tomcat/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Processor.java tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java Modified: tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java?rev=1600109r1=1600108r2=1600109view=diff == --- tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java (original) +++ tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java Wed Jun 4 11:25:51 2014 @@ -40,9 +40,9 @@ public abstract class AbstractProcessor protected SocketWrapperS socketWrapper = null; /** - * Error flag. + * Error state for the request/response currently being processed. */ -protected boolean error; +private ErrorState errorState; You have to assign ErrorState.NONE here by default. Otherwise I expect setErrorState to fail with NPE. Currently not required as resetErrorState() is always called before any call to setErrorState(). For HTTP, AJP - OK. I see that resetErrorState() is called in process(). (I did not notice it at the first glance). For AJP : why resetErrorState() is not called in recycle()? (The HTTP processor calls it). For SpdyProcessor - broken. It never calls resetErrorState(). Best regards, Konstantin Kolinko - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1600109 - in /tomcat/trunk/java/org/apache/coyote: ./ ajp/ http11/ spdy/
On 04/06/2014 12:57, Konstantin Kolinko wrote: 2014-06-04 15:47 GMT+04:00 Mark Thomas ma...@apache.org: On 04/06/2014 12:36, Konstantin Kolinko wrote: 2014-06-04 15:25 GMT+04:00 ma...@apache.org: Author: markt Date: Wed Jun 4 11:25:51 2014 New Revision: 1600109 URL: http://svn.apache.org/r1600109 Log: Refactoring. Switch from a boolean to an Enum for error state so we can differentiate between an error that requires the connection is closed after the current response is completed and an error that requires that the connection is closed immediately. This commit should be a NO-OP. While the different error states are set, the only the presence of an error (or not) is tested - i.e. no change from the implementation prior to this commit. Try to be consistent when an error occurs. Set the status code first (if required), then set the error state and finally log (if required). Added: tomcat/trunk/java/org/apache/coyote/ErrorState.java (with props) Modified: tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java tomcat/trunk/java/org/apache/coyote/ajp/AjpNio2Processor.java tomcat/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Processor.java tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java Modified: tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java?rev=1600109r1=1600108r2=1600109view=diff == --- tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java (original) +++ tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java Wed Jun 4 11:25:51 2014 @@ -40,9 +40,9 @@ public abstract class AbstractProcessor protected SocketWrapperS socketWrapper = null; /** - * Error flag. + * Error state for the request/response currently being processed. */ -protected boolean error; +private ErrorState errorState; You have to assign ErrorState.NONE here by default. Otherwise I expect setErrorState to fail with NPE. Currently not required as resetErrorState() is always called before any call to setErrorState(). For HTTP, AJP - OK. I see that resetErrorState() is called in process(). (I did not notice it at the first glance). For AJP : why resetErrorState() is not called in recycle()? (The HTTP processor calls it). Because that is what the code did previously. I deliberately opted not to do several refactorings so the commit more obviously introduced no functional changes. From a consistency point of view, I think that call makes more sense in recycle than in process() which means errorState will need to be initialised. For SpdyProcessor - broken. It never calls resetErrorState(). Agreed. This will be fixed as a side-effect of the changes above. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1600109 - in /tomcat/trunk/java/org/apache/coyote: ./ ajp/ http11/ spdy/
2014-06-04 16:52 GMT+04:00 Mark Thomas ma...@apache.org: On 04/06/2014 12:57, Konstantin Kolinko wrote: 2014-06-04 15:47 GMT+04:00 Mark Thomas ma...@apache.org: On 04/06/2014 12:36, Konstantin Kolinko wrote: 2014-06-04 15:25 GMT+04:00 ma...@apache.org: Author: markt Date: Wed Jun 4 11:25:51 2014 New Revision: 1600109 URL: http://svn.apache.org/r1600109 Log: Refactoring. Switch from a boolean to an Enum for error state so we can differentiate between an error that requires the connection is closed after the current response is completed and an error that requires that the connection is closed immediately. This commit should be a NO-OP. While the different error states are set, the only the presence of an error (or not) is tested - i.e. no change from the implementation prior to this commit. Try to be consistent when an error occurs. Set the status code first (if required), then set the error state and finally log (if required). Added: tomcat/trunk/java/org/apache/coyote/ErrorState.java (with props) Modified: tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java tomcat/trunk/java/org/apache/coyote/ajp/AjpNio2Processor.java tomcat/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Processor.java tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java Modified: tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java /** - * Error flag. + * Error state for the request/response currently being processed. */ -protected boolean error; +private ErrorState errorState; You have to assign ErrorState.NONE here by default. Otherwise I expect setErrorState to fail with NPE. Currently not required as resetErrorState() is always called before any call to setErrorState(). For HTTP, AJP - OK. I see that resetErrorState() is called in process(). (I did not notice it at the first glance). For AJP : why resetErrorState() is not called in recycle()? (The HTTP processor calls it). Because that is what the code did previously. I deliberately opted not to do several refactorings so the commit more obviously introduced no functional changes. From a consistency point of view, I think that call makes more sense in recycle than in process() which means errorState will need to be initialised. For SpdyProcessor - broken. It never calls resetErrorState(). Agreed. This will be fixed as a side-effect of the changes above. Ack. I see. Thank you. Best regards, Konstantin Kolinko - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
