This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push:
new a5b3b07 Fix BZ 64974 - non-blocking I/O + pipelining could drop
requests
a5b3b07 is described below
commit a5b3b07ace0b04db02ef99178099f0998b96e094
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Fri Dec 11 15:59:21 2020 +0000
Fix BZ 64974 - non-blocking I/O + pipelining could drop requests
---
.../apache/coyote/http11/Http11InputBuffer.java | 42 +++++-----
.../apache/coyote/http11/TestHttp11Processor.java | 91 +++++++++++++++++++++-
webapps/docs/changelog.xml | 9 +++
3 files changed, 118 insertions(+), 24 deletions(-)
diff --git a/java/org/apache/coyote/http11/Http11InputBuffer.java
b/java/org/apache/coyote/http11/Http11InputBuffer.java
index 555e3ad..c12df8a 100644
--- a/java/org/apache/coyote/http11/Http11InputBuffer.java
+++ b/java/org/apache/coyote/http11/Http11InputBuffer.java
@@ -654,8 +654,10 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
/**
- * Available bytes in the buffers (note that due to encoding, this may not
- * correspond).
+ * Available bytes in the buffers for the current request.
+ *
+ * Note that when requests are pipelined, the data in byteBuffer may relate
+ * to the next request rather than this one.
*/
int available(boolean read) {
int available;
@@ -666,12 +668,19 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
available = activeFilters[lastActiveFilter].available();
}
- if (available > 0 || !read) {
- return available;
- }
-
+ // Only try a non-blocking read if:
+ // - there is no data in the filters
+ // - the caller requested a read
+ // - there is no data in byteBuffer
+ // - the socket wrapper indicates a read is allowed
+ //
+ // Notes: 1. When pipelined requests are being used available may be
+ // zero even when byteBuffer has data. This is because the data
+ // in byteBuffer is for the next request. We don't want to
+ // attempt a read in this case.
+ // 2. wrapper.hasDataToRead() is present to handle the NIO2 case
try {
- if (wrapper.hasDataToRead()) {
+ if (available == 0 && read && !byteBuffer.hasRemaining() &&
wrapper.hasDataToRead()) {
fill(false);
available = byteBuffer.remaining();
}
@@ -694,22 +703,9 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
* faking non-blocking reads with the blocking IO connector.
*/
boolean isFinished() {
- if (byteBuffer.limit() > byteBuffer.position()) {
- // Data to read in the buffer so not finished
- return false;
- }
-
- /*
- * Don't use fill(false) here because in the following circumstances
- * BIO will block - possibly indefinitely
- * - client is using keep-alive and connection is still open
- * - client has sent the complete request
- * - client has not sent any of the next request (i.e. no pipelining)
- * - application has read the complete request
- */
-
- // Check the InputFilters
-
+ // The active filters have the definitive information on whether or not
+ // the current request body has been read. Note that byteBuffer may
+ // contain pipelined data so is not a good indicator.
if (lastActiveFilter >= 0) {
return activeFilters[lastActiveFilter].isFinished();
} else {
diff --git a/test/org/apache/coyote/http11/TestHttp11Processor.java
b/test/org/apache/coyote/http11/TestHttp11Processor.java
index e4f2e49..b6f7a6b 100644
--- a/test/org/apache/coyote/http11/TestHttp11Processor.java
+++ b/test/org/apache/coyote/http11/TestHttp11Processor.java
@@ -39,7 +39,9 @@ import java.util.concurrent.CountDownLatch;
import jakarta.servlet.AsyncContext;
import jakarta.servlet.DispatcherType;
+import jakarta.servlet.ReadListener;
import jakarta.servlet.ServletException;
+import jakarta.servlet.ServletInputStream;
import jakarta.servlet.http.Cookie;
import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
@@ -381,6 +383,49 @@ public class TestHttp11Processor extends TomcatBaseTest {
@Test
+ public void testPipeliningBug64974() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+
+ // Add protected servlet
+ Wrapper w = Tomcat.addServlet(ctx, "servlet", new Bug64974Servlet());
+ w.setAsyncSupported(true);
+ ctx.addServletMappingDecoded("/foo", "servlet");
+
+ tomcat.start();
+
+ String request =
+ "GET /foo HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: any" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF +
+ "GET /foo HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: any" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF;
+
+ final Client client = new Client(tomcat.getConnector().getLocalPort());
+ client.setRequest(new String[] {request});
+ client.setUseContentLength(true);
+ client.connect();
+ client.sendRequest();
+
+ // Now read the first response
+ client.readResponse(true);
+ Assert.assertFalse(client.isResponse50x());
+ Assert.assertTrue(client.isResponse200());
+ Assert.assertEquals("OK", client.getResponseBody());
+
+ // Read the second response. No need to sleep, read will block until
+ // there is data to process
+ client.readResponse(true);
+ Assert.assertFalse(client.isResponse50x());
+ Assert.assertTrue(client.isResponse200());
+ Assert.assertEquals("OK", client.getResponseBody());
+ }
+
+
+ @Test
public void testChunking11NoContentLength() throws Exception {
Tomcat tomcat = getTomcatInstance();
@@ -572,7 +617,8 @@ public class TestHttp11Processor extends TomcatBaseTest {
String request =
"POST /echo HTTP/1.1" + SimpleHttpClient.CRLF +
- "Host: localhost:" + getPort() + SimpleHttpClient.CRLF;
+ "Host: localhost:" + getPort() + SimpleHttpClient.CRLF +
+ "Content-Length: 10" + SimpleHttpClient.CRLF;
if (useExpectation) {
request += "Expect: 100-continue" + SimpleHttpClient.CRLF;
}
@@ -1770,4 +1816,47 @@ public class TestHttp11Processor extends TomcatBaseTest {
doGet(req, resp);
}
}
+
+
+ private static class Bug64974Servlet extends HttpServlet {
+
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ protected void doGet(HttpServletRequest req, HttpServletResponse resp)
+ throws ServletException, IOException {
+
+ // Get requests can have bodies although these requests don't.
+ // Needs to be async to trigger the problematic code path
+ AsyncContext ac = req.startAsync();
+ ServletInputStream sis = req.getInputStream();
+ // This triggers a call to Http11InputBuffer.avalable(true) which
+ // did not handle the pipelining case.
+ sis.setReadListener(new Bug64974ReadListener());
+ ac.complete();
+
+ resp.setContentType("text/plain");
+ PrintWriter out = resp.getWriter();
+ out.print("OK");
+ }
+ }
+
+
+ private static class Bug64974ReadListener implements ReadListener {
+
+ @Override
+ public void onDataAvailable() throws IOException {
+ // NO-OP
+ }
+
+ @Override
+ public void onAllDataRead() throws IOException {
+ // NO-OP
+ }
+
+ @Override
+ public void onError(Throwable throwable) {
+ // NO-OP
+ }
+ }
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 1d73b64..caff865 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -140,6 +140,15 @@
</add>
</changelog>
</subsection>
+ <subsection name="Coyote">
+ <changelog>
+ <fix>
+ <bug>64974</bug>: Improve handling of pipelined HTTP requests in
+ combination with the Servlet non-blocking IO API. It was possible that
+ some requests could get dropped. (markt)
+ </fix>
+ </changelog>
+ </subsection>
<subsection name="Jasper">
<changelog>
<fix>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
