From 73924d277444f5089a13038becb99e965e35afe4 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 16 Oct 2019 14:12:52 +1100 Subject: [PATCH] Issue #4188 Spin in close of GzipHandler (#4198) * Issue #4188 Spin in close of GzipHandler Cleanup and simplify code Signed-off-by: Greg Wilkins * Issue #4188 Spin in close of GzipHandler Fix slice code. Added unit test for it. Signed-off-by: Greg Wilkins * Issue #4188 Spin in close of GzipHandler Fixed last slice. Signed-off-by: Greg Wilkins * cleanup from review Signed-off-by: Greg Wilkins --- .../gzip/GzipHttpOutputInterceptor.java | 171 ++++++++++-------- .../jetty/servlet/GzipHandlerTest.java | 67 +++++++ .../org/eclipse/jetty/util/BufferUtil.java | 14 ++ 3 files changed, 176 insertions(+), 76 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 4a582348277..2589f99c0b2 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 @@ -128,20 +128,8 @@ public class GzipHttpOutputInterceptor implements HttpOutput.Interceptor private void addTrailer() { - int i = _buffer.limit(); - _buffer.limit(i + 8); - - int v = (int)_crc.getValue(); - _buffer.put(i++, (byte)(v & 0xFF)); - _buffer.put(i++, (byte)((v >>> 8) & 0xFF)); - _buffer.put(i++, (byte)((v >>> 16) & 0xFF)); - _buffer.put(i++, (byte)((v >>> 24) & 0xFF)); - - v = _deflater.getTotalIn(); - _buffer.put(i++, (byte)(v & 0xFF)); - _buffer.put(i++, (byte)((v >>> 8) & 0xFF)); - _buffer.put(i++, (byte)((v >>> 16) & 0xFF)); - _buffer.put(i++, (byte)((v >>> 24) & 0xFF)); + BufferUtil.putIntLittleEndian(_buffer, (int)_crc.getValue()); + BufferUtil.putIntLittleEndian(_buffer, _deflater.getTotalIn()); } private void gzip(ByteBuffer content, boolean complete, final Callback callback) @@ -231,8 +219,6 @@ public class GzipHttpOutputInterceptor implements HttpOutput.Interceptor fields.put(GZIP._contentEncoding); _crc.reset(); - _buffer = _channel.getByteBufferPool().acquire(_bufferSize, false); - BufferUtil.fill(_buffer, GZIP_HEADER, 0, GZIP_HEADER.length); // Adjust headers response.setContentLength(-1); @@ -325,82 +311,115 @@ public class GzipHttpOutputInterceptor implements HttpOutput.Interceptor @Override protected Action process() throws Exception { + // If we have no deflator if (_deflater == null) - return Action.SUCCEEDED; - - if (_deflater.needsInput()) { - if (BufferUtil.isEmpty(_content)) + // then the trailer has been generated and written below. + // we have finished compressing the entire content, so + // cleanup and succeed. + if (_buffer != null) { - if (_deflater.finished()) - { - _factory.recycle(_deflater); - _deflater = null; - _channel.getByteBufferPool().release(_buffer); - _buffer = null; - if (_copy != null) - { - _channel.getByteBufferPool().release(_copy); - _copy = null; - } - return Action.SUCCEEDED; - } - - if (!_last) - { - return Action.SUCCEEDED; - } - - _deflater.finish(); + _channel.getByteBufferPool().release(_buffer); + _buffer = null; } - else if (_content.hasArray()) + if (_copy != null) { - byte[] array = _content.array(); - int off = _content.arrayOffset() + _content.position(); - int len = _content.remaining(); - BufferUtil.clear(_content); - - _crc.update(array, off, len); - _deflater.setInput(array, off, len); - if (_last) - _deflater.finish(); - } - else - { - if (_copy == null) - _copy = _channel.getByteBufferPool().acquire(_bufferSize, false); - BufferUtil.clearToFill(_copy); - int took = BufferUtil.put(_content, _copy); - BufferUtil.flipToFlush(_copy, 0); - if (took == 0) - throw new IllegalStateException(); - - byte[] array = _copy.array(); - int off = _copy.arrayOffset() + _copy.position(); - int len = _copy.remaining(); - - _crc.update(array, off, len); - _deflater.setInput(array, off, len); - if (_last && BufferUtil.isEmpty(_content)) - _deflater.finish(); + _channel.getByteBufferPool().release(_copy); + _copy = null; } + return Action.SUCCEEDED; } - BufferUtil.compact(_buffer); - int off = _buffer.arrayOffset() + _buffer.limit(); - int len = _buffer.capacity() - _buffer.limit() - (_last ? 8 : 0); - if (len > 0) + // If we have no buffer + if (_buffer == null) { + // allocate a buffer and add the gzip header + _buffer = _channel.getByteBufferPool().acquire(_bufferSize, false); + BufferUtil.fill(_buffer, GZIP_HEADER, 0, GZIP_HEADER.length); + } + else + { + // otherwise clear the buffer as previous writes will always fully consume. + BufferUtil.clear(_buffer); + } + + // If the deflator is not finished, then compress more data + if (!_deflater.finished()) + { + if (_deflater.needsInput()) + { + // if there is no more content available to compress + // then we are either finished all content or just the current write. + if (BufferUtil.isEmpty(_content)) + { + if (_last) + _deflater.finish(); + else + return Action.SUCCEEDED; + } + else + { + // If there is more content available to compress, we have to make sure + // it is available in an array for the current deflator API, maybe slicing + // of content. + ByteBuffer slice; + if (_content.hasArray()) + slice = _content; + else + { + if (_copy == null) + _copy = _channel.getByteBufferPool().acquire(_bufferSize, false); + else + BufferUtil.clear(_copy); + slice = _copy; + BufferUtil.append(_copy, _content); + } + + // transfer the data from the slice to the the deflator + byte[] array = slice.array(); + int off = slice.arrayOffset() + slice.position(); + int len = slice.remaining(); + _crc.update(array, off, len); + _deflater.setInput(array, off, len); // TODO use ByteBuffer API in Jetty-10 + BufferUtil.clear(slice); + if (_last && BufferUtil.isEmpty(_content)) + _deflater.finish(); + } + } + + // deflate the content into the available space in the buffer + int off = _buffer.arrayOffset() + _buffer.limit(); + int len = BufferUtil.space(_buffer); int produced = _deflater.deflate(_buffer.array(), off, len, _syncFlush ? Deflater.SYNC_FLUSH : Deflater.NO_FLUSH); _buffer.limit(_buffer.limit() + produced); } - boolean finished = _deflater.finished(); - if (finished) + // If we have finished deflation and there is room for the trailer. + if (_deflater.finished() && BufferUtil.space(_buffer) >= 8) + { + // add the trailer and recycle the deflator to flag that we will have had completeSuccess when + // the write below completes. addTrailer(); + _factory.recycle(_deflater); + _deflater = null; + } - _interceptor.write(_buffer, finished, this); + // write the compressed buffer. + _interceptor.write(_buffer, _deflater == null, this); return Action.SCHEDULED; } + + @Override + public String toString() + { + return String.format("%s[content=%s last=%b copy=%s buffer=%s deflate=%s", + super.toString(), + BufferUtil.toDetailString(_content), + _last, + BufferUtil.toDetailString(_copy), + BufferUtil.toDetailString(_buffer), + _deflater, + _deflater != null && _deflater.finished() ? "(finished)" : ""); + } } } diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java index ba7495fee1f..e7c3c6118c0 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerTest.java @@ -45,9 +45,11 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.http.HttpTester; +import org.eclipse.jetty.server.HttpOutput; import org.eclipse.jetty.server.LocalConnector; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.handler.gzip.GzipHandler; +import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; @@ -116,6 +118,7 @@ public class GzipHandlerTest servlets.addServletWithMapping(EchoServlet.class, "/echo/*"); servlets.addServletWithMapping(DumpServlet.class, "/dump/*"); servlets.addServletWithMapping(AsyncServlet.class, "/async/*"); + servlets.addServletWithMapping(BufferServlet.class, "/buffer/*"); servlets.addFilterWithMapping(CheckFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST)); _server.start(); @@ -221,6 +224,19 @@ public class GzipHandlerTest } } + public static class BufferServlet extends HttpServlet + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException + { + HttpOutput out = (HttpOutput)response.getOutputStream(); + ByteBuffer buffer = BufferUtil.toBuffer(__bytes).asReadOnlyBuffer(); + response.setContentLength(buffer.remaining()); + response.setContentType("text/plain"); + out.write(buffer); + } + } + public static class EchoServlet extends HttpServlet { @Override @@ -357,6 +373,32 @@ public class GzipHandlerTest assertEquals(__content, testOut.toString("UTF8")); } + @Test + public void testBufferResponse() throws Exception + { + // generated and parsed test + HttpTester.Request request = HttpTester.newRequest(); + HttpTester.Response response; + + request.setMethod("GET"); + request.setURI("/ctx/buffer/info"); + request.setVersion("HTTP/1.0"); + request.setHeader("Host", "tester"); + request.setHeader("accept-encoding", "gzip"); + + response = HttpTester.parseResponse(_connector.getResponse(request.generate())); + + assertThat(response.getStatus(), is(200)); + assertThat(response.get("Content-Encoding"), Matchers.equalToIgnoringCase("gzip")); + assertThat(response.getCSV("Vary", false), Matchers.contains("Accept-Encoding")); + + InputStream testIn = new GZIPInputStream(new ByteArrayInputStream(response.getContentBytes())); + ByteArrayOutputStream testOut = new ByteArrayOutputStream(); + IO.copy(testIn, testOut); + + assertEquals(__content, testOut.toString("UTF8")); + } + @Test public void testAsyncLargeResponse() throws Exception { @@ -387,6 +429,31 @@ public class GzipHandlerTest assertEquals(__content, new String(Arrays.copyOfRange(bytes,i * __bytes.length, (i + 1) * __bytes.length), StandardCharsets.UTF_8), "chunk " + i); } + @Test + public void testAsyncEmptyResponse() throws Exception + { + int writes = 0; + _server.getChildHandlerByClass(GzipHandler.class).setMinGzipSize(0); + + // generated and parsed test + HttpTester.Request request = HttpTester.newRequest(); + HttpTester.Response response; + + request.setMethod("GET"); + request.setURI("/ctx/async/info?writes=" + writes); + request.setVersion("HTTP/1.0"); + request.setHeader("Host", "tester"); + request.setHeader("accept-encoding", "gzip"); + + response = HttpTester.parseResponse(_connector.getResponse(request.generate())); + + assertThat(response.getStatus(), is(200)); + assertThat(response.get("Content-Encoding"), Matchers.equalToIgnoringCase("gzip")); + assertThat(response.getCSV("Vary", false), Matchers.contains("Accept-Encoding")); + + + } + @Test public void testGzipHandlerWithMultipleAcceptEncodingHeaders() throws Exception { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java index 452c1be848c..ee54e2726ed 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java @@ -219,6 +219,20 @@ public class BufferUtil buffer.position(position); } + /** Put an integer little endian + * @param buffer The buffer to put to + * @param value The value to put. + */ + public static void putIntLittleEndian(ByteBuffer buffer, int value) + { + int p = flipToFill(buffer); + buffer.put((byte)(value & 0xFF)); + buffer.put((byte)((value >>> 8) & 0xFF)); + buffer.put((byte)((value >>> 16) & 0xFF)); + buffer.put((byte)((value >>> 24) & 0xFF)); + flipToFlush(buffer, p); + } + /** * Convert a ByteBuffer to a byte array. *