From e578a2db157809d24694ff5163b241d153b268e9 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 26 Apr 2023 11:57:41 +1000 Subject: [PATCH 1/5] Issue #9637 - Update behaviour and add testing for ServletRequestListener Signed-off-by: Lachlan Roberts --- .../jetty/ee10/servlet/ErrorHandler.java | 15 +- .../jetty/ee10/servlet/ServletChannel.java | 22 +- .../ee10/servlet/ServletContextHandler.java | 3 +- .../servlet/ServletRequestListenerTest.java | 266 ++++++++++++++++++ 4 files changed, 299 insertions(+), 7 deletions(-) create mode 100644 jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorHandler.java index af82e37aa9b1..79de5f2ec442 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ErrorHandler.java @@ -89,7 +89,9 @@ public boolean handle(Request request, Response response, Callback callback) thr } ServletContextRequest servletContextRequest = Request.as(request, ServletContextRequest.class); - + HttpServletRequest httpServletRequest = servletContextRequest.getHttpServletRequest(); + HttpServletResponse httpServletResponse = servletContextRequest.getHttpServletResponse(); + ServletContextHandler contextHandler = servletContextRequest.getContext().getServletContextHandler(); String cacheControl = getCacheControl(); if (cacheControl != null) response.getHeaders().put(HttpHeader.CACHE_CONTROL.asString(), cacheControl); @@ -97,7 +99,7 @@ public boolean handle(Request request, Response response, Callback callback) thr // Look for an error page dispatcher // This logic really should be in ErrorPageErrorHandler, but some implementations extend ErrorHandler // and implement ErrorPageMapper directly, so we do this here in the base class. - String errorPage = (this instanceof ErrorPageMapper) ? ((ErrorPageMapper)this).getErrorPage(servletContextRequest.getHttpServletRequest()) : null; + String errorPage = (this instanceof ErrorPageMapper) ? ((ErrorPageMapper)this).getErrorPage(httpServletRequest) : null; ServletContextHandler.ServletScopedContext context = servletContextRequest.getErrorContext(); Dispatcher errorDispatcher = (errorPage != null && context != null) ? (Dispatcher)context.getServletContext().getRequestDispatcher(errorPage) : null; @@ -106,7 +108,8 @@ public boolean handle(Request request, Response response, Callback callback) thr { try { - errorDispatcher.error(servletContextRequest.getHttpServletRequest(), servletContextRequest.getHttpServletResponse()); + contextHandler.requestInitialized(servletContextRequest, httpServletRequest); + errorDispatcher.error(httpServletRequest, httpServletResponse); callback.succeeded(); return true; } @@ -119,12 +122,16 @@ public boolean handle(Request request, Response response, Callback callback) thr return true; } } + finally + { + contextHandler.requestDestroyed(servletContextRequest, httpServletRequest); + } } String message = (String)request.getAttribute(Dispatcher.ERROR_MESSAGE); if (message == null) message = HttpStatus.getMessage(response.getStatus()); - generateAcceptableResponse(servletContextRequest, servletContextRequest.getHttpServletRequest(), servletContextRequest.getHttpServletResponse(), response.getStatus(), message); + generateAcceptableResponse(servletContextRequest, httpServletRequest, httpServletResponse, response.getStatus(), message); callback.succeeded(); return true; } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 0ca08507cb27..60edfb231afc 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -510,7 +510,7 @@ public boolean handle() // _state.completing(); try (Blocker.Callback blocker = Blocker.callback()) { - dispatch(() -> errorHandler.handle(_servletContextRequest, getResponse(), blocker)); + errorDispatch(() -> errorHandler.handle(_servletContextRequest, getResponse(), blocker)); blocker.block(); } } @@ -670,6 +670,26 @@ private void dispatch(Dispatchable dispatchable) throws Exception } } + private void errorDispatch(Dispatchable dispatchable) throws Exception + { + try + { + _servletContextRequest.getResponse().getHttpOutput().reopen(); + getHttpOutput().reopen(); + _combinedListener.onBeforeDispatch(_servletContextRequest); + dispatchable.dispatch(); + } + catch (Throwable x) + { + _combinedListener.onDispatchFailure(_servletContextRequest, x); + throw x; + } + finally + { + _combinedListener.onAfterDispatch(_servletContextRequest); + } + } + /** *

