diff options
Diffstat (limited to 'jdisc_http_service')
6 files changed, 133 insertions, 45 deletions
diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ErrorResponseContentCreator.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ErrorResponseContentCreator.java new file mode 100644 index 00000000000..1250745e356 --- /dev/null +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ErrorResponseContentCreator.java @@ -0,0 +1,40 @@ +package com.yahoo.jdisc.http.server.jetty; + +import org.eclipse.jetty.util.ByteArrayISO8859Writer; +import org.eclipse.jetty.util.StringUtil; + +import java.io.IOException; +import java.util.Optional; + +/** + * Creates HTML body having the status code, error message and request uri. + * The body is constructed from a template that is inspired by the default Jetty template (see {@link org.eclipse.jetty.server.Response#sendError(int, String)}). + * The content is written using the ISO-8859-1 charset. + * + * @author bjorncs + */ +public class ErrorResponseContentCreator { + + private final ByteArrayISO8859Writer writer = new ByteArrayISO8859Writer(2048); + + public byte[] createErrorContent(String requestUri, int statusCode, Optional<String> message) { + String sanitizedString = message.map(StringUtil::sanitizeXmlString).orElse(""); + String statusCodeString = Integer.toString(statusCode); + writer.resetWriter(); + try { + writer.write("<html>\n<head>\n<meta http-equiv=\"Content-Type\" content=\"text/html;charset=ISO-8859-1\"/>\n<title>Error "); + writer.write(statusCodeString); + writer.write("</title>\n</head>\n<body>\n<h2>HTTP ERROR: "); + writer.write(statusCodeString); + writer.write("</h2>\n<p>Problem accessing "); + writer.write(StringUtil.sanitizeXmlString(requestUri)); + writer.write(". Reason:\n<pre> "); + writer.write(sanitizedString); + writer.write("</pre></p>\n<hr/>\n</body>\n</html>\n"); + } catch (IOException e) { + // IOException should not be thrown unless writer is constructed using byte[] parameter + throw new RuntimeException(e); + } + return writer.getByteArray(); + } +} diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java index 295d3e9f4d4..b42ed90569e 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java @@ -62,6 +62,7 @@ class HttpRequestDispatch { this.servletRequest = servletRequest; honourMaxKeepAliveRequests(); this.servletResponseController = new ServletResponseController( + servletRequest, servletResponse, jDiscContext.janitor, metricReporter, diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletOutputStreamWriter.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletOutputStreamWriter.java index c771fd55f1c..0cb44e445d2 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletOutputStreamWriter.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletOutputStreamWriter.java @@ -20,6 +20,7 @@ import java.util.logging.Logger; /** * @author tonytv + * @author bjorncs */ public class ServletOutputStreamWriter { /** Rules: @@ -75,11 +76,12 @@ public class ServletOutputStreamWriter { this.metricReporter = metricReporter; } - public void setSendingError() { + public void sendErrorContentAndCloseAsync(ByteBuffer errorContent) { synchronized (monitor) { // Assert that no content has been written as it is too late to write error response if the response is committed. assertStateIs(state, State.NOT_STARTED); - state = State.FINISHED_OR_ERROR; + writeBuffer(errorContent, null); + close(null); } } diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java index 835b44a4e69..afb16659b9e 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletRequestReader.java @@ -199,11 +199,6 @@ class ServletRequestReader implements ReadListener { state = State.REQUEST_CONTENT_CLOSED; } } - // Early complete if response is already committed. No point of waiting for any exceptions from request handler - // if we cannot write back an error anyway - if (responseController.isResponseCommitted()) { - finishedFuture.complete(null); - } if (shouldCloseRequestContentChannel) { closeCompletionHandler_noThrow(); diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java index 511451b3577..7f3af1258b6 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ServletResponseController.java @@ -6,10 +6,13 @@ import com.yahoo.jdisc.handler.BindingNotFoundException; import com.yahoo.jdisc.handler.CompletionHandler; import com.yahoo.jdisc.handler.ContentChannel; import com.yahoo.jdisc.handler.ResponseHandler; +import com.yahoo.jdisc.http.HttpHeaders; import com.yahoo.jdisc.http.HttpResponse; import com.yahoo.jdisc.service.BindingSetNotFoundException; +import org.eclipse.jetty.http.MimeTypes; import javax.annotation.concurrent.GuardedBy; +import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.io.PrintWriter; @@ -24,8 +27,10 @@ import java.util.logging.Logger; /** * @author tonytv + * @author bjorncs */ public class ServletResponseController { + private static Logger log = Logger.getLogger(ServletResponseController.class.getName()); /** @@ -37,9 +42,10 @@ public class ServletResponseController { private final Object monitor = new Object(); //servletResponse must not be modified after the response has been committed. + private final HttpServletRequest servletRequest; private final HttpServletResponse servletResponse; private final boolean developerMode; - private final Executor executor; + private final ErrorResponseContentCreator errorResponseContentCreator = new ErrorResponseContentCreator(); //all calls to the servletOutputStreamWriter must hold the monitor first to ensure visibility of servletResponse changes. private final ServletOutputStreamWriter servletOutputStreamWriter; @@ -47,17 +53,16 @@ public class ServletResponseController { @GuardedBy("monitor") private boolean responseCommitted = false; - - public ServletResponseController( + HttpServletRequest servletRequest, HttpServletResponse servletResponse, Executor executor, MetricReporter metricReporter, boolean developerMode) throws IOException { + this.servletRequest = servletRequest; this.servletResponse = servletResponse; this.developerMode = developerMode; - this.executor = executor; this.servletOutputStreamWriter = new ServletOutputStreamWriter(servletResponse.getOutputStream(), executor, metricReporter); } @@ -89,50 +94,60 @@ public class ServletResponseController { public void trySendError(Throwable t) { - final boolean responseWasCommitted; - - synchronized (monitor) { - responseWasCommitted = responseCommitted; + String reasonPhrase = getReasonPhrase(t, developerMode); + int statusCode = getStatusCode(t); - if (!responseCommitted) { - responseCommitted = true; - servletOutputStreamWriter.setSendingError(); + final boolean responseWasCommitted; + try { + synchronized (monitor) { + responseWasCommitted = responseCommitted; + if (!responseCommitted) { + responseCommitted = true; + sendErrorAsync(statusCode, reasonPhrase); + } } + } catch (Throwable e) { + servletOutputStreamWriter.fail(t); + return; } //Must be evaluated after state transition for test purposes(See ConformanceTestException) //Done outside the monitor since it causes a callback in tests. - String reasonPhrase = getReasonPhrase(t, developerMode); - int statusCode = getStatusCode(t); - if (responseWasCommitted) { - RuntimeException exceptionWithStackTrace = new RuntimeException(t); log.log(Level.FINE, "Response already committed, can't change response code", exceptionWithStackTrace); // TODO: should always have failed here, but that breaks test assumptions. Doing soft close instead. //assert !Thread.holdsLock(monitor); //servletOutputStreamWriter.fail(t); servletOutputStreamWriter.close(null); - return; } - try { - - // HttpServletResponse.sendError() is blocking and must not be executed in Jetty/RequestHandler thread. - executor.execute(() -> { - try { - // TODO We should control the response content this method generates - // a response body based on Jetty's own response templates ("Powered by Jetty"). - servletResponse.sendError(statusCode, reasonPhrase); - finishedFuture().complete(null); - } catch (IOException e) { - log.severe("Failed to send error response: " + e.getMessage()); - throw new RuntimeException(e); - } - }); + } - } catch (Throwable e) { - servletOutputStreamWriter.fail(t); + /** + * Async version of {@link org.eclipse.jetty.server.Response#sendError(int, String)}. + */ + private void sendErrorAsync(int statusCode, String reasonPhrase) { + servletResponse.setHeader(HttpHeaders.Names.EXPIRES, null); + servletResponse.setHeader(HttpHeaders.Names.LAST_MODIFIED, null); + servletResponse.setHeader(HttpHeaders.Names.CACHE_CONTROL, null); + servletResponse.setHeader(HttpHeaders.Names.CONTENT_TYPE, null); + servletResponse.setHeader(HttpHeaders.Names.CONTENT_LENGTH, null); + setStatus(servletResponse, statusCode, Optional.of(reasonPhrase)); + + // If we are allowed to have a body + if (statusCode != HttpServletResponse.SC_NO_CONTENT && + statusCode != HttpServletResponse.SC_NOT_MODIFIED && + statusCode != HttpServletResponse.SC_PARTIAL_CONTENT && + statusCode >= HttpServletResponse.SC_OK) { + servletResponse.setHeader(HttpHeaders.Names.CACHE_CONTROL, "must-revalidate,no-cache,no-store"); + servletResponse.setContentType(MimeTypes.Type.TEXT_HTML_8859_1.toString()); + byte[] errorContent = errorResponseContentCreator + .createErrorContent(servletRequest.getRequestURI(), statusCode, Optional.ofNullable(reasonPhrase)); + servletResponse.setContentLength(errorContent.length); + servletOutputStreamWriter.sendErrorContentAndCloseAsync(ByteBuffer.wrap(errorContent)); + } else { + servletOutputStreamWriter.close(null); } } @@ -207,12 +222,6 @@ public class ServletResponseController { } } - public boolean isResponseCommitted() { - synchronized (monitor) { - return responseCommitted; - } - } - public final ResponseHandler responseHandler = new ResponseHandler() { @Override public ContentChannel handleResponse(Response response) { diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ErrorResponseContentCreatorTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ErrorResponseContentCreatorTest.java new file mode 100644 index 00000000000..c0cb9b1acb4 --- /dev/null +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ErrorResponseContentCreatorTest.java @@ -0,0 +1,41 @@ +package com.yahoo.jdisc.http.server.jetty; + +import org.testng.annotations.Test; + +import javax.servlet.http.HttpServletResponse; +import java.nio.charset.StandardCharsets; +import java.util.Optional; + +import static org.testng.Assert.assertEquals; + +/** + * @author bjorncs + */ +public class ErrorResponseContentCreatorTest { + + @Test + public void response_content_matches_expected_string() { + String expectedHtml = + "<html>\n" + + "<head>\n" + + "<meta http-equiv=\"Content-Type\" content=\"text/html;charset=ISO-8859-1\"/>\n" + + "<title>Error 200</title>\n" + + "</head>\n" + + "<body>\n" + + "<h2>HTTP ERROR: 200</h2>\n" + + "<p>Problem accessing http://foo.bar. Reason:\n" + + "<pre> My custom error message</pre></p>\n" + + "<hr/>\n" + + "</body>\n" + + "</html>\n"; + + ErrorResponseContentCreator c = new ErrorResponseContentCreator(); + byte[] rawContent = c.createErrorContent( + "http://foo.bar", + HttpServletResponse.SC_OK, + Optional.of("My custom error message")); + String actualHtml = new String(rawContent, StandardCharsets.ISO_8859_1); + assertEquals(expectedHtml, actualHtml); + } + +} |