From 3c815e23c713746847fbf10d2b161439be49a4c5 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 16 May 2022 10:37:36 -0500 Subject: [PATCH 1/5] Issue #7858 - GzipHandler request.isHandled support * Better conditional logic in GzipHandler * Correct test expectations Signed-off-by: Joakim Erdfelt --- .../server/handler/gzip/GzipHandler.java | 11 +- .../servlet/GzipHandlerIsHandledTest.java | 131 ++++++++++++++++++ 2 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java index 65becfe88bd6..bad174604a77 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java @@ -678,7 +678,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques // Handle request inflation HttpFields httpFields = baseRequest.getHttpFields(); boolean inflated = _inflateBufferSize > 0 && httpFields.contains(HttpHeader.CONTENT_ENCODING, "gzip") && isPathInflatable(path); - if (inflated) + if (inflated && !baseRequest.isHandled()) { if (LOG.isDebugEnabled()) LOG.debug("{} inflate {}", this, request); @@ -688,6 +688,15 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques baseRequest.getHttpInput().addInterceptor(gzipHttpInputInterceptor); } + // From here on out, the response output gzip determination is made + + // Don't attempt to modify the response output if it's already committed. + if (response.isCommitted()) + { + _handler.handle(target, baseRequest, request, response); + return; + } + // Are we already being gzipped? HttpOutput out = baseRequest.getResponse().getHttpOutput(); HttpOutput.Interceptor interceptor = out.getInterceptor(); diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java new file mode 100644 index 000000000000..8cc3be9a9d59 --- /dev/null +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java @@ -0,0 +1,131 @@ +// +// ======================================================================== +// Copyright (c) 1995-2022 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.util.concurrent.ExecutionException; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.TimeoutException; +import javax.servlet.ServletException; +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.server.Handler; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.server.handler.AbstractHandler; +import org.eclipse.jetty.server.handler.DefaultHandler; +import org.eclipse.jetty.server.handler.HandlerCollection; +import org.eclipse.jetty.server.handler.ResourceHandler; +import org.eclipse.jetty.server.handler.gzip.GzipHandler; +import org.eclipse.jetty.toolchain.test.jupiter.WorkDir; +import org.eclipse.jetty.util.component.LifeCycle; +import org.eclipse.jetty.util.resource.PathResource; +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.containsString; +import static org.hamcrest.Matchers.is; + +/** + * Tests of behavior of GzipHandler when Request.isHandled() or Response.isCommitted() is true + */ +public class GzipHandlerIsHandledTest +{ + public WorkDir workDir; + + private Server server; + private HttpClient client; + public LinkedBlockingQueue events = new LinkedBlockingQueue<>(); + + @BeforeEach + public void setUp() throws Exception + { + server = new Server(); + ServerConnector connector = new ServerConnector(server); + connector.setPort(0); + server.addConnector(connector); + + HandlerCollection handlers = new HandlerCollection(); + + ResourceHandler resourceHandler = new ResourceHandler(); + resourceHandler.setBaseResource(new PathResource(workDir.getPath())); + resourceHandler.setDirectoriesListed(true); + resourceHandler.setHandler(new EventHandler(events, "ResourceHandler")); + + GzipHandler gzipHandler = new GzipHandler(); + gzipHandler.setMinGzipSize(32); + gzipHandler.setHandler(new EventHandler(events, "GzipHandler-wrapped-handler")); + + handlers.setHandlers(new Handler[]{resourceHandler, gzipHandler, new DefaultHandler()}); + + server.setHandler(handlers); + server.start(); + + client = new HttpClient(); + client.start(); + } + + @AfterEach + public void tearDown() + { + LifeCycle.stop(client); + LifeCycle.stop(server); + } + + @Test + public void testRequest() throws ExecutionException, InterruptedException, TimeoutException + { + ContentResponse response = client.GET(server.getURI().resolve("/")); + assertThat("response.status", response.getStatus(), is(200)); + // we should have received a directory listing from the ResourceHandler + assertThat("response.content", response.getContentAsString(), containsString("Directory: /")); + // resource handler should have handled the request + // the gzip handler and default handlers should have been executed, seeing as this is a HandlerCollection + // but the gzip handler should not have acted on the request, as the response is committed + assertThat("One event should have been recorded", events.size(), is(1)); + // the event handler should see the request.isHandled = true + // and response.isCommitted = true as the gzip handler didn't really do anything due to these + // states and let the wrapped handler (the EventHandler in this case) make the call on what it should do. + assertThat("Event indicating that GzipHandler-wrapped-handler ran", events.remove(), is("GzipHandler-wrapped-handler [request.isHandled=true, response.isCommitted=true]")); + } + + private static class EventHandler extends AbstractHandler + { + private final LinkedBlockingQueue events; + private final String action; + + public EventHandler(LinkedBlockingQueue events, String action) + { + this.events = events; + this.action = action; + } + + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + events.offer(String.format("%s [request.isHandled=%b, response.isCommitted=%b]", action, baseRequest.isHandled(), response.isCommitted())); + } + } +} From ac10e99a577a9947e8412be87ad24c470b23efee Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 17 May 2022 15:19:58 -0500 Subject: [PATCH 2/5] Fixing license header Signed-off-by: Joakim Erdfelt --- .../servlet/GzipHandlerIsHandledTest.java | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java index 8cc3be9a9d59..7bef23cb07e9 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java @@ -1,19 +1,14 @@ // -// ======================================================================== -// Copyright (c) 1995-2022 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. +// ======================================================================== +// Copyright (c) 1995-2022 Mort Bay Consulting Pty Ltd and others. // -// The Eclipse Public License is available at -// http://www.eclipse.org/legal/epl-v10.html +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. // -// 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. -// ======================================================================== +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== // package org.eclipse.jetty.servlet; From 96abb953b807b987a19d4902c608a86cd0423985 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 18 May 2022 13:22:43 -0500 Subject: [PATCH 3/5] Fixing based on review Signed-off-by: Joakim Erdfelt --- .../eclipse/jetty/server/handler/gzip/GzipHandler.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java index bad174604a77..cf546174e060 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java @@ -664,6 +664,12 @@ public void setInflateBufferSize(int size) @Override public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { + if (baseRequest.isHandled()) + { + _handler.handle(target, baseRequest, request, response); + return; + } + final ServletContext context = baseRequest.getServletContext(); final String path = baseRequest.getPathInContext(); LOG.debug("{} handle {} in {}", this, baseRequest, context); @@ -678,7 +684,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques // Handle request inflation HttpFields httpFields = baseRequest.getHttpFields(); boolean inflated = _inflateBufferSize > 0 && httpFields.contains(HttpHeader.CONTENT_ENCODING, "gzip") && isPathInflatable(path); - if (inflated && !baseRequest.isHandled()) + if (inflated) { if (LOG.isDebugEnabled()) LOG.debug("{} inflate {}", this, request); From b50afd12684a27d36880b83176734fdaff3b70fc Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 18 May 2022 13:25:24 -0500 Subject: [PATCH 4/5] Use super.handle() where appropriate Signed-off-by: Joakim Erdfelt --- .../jetty/server/handler/gzip/GzipHandler.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java index cf546174e060..e5ccd17d100d 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHandler.java @@ -666,7 +666,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques { if (baseRequest.isHandled()) { - _handler.handle(target, baseRequest, request, response); + super.handle(target, baseRequest, request, response); return; } @@ -677,7 +677,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques if (!_dispatchers.contains(baseRequest.getDispatcherType())) { LOG.debug("{} excluded by dispatcherType {}", this, baseRequest.getDispatcherType()); - _handler.handle(target, baseRequest, request, response); + super.handle(target, baseRequest, request, response); return; } @@ -699,7 +699,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques // Don't attempt to modify the response output if it's already committed. if (response.isCommitted()) { - _handler.handle(target, baseRequest, request, response); + super.handle(target, baseRequest, request, response); return; } @@ -779,7 +779,7 @@ else if (COMMA_GZIP.matcher(field.getValue()).matches()) if (alreadyGzipped) { LOG.debug("{} already intercepting {}", this, request); - _handler.handle(target, baseRequest, request, response); + super.handle(target, baseRequest, request, response); return; } @@ -787,7 +787,7 @@ else if (COMMA_GZIP.matcher(field.getValue()).matches()) if (!_methods.test(baseRequest.getMethod())) { LOG.debug("{} excluded by method {}", this, request); - _handler.handle(target, baseRequest, request, response); + super.handle(target, baseRequest, request, response); return; } @@ -796,7 +796,7 @@ else if (COMMA_GZIP.matcher(field.getValue()).matches()) if (!isPathGzipable(path)) { LOG.debug("{} excluded by path {}", this, request); - _handler.handle(target, baseRequest, request, response); + super.handle(target, baseRequest, request, response); return; } @@ -809,7 +809,7 @@ else if (COMMA_GZIP.matcher(field.getValue()).matches()) { LOG.debug("{} excluded by path suffix mime type {}", this, request); // handle normally without setting vary header - _handler.handle(target, baseRequest, request, response); + super.handle(target, baseRequest, request, response); return; } } @@ -819,9 +819,7 @@ else if (COMMA_GZIP.matcher(field.getValue()).matches()) { // install interceptor and handle out.setInterceptor(new GzipHttpOutputInterceptor(this, getVaryField(), baseRequest.getHttpChannel(), origInterceptor, isSyncFlush())); - - if (_handler != null) - _handler.handle(target, baseRequest, request, response); + super.handle(target, baseRequest, request, response); } finally { From 0154c8d9385e15089729d1d74d3abc7975cb8334 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 18 May 2022 13:33:40 -0500 Subject: [PATCH 5/5] Make test easier to follow Signed-off-by: Joakim Erdfelt --- .../servlet/GzipHandlerIsHandledTest.java | 38 +++++++++---------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java index 7bef23cb07e9..8bef4367c23f 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/GzipHandlerIsHandledTest.java @@ -14,9 +14,7 @@ package org.eclipse.jetty.servlet; import java.io.IOException; -import java.util.concurrent.ExecutionException; import java.util.concurrent.LinkedBlockingQueue; -import java.util.concurrent.TimeoutException; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -36,7 +34,6 @@ import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.util.resource.PathResource; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; @@ -54,28 +51,14 @@ public class GzipHandlerIsHandledTest private HttpClient client; public LinkedBlockingQueue events = new LinkedBlockingQueue<>(); - @BeforeEach - public void setUp() throws Exception + public void startServer(Handler rootHandler) throws Exception { server = new Server(); ServerConnector connector = new ServerConnector(server); connector.setPort(0); server.addConnector(connector); - HandlerCollection handlers = new HandlerCollection(); - - ResourceHandler resourceHandler = new ResourceHandler(); - resourceHandler.setBaseResource(new PathResource(workDir.getPath())); - resourceHandler.setDirectoriesListed(true); - resourceHandler.setHandler(new EventHandler(events, "ResourceHandler")); - - GzipHandler gzipHandler = new GzipHandler(); - gzipHandler.setMinGzipSize(32); - gzipHandler.setHandler(new EventHandler(events, "GzipHandler-wrapped-handler")); - - handlers.setHandlers(new Handler[]{resourceHandler, gzipHandler, new DefaultHandler()}); - - server.setHandler(handlers); + server.setHandler(rootHandler); server.start(); client = new HttpClient(); @@ -90,8 +73,23 @@ public void tearDown() } @Test - public void testRequest() throws ExecutionException, InterruptedException, TimeoutException + public void testRequest() throws Exception { + HandlerCollection handlers = new HandlerCollection(); + + ResourceHandler resourceHandler = new ResourceHandler(); + resourceHandler.setBaseResource(new PathResource(workDir.getPath())); + resourceHandler.setDirectoriesListed(true); + resourceHandler.setHandler(new EventHandler(events, "ResourceHandler")); + + GzipHandler gzipHandler = new GzipHandler(); + gzipHandler.setMinGzipSize(32); + gzipHandler.setHandler(new EventHandler(events, "GzipHandler-wrapped-handler")); + + handlers.setHandlers(new Handler[]{resourceHandler, gzipHandler, new DefaultHandler()}); + + startServer(handlers); + ContentResponse response = client.GET(server.getURI().resolve("/")); assertThat("response.status", response.getStatus(), is(200)); // we should have received a directory listing from the ResourceHandler