From 49ec41f60047248b23dac5140ad583c1c1efd7e0 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 18 May 2016 12:53:44 +1000 Subject: [PATCH] Issue #525 fix blockForContent spin Improved test --- .../jetty/server/HttpChannelOverHttp.java | 6 +- .../jetty/servlet/PostServletTest.java | 118 +++++++++++------- 2 files changed, 77 insertions(+), 47 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java index 121bd15c517..361c55cdd70 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java @@ -361,7 +361,11 @@ public class HttpChannelOverHttp extends HttpChannel implements HttpParser.Reque // Should we delay dispatch until we have some content? // We should not delay if there is no content expect or client is expecting 100 or the response is already committed or the request buffer already has something in it to parse - _delayedForContent = (getHttpConfiguration().isDelayDispatchUntilContent() && _httpConnection.getParser().getContentLength()>0 && !isExpecting100Continue() && !isCommitted() && _httpConnection.isRequestBufferEmpty()); + _delayedForContent = (getHttpConfiguration().isDelayDispatchUntilContent() + && (_httpConnection.getParser().getContentLength()>0 || _httpConnection.getParser().isChunking() ) + && !isExpecting100Continue() + && !isCommitted() + && _httpConnection.isRequestBufferEmpty()); return !_delayedForContent; } diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/PostServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/PostServletTest.java index 113847b394a..71e7b862b88 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/PostServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/PostServletTest.java @@ -18,12 +18,15 @@ package org.eclipse.jetty.servlet; +import java.io.IOException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.server.HttpChannel; import org.eclipse.jetty.server.LocalConnector; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.util.log.Log; @@ -35,32 +38,41 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.*; import static org.junit.Assert.assertThat; public class PostServletTest { private static final Logger LOG = Log.getLogger(PostServletTest.class); + private static final AtomicReference ex0=new AtomicReference<>(); + private static final AtomicReference ex1=new AtomicReference<>(); public static class BasicReadPostServlet extends HttpServlet { protected void doPost(HttpServletRequest request, HttpServletResponse response) { + byte[] buffer = new byte[1024]; try { - response.flushBuffer(); - request.getInputStream().read(); + int len=request.getInputStream().read(buffer); + while(len>0) + { + response.getOutputStream().println("read "+len); + response.getOutputStream().flush(); + len = request.getInputStream().read(buffer); + } } catch (Exception e0) { + ex0.set(e0); try { // this read-call should fail immediately - request.getInputStream().read(); + request.getInputStream().read(buffer); } catch (Exception e1) { + ex1.set(e1); LOG.warn(e1.toString()); } } @@ -73,6 +85,8 @@ public class PostServletTest @Before public void startServer() throws Exception { + ex0.set(null); + ex1.set(null); server = new Server(); connector = new LocalConnector(server); server.addConnector(connector); @@ -98,21 +112,27 @@ public class PostServletTest StringBuilder req = new StringBuilder(); req.append("POST /post HTTP/1.1\r\n"); req.append("Host: localhost\r\n"); - req.append("Connection: close\r\n"); req.append("Transfer-Encoding: chunked\r\n"); req.append("\r\n"); req.append("6\r\n"); req.append("Hello "); req.append("\r\n"); - req.append("6\r\n"); - req.append("World\n"); + req.append("7\r\n"); + req.append("World!\n"); req.append("\r\n"); req.append("0\r\n"); req.append("\r\n"); - String resp = connector.getResponses(req.toString()); + String resp = connector.getResponses(req.toString(),1,TimeUnit.SECONDS); - assertThat("resp", resp, Matchers.containsString("HTTP/1.1 200 OK")); + assertThat("resp", resp, containsString("HTTP/1.1 200 OK")); + assertThat("resp", resp, containsString("chunked")); + assertThat("resp", resp, containsString("read 6")); + assertThat("resp", resp, containsString("read 7")); + assertThat("resp", resp, containsString("\r\n0\r\n")); + + assertThat(ex0.get(),nullValue()); + assertThat(ex1.get(),nullValue()); } @Test @@ -132,12 +152,47 @@ public class PostServletTest try (StacklessLogging scope = new StacklessLogging(ServletHandler.class)) { String resp = connector.getResponses(req.toString()); - assertThat("resp", resp, Matchers.containsString("HTTP/1.1 200 ")); - assertThat("resp", resp, Matchers.containsString("chunked")); - assertThat("resp", resp, not(containsString("\r\n0\r\n"))); + assertThat(resp,is("")); // Aborted before response committed } + assertThat(ex0.get(),not(nullValue())); + assertThat(ex1.get(),not(nullValue())); } + @Test + public void testDeferredBadPost() throws Exception + { + StringBuilder req = new StringBuilder(16*1024); + req.append("POST /post HTTP/1.1\r\n"); + req.append("Host: localhost\r\n"); + req.append("Transfer-Encoding: chunked\r\n"); + req.append("\r\n"); + + try (StacklessLogging scope = new StacklessLogging(ServletHandler.class)) + { + LocalConnector.LocalEndPoint endp=connector.executeRequest(req.toString()); + Thread.sleep(1000); + req.setLength(0); + // intentionally bad (not a valid chunked char here) + for (int i=1024;i-->0;) + req.append("xxxxxxxxxxxx"); + req.append("\r\n"); + req.append("\r\n"); + + endp.addInput(req.toString()); + + endp.waitUntilClosedOrIdleFor(1,TimeUnit.SECONDS); + String resp = endp.takeOutputString(); + + assertThat("resp", resp, containsString("HTTP/1.1 400 ")); + + } + + // null because it was never dispatched! + assertThat(ex0.get(),nullValue()); + assertThat(ex1.get(),nullValue()); + } + + @Test public void testBadSplitPost() throws Exception { @@ -166,40 +221,11 @@ public class PostServletTest endp.waitUntilClosedOrIdleFor(1,TimeUnit.SECONDS); String resp = endp.takeOutputString(); - assertThat("resp", resp, Matchers.containsString("HTTP/1.1 400 ")); + assertThat("resp", resp, containsString("HTTP/1.1 200 ")); + assertThat("resp", resp, not(containsString("\r\n0\r\n"))); // aborted } + assertThat(ex0.get(),not(nullValue())); + assertThat(ex1.get(),not(nullValue())); } - @Test - public void testBadFlushedPost() throws Exception - { - StringBuilder req = new StringBuilder(); - req.append("POST /post HTTP/1.1\r\n"); - req.append("Host: localhost\r\n"); - req.append("Transfer-Encoding: chunked\r\n"); - req.append("\r\n"); - req.append("6\r\n"); - req.append("Flush "); - req.append("\r\n"); - - try (StacklessLogging scope = new StacklessLogging(ServletHandler.class)) - { - LocalConnector.LocalEndPoint endp=connector.executeRequest(req.toString()); - req.setLength(0); - - Thread.sleep(1000); - req.append("x\r\n"); - req.append("World\n"); - req.append("\r\n"); - req.append("0\r\n"); - req.append("\r\n"); - endp.addInput(req.toString()); - - endp.waitUntilClosedOrIdleFor(1,TimeUnit.SECONDS); - String resp = endp.takeOutputString(); - assertThat("resp", resp, Matchers.containsString("HTTP/1.1 200 ")); - assertThat("resp", resp, Matchers.containsString("Transfer-Encoding: chunked")); - assertThat("resp", resp, not(Matchers.containsString("\r\n0\r\n"))); - } - } }