From c645d0f7c4abf31a8ca1e56f7982de33efef8e88 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Fri, 1 May 2020 14:40:29 -0500 Subject: [PATCH 1/2] Issue #4835 - Addressing flush/commit with GzipHttpOutputInterceptor Signed-off-by: Joakim Erdfelt --- .../gzip/GzipHttpOutputInterceptor.java | 12 +- .../jetty/servlet/GzipHandlerCommitTest.java | 157 ++++++++++++++++++ 2 files changed, 167 insertions(+), 2 deletions(-) create mode 100644 jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerCommitTest.java diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java index 29ba5434e74..3128a5e7f04 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java @@ -229,7 +229,15 @@ public class GzipHttpOutputInterceptor implements HttpOutput.Interceptor LOG.debug("{} compressing {}", this, _deflater); _state.set(GZState.COMPRESSING); - gzip(content, complete, callback); + if (content.remaining() == 0) + { + // We are committing, and we have no content to compress. + _interceptor.write(content, complete, callback); + } + else + { + gzip(content, complete, callback); + } } else callback.failed(new WritePendingException()); @@ -412,7 +420,7 @@ public class GzipHttpOutputInterceptor implements HttpOutput.Interceptor @Override public String toString() { - return String.format("%s[content=%s last=%b copy=%s buffer=%s deflate=%s", + return String.format("%s[content=%s last=%b copy=%s buffer=%s deflate=%s %s]", super.toString(), BufferUtil.toDetailString(_content), _last, diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerCommitTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerCommitTest.java new file mode 100644 index 00000000000..92c805ae9ef --- /dev/null +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerCommitTest.java @@ -0,0 +1,157 @@ +// +// ======================================================================== +// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.servlet; + +import java.io.IOException; +import java.net.URI; +import java.util.Arrays; +import java.util.concurrent.atomic.AtomicLong; +import javax.servlet.ServletException; +import javax.servlet.ServletOutputStream; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.client.api.Request; +import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.server.handler.gzip.GzipHandler; +import org.eclipse.jetty.util.component.LifeCycle; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.lessThan; + +public class GzipHandlerCommitTest +{ + private static Server server; + private static HttpClient client; + + @BeforeEach + public void startUp() throws Exception + { + server = new Server(); + ServerConnector connector = new ServerConnector(server); + connector.setPort(0); + server.addConnector(connector); + + ServletContextHandler contextHandler = new ServletContextHandler(); + contextHandler.setContextPath("/"); + contextHandler.addServlet(FlushBufferServlet.class, "/flush-buffer/*"); + + GzipHandler gzipHandler = new GzipHandler(); + gzipHandler.setHandler(contextHandler); + + server.setHandler(gzipHandler); + server.start(); + + client = new HttpClient(); + client.start(); + } + + @AfterEach + public void tearDown() + { + LifeCycle.stop(client); + LifeCycle.stop(server); + } + + /** + * A servlet should be able to flush and then produce no content. + */ + @Test + public void testFlushNoContent() throws Exception + { + long delay = 2000; + AtomicLong requestCommitTimestamp = new AtomicLong(-1); + AtomicLong responseBeganTimestamp = new AtomicLong(-1); + AtomicLong responseHeadersTimestamp = new AtomicLong(-1); + URI uri = server.getURI().resolve("/flush-buffer/?size=0&delay=" + delay); + Request request = client.newRequest(uri); + request.header(HttpHeader.CONNECTION, "Close"); + request.onRequestCommit((r) -> requestCommitTimestamp.set(System.currentTimeMillis())); + request.onResponseBegin((r) -> responseBeganTimestamp.set(System.currentTimeMillis())); + request.onResponseHeaders((r) -> responseHeadersTimestamp.set(System.currentTimeMillis())); + ContentResponse response = request.send(); + assertThat("Response status", response.getStatus(), is(200)); + long responseCommitDuration = responseHeadersTimestamp.get() - requestCommitTimestamp.get(); + assertThat("Response headers duration", responseCommitDuration, lessThan(delay)); + } + + /** + * A servlet should be able to flush, response is committed, and then content is produced. + */ + @Test + public void testFlushThenSomeContent() throws Exception + { + int size = 8000; + long delay = 2000; + AtomicLong requestCommitTimestamp = new AtomicLong(-1); + AtomicLong responseBeganTimestamp = new AtomicLong(-1); + AtomicLong responseHeadersTimestamp = new AtomicLong(-1); + URI uri = server.getURI().resolve("/flush-buffer/?size=" + size + "&delay=" + delay); + Request request = client.newRequest(uri); + request.header(HttpHeader.CONNECTION, "Close"); + request.onRequestCommit((r) -> requestCommitTimestamp.set(System.currentTimeMillis())); + request.onResponseBegin((r) -> responseBeganTimestamp.set(System.currentTimeMillis())); + request.onResponseHeaders((r) -> responseHeadersTimestamp.set(System.currentTimeMillis())); + ContentResponse response = request.send(); + assertThat("Response status", response.getStatus(), is(200)); + long responseCommitDuration = responseHeadersTimestamp.get() - requestCommitTimestamp.get(); + assertThat("Response headers duration", responseCommitDuration, lessThan(delay)); + } + + public static class FlushBufferServlet extends HttpServlet + { + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + { + int delay = Integer.parseInt(request.getParameter("delay")); + int size = Integer.parseInt(request.getParameter("size")); + + response.setContentType("text/plain"); + ServletOutputStream out = response.getOutputStream(); + response.flushBuffer(); + + if (delay > 0) + { + try + { + Thread.sleep(delay); + } + catch (InterruptedException ignored) + { + } + } + + byte[] buf = new byte[size]; + if (size > 0) + { + Arrays.fill(buf, (byte)'a'); + out.write(buf); + } + } + } +} From d58da0f7d2e30c732f52a38feaba2974e299bf70 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 4 May 2020 09:22:08 -0500 Subject: [PATCH 2/2] Issue #4824 - Addressing flush/commit with GzipHttpOutputInterceptor Signed-off-by: Joakim Erdfelt --- .../gzip/GzipHttpOutputInterceptor.java | 6 +- .../jetty/servlet/GzipHandlerCommitTest.java | 113 +++++++----------- 2 files changed, 46 insertions(+), 73 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java index 3128a5e7f04..6b9172158a1 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java @@ -229,10 +229,10 @@ public class GzipHttpOutputInterceptor implements HttpOutput.Interceptor LOG.debug("{} compressing {}", this, _deflater); _state.set(GZState.COMPRESSING); - if (content.remaining() == 0) + if (BufferUtil.isEmpty(content)) { - // We are committing, and we have no content to compress. - _interceptor.write(content, complete, callback); + // We are committing, but have no content to compress, so flush empty buffer to write headers. + _interceptor.write(BufferUtil.EMPTY_BUFFER, complete, callback); } else { diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerCommitTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerCommitTest.java index 92c805ae9ef..27360b26399 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerCommitTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerCommitTest.java @@ -21,9 +21,9 @@ package org.eclipse.jetty.servlet; import java.io.IOException; import java.net.URI; import java.util.Arrays; -import java.util.concurrent.atomic.AtomicLong; -import javax.servlet.ServletException; -import javax.servlet.ServletOutputStream; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import javax.servlet.Servlet; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -37,20 +37,19 @@ import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.server.handler.gzip.GzipHandler; import org.eclipse.jetty.util.component.LifeCycle; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.lessThan; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertTrue; public class GzipHandlerCommitTest { - private static Server server; - private static HttpClient client; + private Server server; + private HttpClient client; - @BeforeEach - public void startUp() throws Exception + public void start(Servlet servlet) throws Exception { server = new Server(); ServerConnector connector = new ServerConnector(server); @@ -59,7 +58,8 @@ public class GzipHandlerCommitTest ServletContextHandler contextHandler = new ServletContextHandler(); contextHandler.setContextPath("/"); - contextHandler.addServlet(FlushBufferServlet.class, "/flush-buffer/*"); + ServletHolder servletHolder = new ServletHolder(servlet); + contextHandler.addServlet(servletHolder, "/test/*"); GzipHandler gzipHandler = new GzipHandler(); gzipHandler.setHandler(contextHandler); @@ -78,80 +78,53 @@ public class GzipHandlerCommitTest LifeCycle.stop(server); } - /** - * A servlet should be able to flush and then produce no content. - */ @Test - public void testFlushNoContent() throws Exception + public void testImmediateFlushNoContent() throws Exception { - long delay = 2000; - AtomicLong requestCommitTimestamp = new AtomicLong(-1); - AtomicLong responseBeganTimestamp = new AtomicLong(-1); - AtomicLong responseHeadersTimestamp = new AtomicLong(-1); - URI uri = server.getURI().resolve("/flush-buffer/?size=0&delay=" + delay); + CountDownLatch latch = new CountDownLatch(1); + start(new HttpServlet() + { + @Override + protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException + { + response.flushBuffer(); + assertDoesNotThrow(() -> assertTrue(latch.await(1, TimeUnit.SECONDS))); + } + }); + + URI uri = server.getURI().resolve("/test/"); Request request = client.newRequest(uri); request.header(HttpHeader.CONNECTION, "Close"); - request.onRequestCommit((r) -> requestCommitTimestamp.set(System.currentTimeMillis())); - request.onResponseBegin((r) -> responseBeganTimestamp.set(System.currentTimeMillis())); - request.onResponseHeaders((r) -> responseHeadersTimestamp.set(System.currentTimeMillis())); + request.onResponseHeaders((r) -> latch.countDown()); ContentResponse response = request.send(); assertThat("Response status", response.getStatus(), is(200)); - long responseCommitDuration = responseHeadersTimestamp.get() - requestCommitTimestamp.get(); - assertThat("Response headers duration", responseCommitDuration, lessThan(delay)); } - /** - * A servlet should be able to flush, response is committed, and then content is produced. - */ @Test - public void testFlushThenSomeContent() throws Exception + public void testImmediateFlushWithContent() throws Exception { int size = 8000; - long delay = 2000; - AtomicLong requestCommitTimestamp = new AtomicLong(-1); - AtomicLong responseBeganTimestamp = new AtomicLong(-1); - AtomicLong responseHeadersTimestamp = new AtomicLong(-1); - URI uri = server.getURI().resolve("/flush-buffer/?size=" + size + "&delay=" + delay); + CountDownLatch latch = new CountDownLatch(1); + start(new HttpServlet() + { + @Override + protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException + { + response.flushBuffer(); + assertDoesNotThrow(() -> assertTrue(latch.await(1, TimeUnit.SECONDS))); + response.getOutputStream(); + byte[] buf = new byte[size]; + Arrays.fill(buf, (byte)'a'); + response.getOutputStream().write(buf); + } + }); + + URI uri = server.getURI().resolve("/test/"); Request request = client.newRequest(uri); request.header(HttpHeader.CONNECTION, "Close"); - request.onRequestCommit((r) -> requestCommitTimestamp.set(System.currentTimeMillis())); - request.onResponseBegin((r) -> responseBeganTimestamp.set(System.currentTimeMillis())); - request.onResponseHeaders((r) -> responseHeadersTimestamp.set(System.currentTimeMillis())); + request.onResponseHeaders((r) -> latch.countDown()); ContentResponse response = request.send(); assertThat("Response status", response.getStatus(), is(200)); - long responseCommitDuration = responseHeadersTimestamp.get() - requestCommitTimestamp.get(); - assertThat("Response headers duration", responseCommitDuration, lessThan(delay)); - } - - public static class FlushBufferServlet extends HttpServlet - { - @Override - protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException - { - int delay = Integer.parseInt(request.getParameter("delay")); - int size = Integer.parseInt(request.getParameter("size")); - - response.setContentType("text/plain"); - ServletOutputStream out = response.getOutputStream(); - response.flushBuffer(); - - if (delay > 0) - { - try - { - Thread.sleep(delay); - } - catch (InterruptedException ignored) - { - } - } - - byte[] buf = new byte[size]; - if (size > 0) - { - Arrays.fill(buf, (byte)'a'); - out.write(buf); - } - } + assertThat("Response content size", response.getContent().length, is(size)); } }