You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2014/06/04 13:25:52 UTC
svn commit: r1600109 - in /tomcat/trunk/java/org/apache/coyote: ./ ajp/
http11/ spdy/
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=1600109&r1=1600108&r2=1600109&view=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 SocketWrapper<S> socketWrapper = null;
/**
- * Error flag.
+ * Error state for the request/response currently being processed.
*/
- protected boolean error;
+ private ErrorState errorState;
/**
@@ -69,6 +69,24 @@ public abstract class AbstractProcessor<
/**
+ * Update the current error state to the new error state if the new error
+ * state is more severe than the current error state.
+ */
+ protected void setErrorState(ErrorState errorState) {
+ this.errorState = this.errorState.getMostSevere(errorState);
+ }
+
+
+ protected void resetErrorState() {
+ errorState = ErrorState.NONE;
+ }
+
+
+ protected ErrorState getErrorState() {
+ return errorState;
+ }
+
+ /**
* The endpoint receiving connections that are handled by this processor.
*/
protected AbstractEndpoint<S> getEndpoint() {
Added: tomcat/trunk/java/org/apache/coyote/ErrorState.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/ErrorState.java?rev=1600109&view=auto
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/ErrorState.java (added)
+++ tomcat/trunk/java/org/apache/coyote/ErrorState.java Wed Jun 4 11:25:51 2014
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.coyote;
+
+public enum ErrorState {
+
+ /**
+ * Not in an error state.
+ */
+ NONE(false, 0, true),
+
+ /**
+ * The current request/response is in an error state and while it is safe to
+ * complete the current response it is not safe to continue to use the
+ * existing connection which must be closed once the response has been
+ * completed.
+ */
+ CLOSE_CLEAN(true, 1, true),
+
+ /**
+ * The current request/response is in an error state and it is not safe to
+ * continue to use the existing connection which must be closed immediately.
+ */
+ CLOSE_NOW(true, 2, false);
+
+ private final boolean error;
+ private final int severity;
+ private final boolean ioAllowed;
+
+ private ErrorState(boolean error, int severity, boolean ioAllowed) {
+ this.error = error;
+ this.severity = severity;
+ this.ioAllowed = ioAllowed;
+ }
+
+ public boolean isError() {
+ return error;
+ }
+
+ /**
+ * Compare this ErrorState with the provided ErrorState and return the most
+ * severe.
+ */
+ public ErrorState getMostSevere(ErrorState input) {
+ if (input.severity > this.severity) {
+ return input;
+ } else {
+ return this;
+ }
+ }
+
+ public boolean isIoAllowed() {
+ return ioAllowed;
+ }
+}
Propchange: tomcat/trunk/java/org/apache/coyote/ErrorState.java
------------------------------------------------------------------------------
svn:eol-style = native
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=1600109&r1=1600108&r2=1600109&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java Wed Jun 4 11:25:51 2014
@@ -36,6 +36,7 @@ import org.apache.coyote.AbstractProcess
import org.apache.coyote.ActionCode;
import org.apache.coyote.AsyncContextCallback;
import org.apache.coyote.ByteBufferHolder;
+import org.apache.coyote.ErrorState;
import org.apache.coyote.InputBuffer;
import org.apache.coyote.OutputBuffer;
import org.apache.coyote.Request;
@@ -362,8 +363,7 @@ public abstract class AbstractAjpProcess
try {
finish();
} catch (IOException e) {
- // Set error flag
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
break;
}
@@ -375,15 +375,13 @@ public abstract class AbstractAjpProcess
try {
prepareResponse();
} catch (IOException e) {
- // Set error flag
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
try {
flush(false);
} catch (IOException e) {
- // Set error flag
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
break;
}
@@ -397,8 +395,7 @@ public abstract class AbstractAjpProcess
try {
prepareResponse();
} catch (IOException e) {
- // Set error flag
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
return;
}
}
@@ -406,19 +403,18 @@ public abstract class AbstractAjpProcess
try {
flush(true);
} catch (IOException e) {
- // Set error flag
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
break;
}
case IS_ERROR: {
- ((AtomicBoolean) param).set(error);
+ ((AtomicBoolean) param).set(getErrorState().isError());
break;
}
case DISABLE_SWALLOW_INPUT: {
// TODO: Do not swallow request input but
// make sure we are closing the connection
- error = true;
+ setErrorState(ErrorState.CLOSE_CLEAN);
break;
}
case RESET: {
@@ -691,20 +687,22 @@ public abstract class AbstractAjpProcess
RequestInfo rp = request.getRequestProcessor();
try {
rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);
- error = !getAdapter().asyncDispatch(request, response, status);
+ if(!getAdapter().asyncDispatch(request, response, status)) {
+ setErrorState(ErrorState.CLOSE_NOW);
+ }
resetTimeouts();
} catch (InterruptedIOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
+ setErrorState(ErrorState.CLOSE_NOW);
getLog().error(sm.getString("http11processor.request.process"), t);
- error = true;
}
rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
if (isAsync()) {
- if (error) {
+ if (getErrorState().isError()) {
request.updateCounters();
return SocketState.CLOSED;
} else {
@@ -712,7 +710,7 @@ public abstract class AbstractAjpProcess
}
} else {
request.updateCounters();
- if (error) {
+ if (getErrorState().isError()) {
return SocketState.CLOSED;
} else {
return SocketState.OPEN;
@@ -742,11 +740,11 @@ public abstract class AbstractAjpProcess
boolean cping = false;
// Error flag
- error = false;
+ resetErrorState();
boolean keptAlive = false;
- while (!error && !endpoint.isPaused()) {
+ while (!getErrorState().isError() && !endpoint.isPaused()) {
// Parsing the request header
try {
// Get first message of the request
@@ -769,7 +767,7 @@ public abstract class AbstractAjpProcess
try {
output(pongMessageArray, 0, pongMessageArray.length, true);
} catch (IOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
recycle(false);
continue;
@@ -779,24 +777,24 @@ public abstract class AbstractAjpProcess
if (getLog().isDebugEnabled()) {
getLog().debug("Unexpected message: " + type);
}
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
break;
}
keptAlive = true;
request.setStartTime(System.currentTimeMillis());
} catch (IOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
break;
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
getLog().debug(sm.getString("ajpprocessor.header.error"), t);
// 400 - Bad Request
response.setStatus(400);
+ setErrorState(ErrorState.CLOSE_CLEAN);
getAdapter().log(request, response, 0);
- error = true;
}
- if (!error) {
+ if (!getErrorState().isError()) {
// Setting up filters, and parse some request headers
rp.setStage(org.apache.coyote.Constants.STAGE_PREPARE);
try {
@@ -806,37 +804,37 @@ public abstract class AbstractAjpProcess
getLog().debug(sm.getString("ajpprocessor.request.prepare"), t);
// 500 - Internal Server Error
response.setStatus(500);
+ setErrorState(ErrorState.CLOSE_CLEAN);
getAdapter().log(request, response, 0);
- error = true;
}
}
- if (!error && !cping && endpoint.isPaused()) {
+ if (!getErrorState().isError() && !cping && endpoint.isPaused()) {
// 503 - Service unavailable
response.setStatus(503);
+ setErrorState(ErrorState.CLOSE_CLEAN);
getAdapter().log(request, response, 0);
- error = true;
}
cping = false;
// Process the request in the adapter
- if (!error) {
+ if (!getErrorState().isError()) {
try {
rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);
getAdapter().service(request, response);
} catch (InterruptedIOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
getLog().error(sm.getString("ajpprocessor.request.process"), t);
// 500 - Internal Server Error
response.setStatus(500);
+ setErrorState(ErrorState.CLOSE_CLEAN);
getAdapter().log(request, response, 0);
- error = true;
}
}
- if (isAsync() && !error) {
+ if (isAsync() && !getErrorState().isError()) {
break;
}
@@ -846,13 +844,13 @@ public abstract class AbstractAjpProcess
finish();
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
}
// If there was an error, make sure the request is counted as
// and error, and update the statistics counter
- if (error) {
+ if (getErrorState().isError()) {
response.setStatus(500);
}
request.updateCounters();
@@ -868,14 +866,14 @@ public abstract class AbstractAjpProcess
rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
- if (!error && !endpoint.isPaused()) {
+ if (getErrorState().isError() || endpoint.isPaused()) {
+ return SocketState.CLOSED;
+ } else {
if (isAsync()) {
return SocketState.LONG;
} else {
return SocketState.OPEN;
}
- } else {
- return SocketState.CLOSED;
}
}
@@ -1193,7 +1191,7 @@ public abstract class AbstractAjpProcess
long cl = vMB.getLong();
if (contentLengthSet) {
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
- error = true;
+ setErrorState(ErrorState.CLOSE_CLEAN);
} else {
contentLengthSet = true;
// Set the content-length header for the request
@@ -1308,7 +1306,7 @@ public abstract class AbstractAjpProcess
secret = true;
if (!tmpMB.equals(requiredSecret)) {
response.setStatus(403);
- error = true;
+ setErrorState(ErrorState.CLOSE_CLEAN);
}
}
break;
@@ -1324,7 +1322,7 @@ public abstract class AbstractAjpProcess
// Check if secret was submitted if required
if ((requiredSecret != null) && !secret) {
response.setStatus(403);
- error = true;
+ setErrorState(ErrorState.CLOSE_CLEAN);
}
// Check for a full URI (including protocol://host:port/)
@@ -1357,7 +1355,7 @@ public abstract class AbstractAjpProcess
MessageBytes valueMB = request.getMimeHeaders().getValue("host");
parseHost(valueMB);
- if (error) {
+ if (getErrorState().isError()) {
getAdapter().log(request, response, 0);
}
}
@@ -1375,7 +1373,7 @@ public abstract class AbstractAjpProcess
request.serverName().duplicate(request.localName());
} catch (IOException e) {
response.setStatus(400);
- error = true;
+ setErrorState(ErrorState.CLOSE_CLEAN);
}
return;
}
@@ -1423,9 +1421,9 @@ public abstract class AbstractAjpProcess
int charValue = HexUtils.getDec(valueB[i + valueS]);
if (charValue == -1) {
// Invalid character
- error = true;
// 400 - Bad request
response.setStatus(400);
+ setErrorState(ErrorState.CLOSE_CLEAN);
break;
}
port = port + (charValue * mult);
@@ -1544,8 +1542,7 @@ public abstract class AbstractAjpProcess
try {
prepareResponse();
} catch (IOException e) {
- // Set error flag
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
}
@@ -1560,7 +1557,7 @@ public abstract class AbstractAjpProcess
}
// Add the end message
- if (error) {
+ if (getErrorState().isError()) {
output(endAndCloseMessageArray, 0, endAndCloseMessageArray.length, true);
} else {
output(endMessageArray, 0, endMessageArray.length, true);
@@ -1750,8 +1747,7 @@ public abstract class AbstractAjpProcess
try {
prepareResponse();
} catch (IOException e) {
- // Set error flag
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
}
Modified: tomcat/trunk/java/org/apache/coyote/ajp/AjpNio2Processor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/ajp/AjpNio2Processor.java?rev=1600109&r1=1600108&r2=1600109&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/ajp/AjpNio2Processor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/ajp/AjpNio2Processor.java Wed Jun 4 11:25:51 2014
@@ -104,7 +104,7 @@ public class AjpNio2Processor extends Ab
// The NIO connector uses the timeout configured on the wrapper in the
// poller. Therefore, it needs to be reset once asycn processing has
// finished.
- if (!error && socketWrapper != null &&
+ if (!getErrorState().isError() && socketWrapper != null &&
asyncStateMachine.isAsyncDispatching()) {
long soTimeout = endpoint.getSoTimeout();
Modified: tomcat/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java?rev=1600109&r1=1600108&r2=1600109&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java Wed Jun 4 11:25:51 2014
@@ -82,8 +82,9 @@ public class AjpNioProcessor extends Abs
// The NIO connector uses the timeout configured on the wrapper in the
// poller. Therefore, it needs to be reset once asycn processing has
// finished.
- final NioEndpoint.KeyAttachment attach = (NioEndpoint.KeyAttachment)socketWrapper.getSocket().getAttachment(false);
- if (!error && attach != null &&
+ final NioEndpoint.KeyAttachment attach =
+ (NioEndpoint.KeyAttachment)socketWrapper.getSocket().getAttachment(false);
+ if (!getErrorState().isError() && attach != null &&
asyncStateMachine.isAsyncDispatching()) {
long soTimeout = endpoint.getSoTimeout();
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=1600109&r1=1600108&r2=1600109&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java Wed Jun 4 11:25:51 2014
@@ -30,6 +30,7 @@ import javax.servlet.http.HttpUpgradeHan
import org.apache.coyote.AbstractProcessor;
import org.apache.coyote.ActionCode;
import org.apache.coyote.AsyncContextCallback;
+import org.apache.coyote.ErrorState;
import org.apache.coyote.RequestInfo;
import org.apache.coyote.http11.filters.BufferedInputFilter;
import org.apache.coyote.http11.filters.ChunkedInputFilter;
@@ -699,7 +700,7 @@ public abstract class AbstractHttp11Proc
// Unsupported transfer encoding
// 501 - Unimplemented
response.setStatus(501);
- error = true;
+ setErrorState(ErrorState.CLOSE_CLEAN);
if (getLog().isDebugEnabled()) {
getLog().debug(sm.getString("http11processor.request.prepare") +
" Unsupported transfer encoding [" + encodingName + "]");
@@ -723,8 +724,7 @@ public abstract class AbstractHttp11Proc
try {
getOutputBuffer().endRequest();
} catch (IOException e) {
- // Set error flag
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
break;
}
@@ -739,8 +739,7 @@ public abstract class AbstractHttp11Proc
prepareResponse();
getOutputBuffer().commit();
} catch (IOException e) {
- // Set error flag
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
break;
}
@@ -756,8 +755,7 @@ public abstract class AbstractHttp11Proc
try {
getOutputBuffer().sendAck();
} catch (IOException e) {
- // Set error flag
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
break;
}
@@ -765,20 +763,19 @@ public abstract class AbstractHttp11Proc
try {
getOutputBuffer().flush();
} catch (IOException e) {
- // Set error flag
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
response.setErrorException(e);
}
break;
}
case IS_ERROR: {
- ((AtomicBoolean) param).set(error);
+ ((AtomicBoolean) param).set(getErrorState().isError());
break;
}
case DISABLE_SWALLOW_INPUT: {
// Do not swallow request input and make sure we are closing the
// connection
- error = true;
+ setErrorState(ErrorState.CLOSE_CLEAN);
getInputBuffer().setSwallowInput(false);
break;
}
@@ -880,7 +877,7 @@ public abstract class AbstractHttp11Proc
isReady.set(getOutputBuffer().isReady());
} catch (IOException e) {
getLog().debug("isReady() failed", e);
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
break;
}
@@ -965,7 +962,6 @@ public abstract class AbstractHttp11Proc
getOutputBuffer().init(socketWrapper, endpoint);
// Flags
- error = false;
keepAlive = true;
comet = false;
openSocket = false;
@@ -976,12 +972,13 @@ public abstract class AbstractHttp11Proc
} else {
keptAlive = socketWrapper.isKeptAlive();
}
+ resetErrorState();
if (disableKeepAlive()) {
socketWrapper.setKeepAliveLeft(0);
}
- while (!error && keepAlive && !comet && !isAsync() &&
+ while (!getErrorState().isError() && keepAlive && !comet && !isAsync() &&
httpUpgradeHandler == null && !endpoint.isPaused()) {
// Parsing the request header
@@ -997,7 +994,7 @@ public abstract class AbstractHttp11Proc
if (endpoint.isPaused()) {
// 503 - Service unavailable
response.setStatus(503);
- error = true;
+ setErrorState(ErrorState.CLOSE_CLEAN);
} else {
// Make sure that connectors that are non-blocking during
// header processing (NIO) only set the start time the first
@@ -1025,7 +1022,7 @@ public abstract class AbstractHttp11Proc
getLog().debug(
sm.getString("http11processor.header.parse"), e);
}
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
break;
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
@@ -1047,11 +1044,11 @@ public abstract class AbstractHttp11Proc
}
// 400 - Bad Request
response.setStatus(400);
+ setErrorState(ErrorState.CLOSE_CLEAN);
getAdapter().log(request, response, 0);
- error = true;
}
- if (!error) {
+ if (!getErrorState().isError()) {
// Setting up filters, and parse some request headers
rp.setStage(org.apache.coyote.Constants.STAGE_PREPARE);
try {
@@ -1064,8 +1061,8 @@ public abstract class AbstractHttp11Proc
}
// 500 - Internal Server Error
response.setStatus(500);
+ setErrorState(ErrorState.CLOSE_CLEAN);
getAdapter().log(request, response, 0);
- error = true;
}
}
@@ -1077,7 +1074,7 @@ public abstract class AbstractHttp11Proc
}
// Process the request in the adapter
- if (!error) {
+ if (!getErrorState().isError()) {
try {
rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);
getAdapter().service(request, response);
@@ -1086,22 +1083,25 @@ public abstract class AbstractHttp11Proc
// set the status to 500 and set the errorException.
// If we fail here, then the response is likely already
// committed, so we can't try and set headers.
- if(keepAlive && !error) { // Avoid checking twice.
- error = response.getErrorException() != null ||
- (!isAsync() &&
- statusDropsConnection(response.getStatus()));
+ if(keepAlive && !getErrorState().isError() && (
+ response.getErrorException() != null ||
+ (!isAsync() &&
+ statusDropsConnection(response.getStatus())))) {
+ setErrorState(ErrorState.CLOSE_CLEAN);
}
setCometTimeouts(socketWrapper);
} catch (InterruptedIOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
} catch (HeadersTooLargeException e) {
- error = true;
// The response should not have been committed but check it
// anyway to be safe
- if (!response.isCommitted()) {
+ if (response.isCommitted()) {
+ setErrorState(ErrorState.CLOSE_NOW);
+ } else {
response.reset();
response.setStatus(500);
- response.setHeader("Connection", "close");
+ setErrorState(ErrorState.CLOSE_CLEAN);
+ response.setHeader("Connection", "close"); // TODO: Remove
}
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
@@ -1109,8 +1109,8 @@ public abstract class AbstractHttp11Proc
"http11processor.request.process"), t);
// 500 - Internal Server Error
response.setStatus(500);
+ setErrorState(ErrorState.CLOSE_CLEAN);
getAdapter().log(request, response, 0);
- error = true;
}
}
@@ -1118,7 +1118,7 @@ public abstract class AbstractHttp11Proc
rp.setStage(org.apache.coyote.Constants.STAGE_ENDINPUT);
if (!isAsync() && !comet) {
- if (error) {
+ if (getErrorState().isError()) {
// If we know we are closing the connection, don't drain
// input. This way uploading a 100GB file doesn't tie up the
// thread if the servlet has rejected it.
@@ -1141,12 +1141,12 @@ public abstract class AbstractHttp11Proc
// If there was an error, make sure the request is counted as
// and error, and update the statistics counter
- if (error) {
+ if (getErrorState().isError()) {
response.setStatus(500);
}
request.updateCounters();
- if (!isAsync() && !comet || error) {
+ if (!isAsync() && !comet || getErrorState().isError()) {
getInputBuffer().nextRequest();
getOutputBuffer().nextRequest();
}
@@ -1168,7 +1168,7 @@ public abstract class AbstractHttp11Proc
rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
- if (error || endpoint.isPaused()) {
+ if (getErrorState().isError() || endpoint.isPaused()) {
return SocketState.CLOSED;
} else if (isAsync() || comet) {
return SocketState.LONG;
@@ -1223,13 +1223,13 @@ public abstract class AbstractHttp11Proc
} else {
// Unsupported protocol
http11 = false;
- error = true;
// Send 505; Unsupported HTTP version
+ response.setStatus(505);
+ setErrorState(ErrorState.CLOSE_CLEAN);
if (getLog().isDebugEnabled()) {
getLog().debug(sm.getString("http11processor.request.prepare")+
" Unsupported HTTP version \""+protocolMB+"\"");
}
- response.setStatus(505);
}
MessageBytes methodMB = request.method();
@@ -1262,8 +1262,8 @@ public abstract class AbstractHttp11Proc
getInputBuffer().setSwallowInput(false);
expectation = true;
} else {
- error = true;
response.setStatus(HttpServletResponse.SC_EXPECTATION_FAILED);
+ setErrorState(ErrorState.CLOSE_CLEAN);
}
}
@@ -1355,13 +1355,13 @@ public abstract class AbstractHttp11Proc
// Check host header
if (http11 && (valueMB == null)) {
- error = true;
// 400 - Bad request
+ response.setStatus(400);
+ setErrorState(ErrorState.CLOSE_CLEAN);
if (getLog().isDebugEnabled()) {
getLog().debug(sm.getString("http11processor.request.prepare")+
" host header missing");
}
- response.setStatus(400);
}
parseHost(valueMB);
@@ -1375,7 +1375,7 @@ public abstract class AbstractHttp11Proc
contentDelimitation = true;
}
- if (error) {
+ if (getErrorState().isError()) {
getAdapter().log(request, response, 0);
}
}
@@ -1526,7 +1526,7 @@ public abstract class AbstractHttp11Proc
headers.addValue(Constants.CONNECTION).setString(
Constants.CLOSE);
}
- } else if (!http11 && !error) {
+ } else if (!http11 && !getErrorState().isError()) {
headers.addValue(Constants.CONNECTION).setString(Constants.KEEPALIVE);
}
@@ -1616,9 +1616,9 @@ public abstract class AbstractHttp11Proc
int charValue = HexUtils.getDec(valueB[i + valueS]);
if (charValue == -1 || charValue > 9) {
// Invalid character
- error = true;
// 400 - Bad request
response.setStatus(400);
+ setErrorState(ErrorState.CLOSE_CLEAN);
break;
}
port = port + (charValue * mult);
@@ -1669,19 +1669,21 @@ public abstract class AbstractHttp11Proc
RequestInfo rp = request.getRequestProcessor();
try {
rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);
- error = !getAdapter().asyncDispatch(request, response, status);
+ if(!getAdapter().asyncDispatch(request, response, status)) {
+ setErrorState(ErrorState.CLOSE_NOW);
+ }
resetTimeouts();
} catch (InterruptedIOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
+ setErrorState(ErrorState.CLOSE_NOW);
getLog().error(sm.getString("http11processor.request.process"), t);
- error = true;
}
rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
- if (error) {
+ if (getErrorState().isError()) {
return SocketState.CLOSED;
} else if (isAsync()) {
return SocketState.LONG;
@@ -1744,26 +1746,25 @@ public abstract class AbstractHttp11Proc
try {
getInputBuffer().endRequest();
} catch (IOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
- getLog().error(sm.getString("http11processor.request.finish"), t);
// 500 - Internal Server Error
// Can't add a 500 to the access log since that has already been
// written in the Adapter.service method.
response.setStatus(500);
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
+ getLog().error(sm.getString("http11processor.request.finish"), t);
}
try {
getOutputBuffer().endRequest();
} catch (IOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
+ setErrorState(ErrorState.CLOSE_NOW);
getLog().error(sm.getString("http11processor.response.finish"), t);
- error = true;
}
-
}
@@ -1793,6 +1794,7 @@ public abstract class AbstractHttp11Proc
}
httpUpgradeHandler = null;
comet = false;
+ resetErrorState();
recycleInternal();
}
Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java?rev=1600109&r1=1600108&r2=1600109&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java Wed Jun 4 11:25:51 2014
@@ -23,6 +23,7 @@ import java.security.cert.CertificateFac
import java.security.cert.X509Certificate;
import org.apache.coyote.ActionCode;
+import org.apache.coyote.ErrorState;
import org.apache.coyote.RequestInfo;
import org.apache.coyote.http11.filters.BufferedInputFilter;
import org.apache.juli.logging.Log;
@@ -111,21 +112,23 @@ public class Http11AprProcessor extends
try {
rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);
- error = !getAdapter().event(request, response, status);
+ if (!getAdapter().event(request, response, status)) {
+ setErrorState(ErrorState.CLOSE_NOW);
+ }
} catch (InterruptedIOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
- log.error(sm.getString("http11processor.request.process"), t);
// 500 - Internal Server Error
response.setStatus(500);
+ setErrorState(ErrorState.CLOSE_NOW);
getAdapter().log(request, response, 0);
- error = true;
+ log.error(sm.getString("http11processor.request.process"), t);
}
rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
- if (error || status==SocketStatus.STOP) {
+ if (getErrorState().isError() || status==SocketStatus.STOP) {
return SocketState.CLOSED;
} else if (!comet) {
inputBuffer.nextRequest();
@@ -175,8 +178,8 @@ public class Http11AprProcessor extends
if (endpoint.isPaused()) {
// 503 - Service unavailable
response.setStatus(503);
+ setErrorState(ErrorState.CLOSE_CLEAN);
getAdapter().log(request, response, 0);
- error = true;
} else {
return true;
}
@@ -200,7 +203,7 @@ public class Http11AprProcessor extends
protected boolean breakKeepAliveLoop(SocketWrapper<Long> socketWrapper) {
openSocket = keepAlive;
// Do sendfile as needed: add socket to sendfile and end
- if (sendfileData != null && !error) {
+ if (sendfileData != null && !getErrorState().isError()) {
sendfileData.socket = socketWrapper.getSocket().longValue();
sendfileData.keepAlive = keepAlive;
if (!((AprEndpoint)endpoint).getSendfile().add(sendfileData)) {
@@ -212,7 +215,7 @@ public class Http11AprProcessor extends
log.debug(sm.getString(
"http11processor.sendfile.error"));
}
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
} else {
// The sendfile Poller will add the socket to the main
// Poller once sendfile processing is complete
Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Processor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Processor.java?rev=1600109&r1=1600108&r2=1600109&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Processor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11Nio2Processor.java Wed Jun 4 11:25:51 2014
@@ -24,6 +24,7 @@ import java.net.InetSocketAddress;
import javax.net.ssl.SSLEngine;
import org.apache.coyote.ActionCode;
+import org.apache.coyote.ErrorState;
import org.apache.coyote.RequestInfo;
import org.apache.coyote.http11.filters.BufferedInputFilter;
import org.apache.juli.logging.Log;
@@ -92,8 +93,10 @@ public class Http11Nio2Processor extends
RequestInfo rp = request.getRequestProcessor();
try {
rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);
- error = !getAdapter().event(request, response, status);
- if (!error) {
+ if (!getAdapter().event(request, response, status)) {
+ setErrorState(ErrorState.CLOSE_NOW);
+ }
+ if (!getErrorState().isError()) {
if (socketWrapper != null) {
socketWrapper.setComet(comet);
if (comet) {
@@ -114,19 +117,19 @@ public class Http11Nio2Processor extends
}
}
} catch (InterruptedIOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
- log.error(sm.getString("http11processor.request.process"), t);
// 500 - Internal Server Error
response.setStatus(500);
+ setErrorState(ErrorState.CLOSE_NOW);
getAdapter().log(request, response, 0);
- error = true;
+ log.error(sm.getString("http11processor.request.process"), t);
}
rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
- if (error || status==SocketStatus.STOP) {
+ if (getErrorState().isError() || status==SocketStatus.STOP) {
return SocketState.CLOSED;
} else if (!comet) {
if (keepAlive) {
@@ -172,7 +175,7 @@ public class Http11Nio2Processor extends
@Override
protected void resetTimeouts() {
- if (!error && socketWrapper != null &&
+ if (!getErrorState().isError() && socketWrapper != null &&
asyncStateMachine.isAsyncDispatching()) {
long soTimeout = endpoint.getSoTimeout();
@@ -236,8 +239,8 @@ public class Http11Nio2Processor extends
if (endpoint.isPaused()) {
// 503 - Service unavailable
response.setStatus(503);
+ setErrorState(ErrorState.CLOSE_CLEAN);
getAdapter().log(request, response, 0);
- error = true;
} else {
return true;
}
@@ -271,7 +274,7 @@ public class Http11Nio2Processor extends
SocketWrapper<Nio2Channel> socketWrapper) {
openSocket = keepAlive;
// Do sendfile as needed: add socket to sendfile and end
- if (sendfileData != null && !error) {
+ if (sendfileData != null && !getErrorState().isError()) {
((Nio2Endpoint.Nio2SocketWrapper) socketWrapper).setSendfileData(sendfileData);
sendfileData.keepAlive = keepAlive;
if (((Nio2Endpoint) endpoint).processSendfile(
@@ -282,7 +285,7 @@ public class Http11Nio2Processor extends
if (log.isDebugEnabled()) {
log.debug(sm.getString("http11processor.sendfile.error"));
}
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
return true;
}
Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java?rev=1600109&r1=1600108&r2=1600109&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java Wed Jun 4 11:25:51 2014
@@ -24,6 +24,7 @@ import java.nio.channels.SelectionKey;
import javax.net.ssl.SSLEngine;
import org.apache.coyote.ActionCode;
+import org.apache.coyote.ErrorState;
import org.apache.coyote.RequestInfo;
import org.apache.coyote.http11.filters.BufferedInputFilter;
import org.apache.juli.logging.Log;
@@ -93,8 +94,7 @@ public class Http11NioProcessor extends
* @throws IOException error during an I/O operation
*/
@Override
- public SocketState event(SocketStatus status)
- throws IOException {
+ public SocketState event(SocketStatus status) throws IOException {
long soTimeout = endpoint.getSoTimeout();
@@ -102,8 +102,10 @@ public class Http11NioProcessor extends
final NioEndpoint.KeyAttachment attach = (NioEndpoint.KeyAttachment)socketWrapper.getSocket().getAttachment(false);
try {
rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);
- error = !getAdapter().event(request, response, status);
- if ( !error ) {
+ if (!getAdapter().event(request, response, status)) {
+ setErrorState(ErrorState.CLOSE_NOW);
+ }
+ if (!getErrorState().isError()) {
if (attach != null) {
attach.setComet(comet);
if (comet) {
@@ -124,19 +126,19 @@ public class Http11NioProcessor extends
}
}
} catch (InterruptedIOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
- log.error(sm.getString("http11processor.request.process"), t);
// 500 - Internal Server Error
response.setStatus(500);
+ setErrorState(ErrorState.CLOSE_NOW);
+ log.error(sm.getString("http11processor.request.process"), t);
getAdapter().log(request, response, 0);
- error = true;
}
rp.setStage(org.apache.coyote.Constants.STAGE_ENDED);
- if (error || status==SocketStatus.STOP) {
+ if (getErrorState().isError() || status==SocketStatus.STOP) {
return SocketState.CLOSED;
} else if (!comet) {
if (keepAlive) {
@@ -170,7 +172,7 @@ public class Http11NioProcessor extends
@Override
protected void resetTimeouts() {
final NioEndpoint.KeyAttachment attach = (NioEndpoint.KeyAttachment)socketWrapper.getSocket().getAttachment(false);
- if (!error && attach != null &&
+ if (!getErrorState().isError() && attach != null &&
asyncStateMachine.isAsyncDispatching()) {
long soTimeout = endpoint.getSoTimeout();
@@ -234,8 +236,8 @@ public class Http11NioProcessor extends
if (endpoint.isPaused()) {
// 503 - Service unavailable
response.setStatus(503);
+ setErrorState(ErrorState.CLOSE_CLEAN);
getAdapter().log(request, response, 0);
- error = true;
} else {
return true;
}
@@ -271,11 +273,10 @@ public class Http11NioProcessor extends
@Override
- protected boolean breakKeepAliveLoop(
- SocketWrapper<NioChannel> socketWrapper) {
+ protected boolean breakKeepAliveLoop(SocketWrapper<NioChannel> socketWrapper) {
openSocket = keepAlive;
// Do sendfile as needed: add socket to sendfile and end
- if (sendfileData != null && !error) {
+ if (sendfileData != null && !getErrorState().isError()) {
((KeyAttachment) socketWrapper).setSendfileData(sendfileData);
sendfileData.keepAlive = keepAlive;
SelectionKey key = socketWrapper.getSocket().getIOChannel().keyFor(
@@ -289,7 +290,7 @@ public class Http11NioProcessor extends
if (log.isDebugEnabled()) {
log.debug(sm.getString("http11processor.sendfile.error"));
}
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
return true;
}
@@ -297,8 +298,6 @@ public class Http11NioProcessor extends
}
-
-
@Override
public void recycleInternal() {
socketWrapper = null;
@@ -308,7 +307,6 @@ public class Http11NioProcessor extends
// ----------------------------------------------------- ActionHook Methods
-
/**
* Send an action to the connector.
*
Modified: tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java?rev=1600109&r1=1600108&r2=1600109&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java Wed Jun 4 11:25:51 2014
@@ -27,6 +27,7 @@ import javax.servlet.http.HttpUpgradeHan
import org.apache.coyote.AbstractProcessor;
import org.apache.coyote.ActionCode;
import org.apache.coyote.AsyncContextCallback;
+import org.apache.coyote.ErrorState;
import org.apache.coyote.InputBuffer;
import org.apache.coyote.OutputBuffer;
import org.apache.coyote.Request;
@@ -156,15 +157,16 @@ public class SpdyProcessor<S> extends Ab
rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);
getAdapter().service(request, response);
} catch (InterruptedIOException e) {
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
// log.error(sm.getString("ajpprocessor.request.process"), t);
// 500 - Internal Server Error
+ // TODO Log this properly
t.printStackTrace();
response.setStatus(500);
getAdapter().log(request, response, 0);
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
// TODO: async, etc ( detached mode - use a special light protocol)
@@ -175,11 +177,11 @@ public class SpdyProcessor<S> extends Ab
finish();
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
- error = true;
+ setErrorState(ErrorState.CLOSE_NOW);
}
}
- if (error) {
+ if (getErrorState().isError()) {
response.setStatus(500);
}
@@ -240,13 +242,13 @@ public class SpdyProcessor<S> extends Ab
break;
}
case IS_ERROR: {
- ((AtomicBoolean) param).set(error);
+ ((AtomicBoolean) param).set(getErrorState().isError());
break;
}
case DISABLE_SWALLOW_INPUT: {
// TODO: Do not swallow request input but
// make sure we are closing the connection
- error = true;
+ setErrorState(ErrorState.CLOSE_CLEAN);
break;
}
case CLOSE: {
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1600109 - in /tomcat/trunk/java/org/apache/coyote:
./ ajp/ http11/ spdy/
Posted by Konstantin Kolinko <kn...@gmail.com>.
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1600109 - in /tomcat/trunk/java/org/apache/coyote:
./ ajp/ http11/ spdy/
Posted by 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
>>>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java?rev=1600109&r1=1600108&r2=1600109&view=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 SocketWrapper<S> 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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1600109 - in /tomcat/trunk/java/org/apache/coyote:
./ ajp/ http11/ spdy/
Posted by Konstantin Kolinko <kn...@gmail.com>.
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=1600109&r1=1600108&r2=1600109&view=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 SocketWrapper<S> 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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1600109 - in /tomcat/trunk/java/org/apache/coyote:
./ ajp/ http11/ spdy/
Posted by 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=1600109&r1=1600108&r2=1600109&view=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 SocketWrapper<S> 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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1600109 - in /tomcat/trunk/java/org/apache/coyote:
./ ajp/ http11/ spdy/
Posted by Konstantin Kolinko <kn...@gmail.com>.
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=1600109&r1=1600108&r2=1600109&view=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 SocketWrapper<S> 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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org