[GitHub] [tomcat] malaysf commented on a change in pull request #332: Support sending the 100 continue response when the servlet reads the …
malaysf commented on a change in pull request #332:
URL: https://github.com/apache/tomcat/pull/332#discussion_r477099224
##
File path: test/org/apache/catalina/core/TestStandardContextValve.java
##
@@ -182,4 +186,123 @@ public void requestDestroyed(ServletRequestEvent sre) {
}
}
+
+@Test
+public void test100ContinueDefaultPolicy() throws Exception {
+// the default policy is IMMEDIATELY
+// This test verifies that we get proper 100 Continue responses
+// when the continueHandlingResponsePolicy property is not set
+test100Continue(ContinueHandlingResponsePolicy.IMMEDIATELY);
+}
+
+@Test
+public void test100ContinueSentImmediately() throws Exception {
+final Tomcat tomcat = getTomcatInstance();
+
+final Connector connector = tomcat.getConnector();
+connector.setProperty("continueHandlingResponsePolicy", "immediately");
+
+test100Continue(ContinueHandlingResponsePolicy.IMMEDIATELY);
+}
+
+@Test
+public void test100ContinueSentOnRequestContentRead() throws Exception {
+final Tomcat tomcat = getTomcatInstance();
+
+final Connector connector = tomcat.getConnector();
+final String policyString =
ContinueHandlingResponsePolicy.ON_REQUEST_BODY_READ.toString()
+.toLowerCase(Locale.ENGLISH);
+connector.setProperty("continueHandlingResponsePolicy", policyString);
+
+test100Continue(ContinueHandlingResponsePolicy.ON_REQUEST_BODY_READ);
+}
+
+public void test100Continue(ContinueHandlingResponsePolicy expectedPolicy)
throws Exception {
+final Tomcat tomcat = getTomcatInstance();
+
+// No file system docBase required
+final Context ctx = tomcat.addContext("", null);
+
+// configure the servlet to wait 1 second before reading the request
body
+Tomcat.addServlet(ctx, "echo", new DelayingEchoBodyServlet(1000));
+ctx.addServletMappingDecoded("/echo", "echo");
+
+tomcat.start();
+
+final ExpectationClient client = new ExpectationClient();
+
+client.setPort(tomcat.getConnector().getLocalPort());
+// Expected content doesn't end with a CR-LF so if it isn't chunked
make
+// sure the content length is used as reading it line-by-line will fail
+// since there is no "line".
+client.setUseContentLength(true);
+
+client.connect();
+
+// time how long it takes to send the request headers and get the
+// 100 continue response
+final long startTime = System.currentTimeMillis();
+client.doRequestHeaders();
+final long endTime = System.currentTimeMillis();
+
+final long duration = endTime - startTime;
+
+if(expectedPolicy == ContinueHandlingResponsePolicy.IMMEDIATELY) {
+// the 100 response should be received immediately while
+// the servlet will wait 1 second before responding. 500 ms
+// should be enough time to allow for any slowness that may
+// occur but still differentiate from the 1 second or more
+// expected delay by the ON_REQUEST_BODY_READ policy.
+Assert.assertTrue(duration < 500);
Review comment:
@martin-g I've reworked the interface and tests to differentiate the two
policies without requiring checking the time elapsed. Basically, it can't be
determined which policy was used simply by making a request and checking the
response, so I also added a unit test for Coyote Request directly that mocks
out classes to check that `sendAcknowledgement` is only called when expected.
Can you please take a look at the latest version?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] malaysf commented on a change in pull request #332: Support sending the 100 continue response when the servlet reads the …
malaysf commented on a change in pull request #332:
URL: https://github.com/apache/tomcat/pull/332#discussion_r471649656
##
File path: java/org/apache/coyote/AbstractProtocol.java
##
@@ -262,6 +267,17 @@ public void setConnectionLinger(int connectionLinger) {
endpoint.setConnectionLinger(connectionLinger);
}
+// HTTP specific
properties
+// - queried by
StandardContextValve
+public void setContinueHandlingResponsePolicy(String value) {
+continueHandlingResponsePolicy =
Enum.valueOf(ContinueHandlingResponsePolicy.class, value);
Review comment:
I've added these, thanks for catching!
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] malaysf commented on a change in pull request #332: Support sending the 100 continue response when the servlet reads the …
malaysf commented on a change in pull request #332:
URL: https://github.com/apache/tomcat/pull/332#discussion_r471649313
##
File path: java/org/apache/catalina/connector/Response.java
##
@@ -1197,16 +1197,12 @@ public String encodeUrl(String url) {
public void sendAcknowledgement()
throws IOException {
-if (isCommitted()) {
Review comment:
I've put the `isCommitted` check back into `catalina.connector.Response`
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] malaysf commented on a change in pull request #332: Support sending the 100 continue response when the servlet reads the …
malaysf commented on a change in pull request #332:
URL: https://github.com/apache/tomcat/pull/332#discussion_r471568598
##
File path: java/org/apache/catalina/core/StandardContextValve.java
##
@@ -81,7 +82,17 @@ public final void invoke(Request request, Response response)
// Acknowledge the request
try {
-response.sendAcknowledgement();
+// Acknowledge based on the policy
+final ContinueHandlingResponsePolicy
continueHandlingResponsePolicy = (ContinueHandlingResponsePolicy)
request.getConnector().getProperty("continueHandlingResponsePolicy");
+
Review comment:
thanks, fixed
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] malaysf commented on a change in pull request #332: Support sending the 100 continue response when the servlet reads the …
malaysf commented on a change in pull request #332:
URL: https://github.com/apache/tomcat/pull/332#discussion_r466203459
##
File path: test/org/apache/catalina/core/TestStandardContextValve.java
##
@@ -182,4 +184,66 @@ public void requestDestroyed(ServletRequestEvent sre) {
}
}
+
+@Test
+public void test100ContinueDefaultPolicy() throws Exception {
+// the default policy is IMMEDIATELY
+// This test verifies that we get proper 100 Continue responses
+// when the continueHandlingResponsePolicy property is not set
+final Tomcat tomcat = getTomcatInstance();
+
+final Connector connector = tomcat.getConnector();
+connector.setProperty("continueHandlingResponsePolicy", "IMMEDIATELY");
+
+test100Continue();
+}
+
+@Test
+public void test100ContinueSentImmediately() throws Exception {
+final Tomcat tomcat = getTomcatInstance();
+
+final Connector connector = tomcat.getConnector();
+connector.setProperty("continueHandlingResponsePolicy", "IMMEDIATELY");
+
+test100Continue();
+}
+
+@Test
+public void test100ContinueSentOnRequestContentRead() throws Exception {
+final Tomcat tomcat = getTomcatInstance();
+
+final Connector connector = tomcat.getConnector();
+connector.setProperty("continueHandlingResponsePolicy",
"ON_REQUEST_CONTENT_READ");
+
+test100Continue();
+}
+
+public void test100Continue() throws Exception {
Review comment:
I've reworked the test, please let me know what you think of if you have
other ideas on how to test this change.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] malaysf commented on a change in pull request #332: Support sending the 100 continue response when the servlet reads the …
malaysf commented on a change in pull request #332:
URL: https://github.com/apache/tomcat/pull/332#discussion_r466202972
##
File path: java/org/apache/coyote/ContinueHandlingResponsePolicy.java
##
@@ -0,0 +1,37 @@
+/*
+ * 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;
+
+/**
+ * Enum defining policies on responding to '100-continue' expectations.
+ */
+public enum ContinueHandlingResponsePolicy {
+/**
+ * Tomcat will automatically and always send the 100 intermediate response
Review comment:
I've updated the comment
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] malaysf commented on a change in pull request #332: Support sending the 100 continue response when the servlet reads the …
malaysf commented on a change in pull request #332:
URL: https://github.com/apache/tomcat/pull/332#discussion_r466179556
##
File path: test/org/apache/catalina/core/TestStandardContextValve.java
##
@@ -182,4 +184,66 @@ public void requestDestroyed(ServletRequestEvent sre) {
}
}
+
+@Test
+public void test100ContinueDefaultPolicy() throws Exception {
+// the default policy is IMMEDIATELY
+// This test verifies that we get proper 100 Continue responses
+// when the continueHandlingResponsePolicy property is not set
+final Tomcat tomcat = getTomcatInstance();
+
+final Connector connector = tomcat.getConnector();
+connector.setProperty("continueHandlingResponsePolicy", "IMMEDIATELY");
+
+test100Continue();
+}
+
+@Test
+public void test100ContinueSentImmediately() throws Exception {
+final Tomcat tomcat = getTomcatInstance();
+
+final Connector connector = tomcat.getConnector();
+connector.setProperty("continueHandlingResponsePolicy", "IMMEDIATELY");
+
+test100Continue();
+}
+
+@Test
+public void test100ContinueSentOnRequestContentRead() throws Exception {
+final Tomcat tomcat = getTomcatInstance();
+
+final Connector connector = tomcat.getConnector();
+connector.setProperty("continueHandlingResponsePolicy",
"ON_REQUEST_CONTENT_READ");
+
+test100Continue();
+}
+
+public void test100Continue() throws Exception {
Review comment:
I struggled a bit with writing the tests, in both cases a 100 continue
response is expected. I'll give testing some more thought.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] malaysf commented on a change in pull request #332: Support sending the 100 continue response when the servlet reads the …
malaysf commented on a change in pull request #332:
URL: https://github.com/apache/tomcat/pull/332#discussion_r466178352
##
File path: java/org/apache/coyote/ContinueHandlingResponsePolicy.java
##
@@ -0,0 +1,37 @@
+/*
+ * 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;
+
+/**
+ * Enum defining policies on responding to '100-continue' expectations.
+ */
+public enum ContinueHandlingResponsePolicy {
+/**
+ * Tomcat will automatically and always send the 100 intermediate response
+ *
+ * This is the default behavior
+ */
+IMMEDIATELY,
+
+/**
+ * Send the 100 intermediate response only when the servlet attempts to
+ * read the request's body. This allows the servlet to process the
+ * request headers and possibly respond before asking for the request
Review comment:
I will clarity the comment
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] malaysf commented on a change in pull request #332: Support sending the 100 continue response when the servlet reads the …
malaysf commented on a change in pull request #332:
URL: https://github.com/apache/tomcat/pull/332#discussion_r466174698
##
File path: java/org/apache/catalina/connector/Response.java
##
@@ -1197,16 +1197,12 @@ public String encodeUrl(String url) {
public void sendAcknowledgement()
throws IOException {
-if (isCommitted()) {
Review comment:
The `isCommitted` check moved into `Coyote.Request` so the behavior
should be the same.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] malaysf commented on a change in pull request #332: Support sending the 100 continue response when the servlet reads the …
malaysf commented on a change in pull request #332:
URL: https://github.com/apache/tomcat/pull/332#discussion_r466173884
##
File path: java/org/apache/coyote/Request.java
##
@@ -709,4 +718,13 @@ private static String getCharsetFromContentType(String
contentType) {
return encoding.trim();
}
+
+/**
+ * set whether to acknowledge the request when the request body is
+ * first read from
+ * @param acknowledgeOnFirstRead the value to set
+ */
+public void setAcknowledgeOnRequestBodyRead(boolean
acknowledgeOnFirstRead) {
Review comment:
thanks, missed this one while renaming
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] malaysf commented on a change in pull request #332: Support sending the 100 continue response when the servlet reads the …
malaysf commented on a change in pull request #332:
URL: https://github.com/apache/tomcat/pull/332#discussion_r466173546
##
File path: java/org/apache/coyote/AbstractProtocol.java
##
@@ -262,6 +267,18 @@ public void setConnectionLinger(int connectionLinger) {
endpoint.setConnectionLinger(connectionLinger);
}
+// HTTP specific
properties
+// - queried by
StandardContextValve
+public void setContinueHandlingResponsePolicy(String value) {
+continueHandlingResponsePolicy =
Enum.valueOf(ContinueHandlingResponsePolicy.class, value);
+}
+
+
+//todo(malay) move to AbstractProtocol
Review comment:
sorry, that was a note to myself that I missed removing
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
