This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 8.5.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/8.5.x by this push: new ee14812 Fix BZ 64974 - non-blocking I/O + pipelining could drop requests ee14812 is described below commit ee1481265fefb973bc6a585b973267579cf882ee 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 728242e..3fe2b2b 100644 --- a/java/org/apache/coyote/http11/Http11InputBuffer.java +++ b/java/org/apache/coyote/http11/Http11InputBuffer.java @@ -668,8 +668,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; @@ -680,12 +682,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(); } @@ -708,22 +717,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 64d6003..089be58 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 javax.servlet.AsyncContext; import javax.servlet.DispatcherType; +import javax.servlet.ReadListener; import javax.servlet.ServletException; +import javax.servlet.ServletInputStream; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; @@ -383,6 +385,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(); @@ -576,7 +621,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; } @@ -1786,4 +1832,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 645b070..6b1a6c6 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -147,6 +147,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