From aa07995f373a82441373d2c978f85209461e8eeb Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 28 Aug 2024 20:24:10 +1000 Subject: [PATCH] sendError(-1) is an abort (#12206) restore behaviour from jetty <= 11 where a sendError(-1) is a true abort, without an attempt to send an error response. --- .../org/eclipse/jetty/server/Request.java | 30 +++ .../server/internal/HttpChannelState.java | 18 +- .../ee10/servlet/ServletApiResponse.java | 2 +- .../jetty/ee10/servlet/ErrorPageTest.java | 220 ++++++++++++++++++ .../eclipse/jetty/ee9/nested/Response.java | 2 +- .../jetty/ee9/servlet/ErrorPageTest.java | 46 +++- 6 files changed, 305 insertions(+), 13 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index d1ad6b93ca3..84fbc3cd238 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -47,6 +47,7 @@ import org.eclipse.jetty.http.MultiPartCompliance; import org.eclipse.jetty.http.MultiPartConfig; import org.eclipse.jetty.http.Trailers; import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.server.internal.CompletionStreamWrapper; import org.eclipse.jetty.server.internal.HttpChannelState; import org.eclipse.jetty.util.Attributes; @@ -716,6 +717,7 @@ public interface Request extends Attributes, Content.Source * is returned, then this method must not generate a response, nor complete the callback. * @throws Exception if there is a failure during the handling. Catchers cannot assume that the callback will be * called and thus should attempt to complete the request as if a false had been returned. + * @see AbortException */ boolean handle(Request request, Response response, Callback callback) throws Exception; @@ -725,6 +727,34 @@ public interface Request extends Attributes, Content.Source { return InvocationType.BLOCKING; } + + /** + * A marker {@link Exception} that can be passed the {@link Callback#failed(Throwable)} of the {@link Callback} + * passed in {@link #handle(Request, Response, Callback)}, to cause request handling to be aborted. For HTTP/1 + * an abort is handled with a {@link EndPoint#close()}, for later versions of HTTP, a reset message will be sent. + */ + class AbortException extends Exception + { + public AbortException() + { + super(); + } + + public AbortException(String message) + { + super(message); + } + + public AbortException(String message, Throwable cause) + { + super(message, cause); + } + + public AbortException(Throwable cause) + { + super(cause); + } + } } /** diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index f3455d38095..d2068f6381e 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -1590,18 +1590,18 @@ public class HttpChannelState implements HttpChannel, Components httpChannelState._callbackFailure = failure; - // Consume any input. - Throwable unconsumed = stream.consumeAvailable(); - ExceptionUtil.addSuppressedIfNotAssociated(failure, unconsumed); + if (!stream.isCommitted() && !(failure instanceof Request.Handler.AbortException)) + { + // Consume any input. + Throwable unconsumed = stream.consumeAvailable(); + ExceptionUtil.addSuppressedIfNotAssociated(failure, unconsumed); - ChannelResponse response = httpChannelState._response; - if (LOG.isDebugEnabled()) - LOG.debug("failed stream.isCommitted={}, response.isCommitted={} {}", stream.isCommitted(), response.isCommitted(), this); + ChannelResponse response = httpChannelState._response; + if (LOG.isDebugEnabled()) + LOG.debug("failed stream.isCommitted={}, response.isCommitted={} {}", stream.isCommitted(), response.isCommitted(), this); - // There may have been an attempt to write an error response that failed. - // Do not try to write again an error response if already committed. - if (!stream.isCommitted()) errorResponse = new ErrorResponse(request); + } } if (errorResponse != null) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiResponse.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiResponse.java index 70a145bc116..45bf9b1252c 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiResponse.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiResponse.java @@ -138,7 +138,7 @@ public class ServletApiResponse implements HttpServletResponse { switch (sc) { - case -1 -> getServletChannel().abort(new IOException(msg)); + case -1 -> getServletChannel().abort(new Request.Handler.AbortException(msg)); case HttpStatus.PROCESSING_102, HttpStatus.EARLY_HINTS_103 -> { if (!isCommitted()) diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ErrorPageTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ErrorPageTest.java index c1b3d8234d8..3667b60f8dd 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ErrorPageTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ErrorPageTest.java @@ -13,8 +13,12 @@ package org.eclipse.jetty.ee10.servlet; +import java.io.BufferedReader; import java.io.IOException; +import java.io.InputStreamReader; +import java.io.OutputStream; import java.io.PrintWriter; +import java.net.Socket; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; @@ -50,6 +54,7 @@ import org.eclipse.jetty.server.LocalConnector; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Response; import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.toolchain.test.jupiter.WorkDir; import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension; import org.eclipse.jetty.util.Callback; @@ -70,6 +75,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -780,6 +786,220 @@ public class ErrorPageTest assertThat(responseBody, Matchers.containsString("ERROR_REQUEST_URI: /fail/599")); } + @Test + public void testAbortWithSendError() throws Exception + { + ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.NO_SECURITY | ServletContextHandler.NO_SESSIONS); + contextHandler.setContextPath("/"); + + HttpServlet failServlet = new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException + { + response.sendError(-1); + } + }; + + contextHandler.addServlet(failServlet, "/abort"); + startServer(contextHandler); + + ServerConnector connector = new ServerConnector(_server); + connector.setPort(0); + _server.addConnector(connector); + connector.start(); + try (Socket socket = new Socket("localhost", connector.getLocalPort())) + { + OutputStream output = socket.getOutputStream(); + + String request = """ + GET /abort HTTP/1.1\r + Host: test\r + \r + """; + output.write(request.getBytes(StandardCharsets.UTF_8)); + output.flush(); + + BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); + String line = in.readLine(); + assertNull(line); + } + } + + @Test + public void testAbortWithSendErrorChunked() throws Exception + { + ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.NO_SECURITY | ServletContextHandler.NO_SESSIONS); + contextHandler.setContextPath("/"); + + HttpServlet failServlet = new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException + { + response.getOutputStream().write("test".getBytes(StandardCharsets.UTF_8)); + response.flushBuffer(); + response.sendError(-1); + } + }; + + contextHandler.addServlet(failServlet, "/abort"); + startServer(contextHandler); + + ServerConnector connector = new ServerConnector(_server); + connector.setPort(0); + _server.addConnector(connector); + connector.start(); + try (Socket socket = new Socket("localhost", connector.getLocalPort())) + { + OutputStream output = socket.getOutputStream(); + + String request = """ + GET /abort HTTP/1.1\r + Host: test\r + \r + """; + output.write(request.getBytes(StandardCharsets.UTF_8)); + output.flush(); + + BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); + String line = in.readLine(); + assertThat(line, is("HTTP/1.1 200 OK")); + + boolean chunked = false; + while (!line.isEmpty()) + { + line = in.readLine(); + assertNotNull(line); + chunked |= line.equals("Transfer-Encoding: chunked"); + } + assertTrue(chunked); + + line = in.readLine(); + assertThat(line, is("4")); + line = in.readLine(); + assertThat(line, is("test")); + + line = in.readLine(); + assertNull(line); + } + } + + @Test + public void testAbortWithSendErrorContent() throws Exception + { + ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.NO_SECURITY | ServletContextHandler.NO_SESSIONS); + contextHandler.setContextPath("/"); + + HttpServlet failServlet = new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException + { + response.setContentLength(10); + response.getOutputStream().write("test\r\n".getBytes(StandardCharsets.UTF_8)); + response.flushBuffer(); + response.sendError(-1); + } + }; + + contextHandler.addServlet(failServlet, "/abort"); + startServer(contextHandler); + + ServerConnector connector = new ServerConnector(_server); + connector.setPort(0); + _server.addConnector(connector); + connector.start(); + try (Socket socket = new Socket("localhost", connector.getLocalPort())) + { + OutputStream output = socket.getOutputStream(); + + String request = """ + GET /abort HTTP/1.1\r + Host: test\r + \r + """; + output.write(request.getBytes(StandardCharsets.UTF_8)); + output.flush(); + + BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); + String line = in.readLine(); + assertThat(line, is("HTTP/1.1 200 OK")); + + boolean chunked = false; + while (!line.isEmpty()) + { + line = in.readLine(); + assertNotNull(line); + chunked |= line.equals("Transfer-Encoding: chunked"); + } + assertFalse(chunked); + + line = in.readLine(); + assertThat(line, is("test")); + + line = in.readLine(); + assertNull(line); + } + } + + @Test + public void testAbortWithSendErrorComplete() throws Exception + { + ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.NO_SECURITY | ServletContextHandler.NO_SESSIONS); + contextHandler.setContextPath("/"); + + HttpServlet failServlet = new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException + { + response.setContentLength(6); + response.getOutputStream().write("test\r\n".getBytes(StandardCharsets.UTF_8)); + response.sendError(-1); + } + }; + + contextHandler.addServlet(failServlet, "/abort"); + startServer(contextHandler); + + ServerConnector connector = new ServerConnector(_server); + connector.setPort(0); + _server.addConnector(connector); + connector.start(); + try (Socket socket = new Socket("localhost", connector.getLocalPort())) + { + OutputStream output = socket.getOutputStream(); + + String request = """ + GET /abort HTTP/1.1\r + Host: test\r + \r + """; + output.write(request.getBytes(StandardCharsets.UTF_8)); + output.flush(); + + BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); + String line = in.readLine(); + assertThat(line, is("HTTP/1.1 200 OK")); + + boolean chunked = false; + while (!line.isEmpty()) + { + line = in.readLine(); + assertNotNull(line); + chunked |= line.equals("Transfer-Encoding: chunked"); + } + assertFalse(chunked); + + line = in.readLine(); + assertThat(line, is("test")); + + line = in.readLine(); + assertNull(line); + } + } + @Test public void testErrorCodeNoDefaultServletNonExistentErrorLocation() throws Exception { diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Response.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Response.java index 10e33baa484..37f94674459 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Response.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Response.java @@ -488,7 +488,7 @@ public class Response implements HttpServletResponse switch (code) { - case -1 -> _channel.abort(new IOException(message)); + case -1 -> _channel.abort(new org.eclipse.jetty.server.Request.Handler.AbortException(message)); case HttpStatus.PROCESSING_102 -> sendProcessing(); case HttpStatus.EARLY_HINTS_103 -> sendEarlyHint(); default -> _channel.getState().sendError(code, message); diff --git a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ErrorPageTest.java b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ErrorPageTest.java index 02b5682aa55..bbed1f62278 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ErrorPageTest.java +++ b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ErrorPageTest.java @@ -13,8 +13,12 @@ package org.eclipse.jetty.ee9.servlet; +import java.io.BufferedReader; import java.io.IOException; +import java.io.InputStreamReader; +import java.io.OutputStream; import java.io.PrintWriter; +import java.net.Socket; import java.nio.charset.StandardCharsets; import java.util.EnumSet; import java.util.concurrent.ConcurrentHashMap; @@ -29,7 +33,6 @@ import jakarta.servlet.DispatcherType; import jakarta.servlet.Filter; import jakarta.servlet.FilterChain; import jakarta.servlet.FilterConfig; -import jakarta.servlet.RequestDispatcher; import jakarta.servlet.Servlet; import jakarta.servlet.ServletException; import jakarta.servlet.ServletRequest; @@ -49,10 +52,10 @@ import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.LocalConnector; import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; import org.hamcrest.Matchers; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -62,6 +65,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; //@Disabled // TODO @@ -105,6 +109,7 @@ public class ErrorPageTest _context.addServlet(ErrorAndStatusServlet.class, "/error-and-status/*"); _context.addServlet(ErrorContentTypeCharsetWriterInitializedServlet.class, "/error-mime-charset-writer/*"); _context.addServlet(ExceptionServlet.class, "/exception-servlet"); + _context.addServlet(AbortServlet.class, "/abort"); HandlerWrapper noopHandler = new HandlerWrapper() { @@ -300,6 +305,34 @@ public class ErrorPageTest assertThat(response, Matchers.containsString("ERROR_REQUEST_URI: /fail/code")); } + @Test + public void testAbortWithSendError() throws Exception + { + ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.NO_SECURITY | ServletContextHandler.NO_SESSIONS); + contextHandler.setContextPath("/"); + + ServerConnector connector = new ServerConnector(_server); + connector.setPort(0); + _server.addConnector(connector); + connector.start(); + try (Socket socket = new Socket("localhost", connector.getLocalPort())) + { + OutputStream output = socket.getOutputStream(); + + String request = """ + GET /abort HTTP/1.1\r + Host: test\r + \r + """; + output.write(request.getBytes(StandardCharsets.UTF_8)); + output.flush(); + + BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); + String line = in.readLine(); + assertNull(line); + } + } + @Test public void testErrorException() throws Exception { @@ -871,4 +904,13 @@ public class ErrorPageTest super(rootCause); } } + + public static class AbortServlet extends HttpServlet + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse response) throws ServletException, IOException + { + response.sendError(-1); + } + } }