Sends an error 500, performing a special logic to detect whether the request is suspended, * to avoid concurrent writes from the application.

diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java index 7fabefc9131a..f7eec8fc391f 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java @@ -58,7 +58,6 @@ import jakarta.servlet.descriptor.JspConfigDescriptor; import jakarta.servlet.descriptor.JspPropertyGroupDescriptor; import jakarta.servlet.descriptor.TaglibDescriptor; -import jakarta.servlet.http.HttpFilter; import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; @@ -1466,7 +1465,7 @@ public FilterHolder addFilter(String filterClass, String pathSpec, EnumSet dispatches) + public FilterHolder addFilter(Filter filter, String pathSpec, EnumSet dispatches) { FilterHolder filterHolder = new FilterHolder(filter); getServletHandler().addFilterWithMapping(filterHolder, pathSpec, dispatches); diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java new file mode 100644 index 000000000000..2212a5fd18c7 --- /dev/null +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java @@ -0,0 +1,266 @@ +package org.eclipse.jetty.ee10.servlet; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.EnumSet; +import java.util.List; +import java.util.function.Consumer; + +import jakarta.servlet.AsyncContext; +import jakarta.servlet.DispatcherType; +import jakarta.servlet.Filter; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletRequestEvent; +import jakarta.servlet.ServletRequestListener; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServlet; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.eclipse.jetty.client.ContentResponse; +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.URIUtil; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; + +public class ServletRequestListenerTest +{ + private final List _events = new ArrayList<>(); + private Server _server; + private ServerConnector _connector; + private HttpClient _httpClient; + + public void start(HttpServlet servlet) throws Exception + { + start(contextHandler -> contextHandler.addServlet(servlet, "/")); + } + + public void start(Consumer configuration) throws Exception + { + _server = new Server(); + _connector = new ServerConnector(_server); + _server.addConnector(_connector); + ServletContextHandler contextHandler = new ServletContextHandler(); + contextHandler.addEventListener(new TestRequestListener()); + contextHandler.addFilter(new TestFilter(), "/*", EnumSet.allOf(DispatcherType.class)); + configuration.accept(contextHandler); + + _server.setHandler(contextHandler); + _server.start(); + + _httpClient = new HttpClient(); + _httpClient.start(); + } + + @AfterEach + public void after() throws Exception + { + _httpClient.stop(); + _server.stop(); + } + + @Test + public void testForward() throws Exception + { + start(new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + String pathInContext = URIUtil.addPaths(req.getContextPath(), req.getServletPath()); + _events.add(req.getDispatcherType() + " " + pathInContext); + if (req.getDispatcherType() == DispatcherType.REQUEST) + { + req.getRequestDispatcher(pathInContext).forward(req, resp); + return; + } + + resp.getWriter().print("success"); + } + }); + + ContentResponse response = _httpClient.GET("http://localhost:" + _connector.getLocalPort()); + assertThat(response.getStatus(), equalTo(HttpStatus.OK_200)); + assertThat(response.getContentAsString(), equalTo("success")); + + assertEvents("requestInitialized /", "doFilter /", "REQUEST /", "doFilter /", "FORWARD /", "requestDestroyed /"); + } + + @Test + public void testInclude() throws Exception + { + start(new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + String pathInContext = URIUtil.addPaths(req.getContextPath(), req.getServletPath()); + _events.add(req.getDispatcherType() + " " + pathInContext); + if (req.getDispatcherType() == DispatcherType.REQUEST) + { + req.getRequestDispatcher(pathInContext).include(req, resp); + return; + } + + resp.getWriter().print("success"); + } + }); + + ContentResponse response = _httpClient.GET("http://localhost:" + _connector.getLocalPort()); + assertThat(response.getStatus(), equalTo(HttpStatus.OK_200)); + assertThat(response.getContentAsString(), equalTo("success")); + + assertEvents("requestInitialized /", "doFilter /", "REQUEST /", "doFilter /", "INCLUDE /", "requestDestroyed /"); + } + + @Test + public void testAsync() throws Exception + { + start(new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + String pathInContext = URIUtil.addPaths(req.getContextPath(), req.getServletPath()); + _events.add(req.getDispatcherType() + " " + pathInContext); + if (req.getDispatcherType() == DispatcherType.REQUEST) + { + AsyncContext asyncContext = req.startAsync(); + asyncContext.dispatch(); + return; + } + + if (req.isAsyncStarted()) + req.getAsyncContext().complete(); + resp.getWriter().print("success"); + } + }); + + ContentResponse response = _httpClient.GET("http://localhost:" + _connector.getLocalPort()); + assertThat(response.getStatus(), equalTo(HttpStatus.OK_200)); + assertThat(response.getContentAsString(), equalTo("success")); + + assertEvents("requestInitialized /", "doFilter /", "REQUEST /", "requestDestroyed /", + "requestInitialized /", "doFilter /", "ASYNC /", "requestDestroyed /"); + } + + + @Test + public void testError() throws Exception + { + start(contextHandler -> + { + ErrorPageErrorHandler errorHandler = new ErrorPageErrorHandler(); + errorHandler.addErrorPage(500, "/error"); + contextHandler.setErrorHandler(errorHandler); + + contextHandler.addServlet(new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException + { + String pathInContext = URIUtil.addPaths(req.getContextPath(), req.getServletPath()); + _events.add(req.getDispatcherType() + " " + pathInContext); + if (req.getDispatcherType() == DispatcherType.REQUEST) + { + resp.sendError(500); + return; + } + + resp.setStatus(500); + resp.getWriter().print("error handled"); + } + }, "/"); + }); + + ContentResponse response = _httpClient.GET("http://localhost:" + _connector.getLocalPort()); + assertThat(response.getStatus(), equalTo(HttpStatus.INTERNAL_SERVER_ERROR_500)); + assertThat(response.getContentAsString(), equalTo("error handled")); + + assertEvents("requestInitialized /", "doFilter /", "REQUEST /", "requestDestroyed /", + "requestInitialized /", "doFilter /error", "ERROR /error", "requestDestroyed /"); + } + + @Test + public void testErrorNonDispatched() throws Exception + { + start(contextHandler -> + { + contextHandler.setErrorHandler((request, response, callback) -> + { + response.setStatus(500); + response.write(true, BufferUtil.toBuffer("error handled"), callback); + _events.add("errorHandler"); + return true; + }); + + contextHandler.addServlet(new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException + { + String pathInContext = URIUtil.addPaths(req.getContextPath(), req.getServletPath()); + _events.add(req.getDispatcherType() + " " + pathInContext); + if (req.getDispatcherType() == DispatcherType.REQUEST) + { + resp.sendError(500); + return; + } + + throw new IllegalStateException("should not reach here"); + } + }, "/"); + }); + + ContentResponse response = _httpClient.GET("http://localhost:" + _connector.getLocalPort()); + assertThat(response.getStatus(), equalTo(HttpStatus.INTERNAL_SERVER_ERROR_500)); + assertThat(response.getContentAsString(), equalTo("error handled")); + + assertEvents("requestInitialized /", "doFilter /", "REQUEST /", "requestDestroyed /", "errorHandler"); + } + + public class TestRequestListener implements ServletRequestListener + { + @Override + public void requestInitialized(ServletRequestEvent sre) + { + HttpServletRequest req = (HttpServletRequest)sre.getServletRequest(); + String pathInContext = URIUtil.addPaths(req.getContextPath(), req.getServletPath()); + _events.add("requestInitialized " + pathInContext); + } + + @Override + public void requestDestroyed(ServletRequestEvent sre) + { + HttpServletRequest req = (HttpServletRequest)sre.getServletRequest(); + String pathInContext = URIUtil.addPaths(req.getContextPath(), req.getServletPath()); + _events.add("requestDestroyed " + pathInContext); + } + } + + public class TestFilter implements Filter + { + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException + { + HttpServletRequest req = (HttpServletRequest)request; + String pathInContext = URIUtil.addPaths(req.getContextPath(), req.getServletPath()); + _events.add("doFilter " + pathInContext); + chain.doFilter(request, response); + } + } + + private void assertEvents(String... events) + { + assertThat(_events, equalTo(Arrays.asList(events))); + } +} From ab4c3dfb08d59a50bc6aaeeab8c2fd6a1e2bd49f Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 26 Apr 2023 12:11:55 +1000 Subject: [PATCH 2/5] fix checkstyle errors Signed-off-by: Lachlan Roberts --- .../ee10/servlet/ServletRequestListenerTest.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java index 2212a5fd18c7..f31080280c05 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java @@ -1,3 +1,16 @@ +// +// ======================================================================== +// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others. +// +// 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. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + package org.eclipse.jetty.ee10.servlet; import java.io.IOException; @@ -153,7 +166,6 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws Se "requestInitialized /", "doFilter /", "ASYNC /", "requestDestroyed /"); } - @Test public void testError() throws Exception { From 22bdc6a6c31092530a6c0a711f90bc44dc8f3763 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 27 Apr 2023 15:51:38 +1000 Subject: [PATCH 3/5] Issue #9637 - add test for 403 by securityHandler Signed-off-by: Lachlan Roberts --- .../servlet/ServletRequestListenerTest.java | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java index f31080280c05..f239bf224538 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletRequestListenerTest.java @@ -34,11 +34,14 @@ import jakarta.servlet.http.HttpServletResponse; import org.eclipse.jetty.client.ContentResponse; import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.ee10.servlet.security.ConstraintMapping; +import org.eclipse.jetty.ee10.servlet.security.ConstraintSecurityHandler; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.URIUtil; +import org.eclipse.jetty.util.security.Constraint; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; @@ -240,6 +243,43 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IO assertEvents("requestInitialized /", "doFilter /", "REQUEST /", "requestDestroyed /", "errorHandler"); } + @Test + public void testSecurityHandlerRejectedRequest() throws Exception + { + start(contextHandler -> + { + ConstraintSecurityHandler securityHandler = new ConstraintSecurityHandler(); + securityHandler.addRole("admin"); + ConstraintMapping constraintMapping = new ConstraintMapping(); + constraintMapping.setPathSpec("/authed"); + Constraint constraint = new Constraint("admin", "admin"); + constraint.setAuthenticate(true); + constraintMapping.setConstraint(constraint); + securityHandler.addConstraintMapping(constraintMapping); + contextHandler.setSecurityHandler(securityHandler); + contextHandler.addServlet(new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException + { + String pathInContext = URIUtil.addPaths(req.getContextPath(), req.getServletPath()); + _events.add(req.getDispatcherType() + " " + pathInContext); + resp.setStatus(200); + resp.getWriter().print("from servlet"); + } + }, "/"); + }); + + ContentResponse response = _httpClient.GET("http://localhost:" + _connector.getLocalPort()); + assertThat(response.getStatus(), equalTo(HttpStatus.OK_200)); + assertThat(response.getContentAsString(), equalTo("from servlet")); + assertEvents("requestInitialized /", "doFilter /", "REQUEST /", "requestDestroyed /"); + + response = _httpClient.GET("http://localhost:" + _connector.getLocalPort() + "/authed"); + assertThat(response.getStatus(), equalTo(HttpStatus.FORBIDDEN_403)); + assertEventsEmpty(); + } + public class TestRequestListener implements ServletRequestListener { @Override @@ -271,8 +311,15 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha } } + private void assertEventsEmpty() + { + assertThat(_events.size(), equalTo(0)); + _events.clear(); + } + private void assertEvents(String... events) { assertThat(_events, equalTo(Arrays.asList(events))); + _events.clear(); } } From 082b36df96ca20ce30accd5a245ea2eda0efaea2 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 28 Apr 2023 00:08:11 +1000 Subject: [PATCH 4/5] Issue #9637 - add boolean argument to the ServletChannel.dispatch method Signed-off-by: Lachlan Roberts --- .../jetty/ee10/servlet/ServletChannel.java | 36 ++++++------------- 1 file changed, 10 insertions(+), 26 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 60edfb231afc..cda41ec8e618 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -423,7 +423,7 @@ public boolean handle() ServletHandler.MappedServlet mappedServlet = _servletContextRequest._mappedServlet; mappedServlet.handle(servletHandler, Request.getPathInContext(_servletContextRequest), _servletContextRequest.getHttpServletRequest(), _servletContextRequest.getHttpServletResponse()); - }); + }, true); break; } @@ -465,7 +465,7 @@ public boolean handle() Dispatcher dispatcher = new Dispatcher(getContextHandler(), uri, decodedPathInContext); dispatcher.async(asyncContextEvent.getSuppliedRequest(), asyncContextEvent.getSuppliedResponse()); - }); + }, true); break; } @@ -510,7 +510,9 @@ public boolean handle() // _state.completing(); try (Blocker.Callback blocker = Blocker.callback()) { - errorDispatch(() -> errorHandler.handle(_servletContextRequest, getResponse(), blocker)); + // We do not notify ServletRequestListener on this dispatch because it might not + // be dispatched to an error page, so we delegate this responsibility to the ErrorHandler. + dispatch(() -> errorHandler.handle(_servletContextRequest, getResponse(), blocker), false); blocker.block(); } } @@ -648,33 +650,13 @@ public boolean sendErrorOrAbort(String message) return false; } - private void dispatch(Dispatchable dispatchable) throws Exception - { - try - { - _servletContextRequest.getResponse().getHttpOutput().reopen(); - _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); - getHttpOutput().reopen(); - _combinedListener.onBeforeDispatch(_servletContextRequest); - dispatchable.dispatch(); - } - catch (Throwable x) - { - _combinedListener.onDispatchFailure(_servletContextRequest, x); - throw x; - } - finally - { - _combinedListener.onAfterDispatch(_servletContextRequest); - _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); - } - } - - private void errorDispatch(Dispatchable dispatchable) throws Exception + private void dispatch(Dispatchable dispatchable, boolean notifyRequestListener) throws Exception { try { _servletContextRequest.getResponse().getHttpOutput().reopen(); + if (notifyRequestListener) + _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); getHttpOutput().reopen(); _combinedListener.onBeforeDispatch(_servletContextRequest); dispatchable.dispatch(); @@ -687,6 +669,8 @@ private void errorDispatch(Dispatchable dispatchable) throws Exception finally { _combinedListener.onAfterDispatch(_servletContextRequest); + if (notifyRequestListener) + _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); } } From ee047f1d913e24b0cbee84e23aa1738e385935e9 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 2 May 2023 10:00:49 +1000 Subject: [PATCH 5/5] Issue #9637 - changes from review Signed-off-by: Lachlan Roberts --- .../jetty/ee10/servlet/ServletChannel.java | 88 +++++++++++-------- 1 file changed, 51 insertions(+), 37 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index cda41ec8e618..1124c9017102 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -419,11 +419,20 @@ public boolean handle() { dispatch(() -> { - ServletHandler servletHandler = _context.getServletContextHandler().getServletHandler(); - ServletHandler.MappedServlet mappedServlet = _servletContextRequest._mappedServlet; + try + { + _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); - mappedServlet.handle(servletHandler, Request.getPathInContext(_servletContextRequest), _servletContextRequest.getHttpServletRequest(), _servletContextRequest.getHttpServletResponse()); - }, true); + ServletHandler servletHandler = _context.getServletContextHandler().getServletHandler(); + ServletHandler.MappedServlet mappedServlet = _servletContextRequest._mappedServlet; + + mappedServlet.handle(servletHandler, Request.getPathInContext(_servletContextRequest), _servletContextRequest.getHttpServletRequest(), _servletContextRequest.getHttpServletResponse()); + } + finally + { + _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); + } + }); break; } @@ -432,40 +441,49 @@ public boolean handle() { dispatch(() -> { - HttpURI uri; - String pathInContext; - AsyncContextEvent asyncContextEvent = _state.getAsyncContextEvent(); - String dispatchString = asyncContextEvent.getDispatchPath(); - if (dispatchString != null) + try { - String contextPath = _context.getContextPath(); - HttpURI.Immutable dispatchUri = HttpURI.from(dispatchString); - pathInContext = URIUtil.canonicalPath(dispatchUri.getPath()); - uri = HttpURI.build(_servletContextRequest.getHttpURI()) - .path(URIUtil.addPaths(contextPath, pathInContext)) - .query(dispatchUri.getQuery()); - } - else - { - uri = asyncContextEvent.getBaseURI(); - if (uri == null) + _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); + + HttpURI uri; + String pathInContext; + AsyncContextEvent asyncContextEvent = _state.getAsyncContextEvent(); + String dispatchString = asyncContextEvent.getDispatchPath(); + if (dispatchString != null) { - uri = _servletContextRequest.getHttpURI(); - pathInContext = Request.getPathInContext(_servletContextRequest); + String contextPath = _context.getContextPath(); + HttpURI.Immutable dispatchUri = HttpURI.from(dispatchString); + pathInContext = URIUtil.canonicalPath(dispatchUri.getPath()); + uri = HttpURI.build(_servletContextRequest.getHttpURI()) + .path(URIUtil.addPaths(contextPath, pathInContext)) + .query(dispatchUri.getQuery()); } else { - pathInContext = uri.getCanonicalPath(); - if (_context.getContextPath().length() > 1) - pathInContext = pathInContext.substring(_context.getContextPath().length()); + uri = asyncContextEvent.getBaseURI(); + if (uri == null) + { + uri = _servletContextRequest.getHttpURI(); + pathInContext = Request.getPathInContext(_servletContextRequest); + } + else + { + pathInContext = uri.getCanonicalPath(); + if (_context.getContextPath().length() > 1) + pathInContext = pathInContext.substring(_context.getContextPath().length()); + } } - } - // We first worked with the core pathInContext above, but now need to convert to servlet style - String decodedPathInContext = URIUtil.decodePath(pathInContext); + // We first worked with the core pathInContext above, but now need to convert to servlet style + String decodedPathInContext = URIUtil.decodePath(pathInContext); - Dispatcher dispatcher = new Dispatcher(getContextHandler(), uri, decodedPathInContext); - dispatcher.async(asyncContextEvent.getSuppliedRequest(), asyncContextEvent.getSuppliedResponse()); - }, true); + Dispatcher dispatcher = new Dispatcher(getContextHandler(), uri, decodedPathInContext); + dispatcher.async(asyncContextEvent.getSuppliedRequest(), asyncContextEvent.getSuppliedResponse()); + } + finally + { + _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); + } + }); break; } @@ -512,7 +530,7 @@ public boolean handle() { // We do not notify ServletRequestListener on this dispatch because it might not // be dispatched to an error page, so we delegate this responsibility to the ErrorHandler. - dispatch(() -> errorHandler.handle(_servletContextRequest, getResponse(), blocker), false); + dispatch(() -> errorHandler.handle(_servletContextRequest, getResponse(), blocker)); blocker.block(); } } @@ -650,13 +668,11 @@ public boolean sendErrorOrAbort(String message) return false; } - private void dispatch(Dispatchable dispatchable, boolean notifyRequestListener) throws Exception + private void dispatch(Dispatchable dispatchable) throws Exception { try { _servletContextRequest.getResponse().getHttpOutput().reopen(); - if (notifyRequestListener) - _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); getHttpOutput().reopen(); _combinedListener.onBeforeDispatch(_servletContextRequest); dispatchable.dispatch(); @@ -669,8 +685,6 @@ private void dispatch(Dispatchable dispatchable, boolean notifyRequestListener) finally { _combinedListener.onAfterDispatch(_servletContextRequest); - if (notifyRequestListener) - _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); } }