Skip to content

Commit

Permalink
sendError(-1) is an abort (#12206)
Browse files Browse the repository at this point in the history
restore behaviour from jetty <= 11 where a sendError(-1) is a true abort, without an attempt to send an error response.
  • Loading branch information
gregw committed Aug 28, 2024
1 parent edbd21b commit aa07995
Show file tree
Hide file tree
Showing 6 changed files with 305 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
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;
Expand Down Expand Up @@ -716,6 +717,7 @@ interface Handler extends Invocable
* 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;

Expand All @@ -725,6 +727,34 @@ default InvocationType getInvocationType()
{
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);
}
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1590,18 +1590,18 @@ public void failed(Throwable failure)

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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public void sendError(int sc, String msg) throws IOException
{
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())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -50,6 +54,7 @@
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;
Expand All @@ -70,6 +75,7 @@
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;
Expand Down Expand Up @@ -780,6 +786,220 @@ protected void doGet(HttpServletRequest req, HttpServletResponse response) throw
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
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ public void sendError(int code, String message) throws IOException

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);
Expand Down
Loading

0 comments on commit aa07995

Please sign in to comment.