Use async Servlet API to send error response

author: Bjørn Christian Seime <bjorncs@yahoo-inc.com> 2016-11-29 14:13:26 +0100
committer: Bjørn Christian Seime <bjorncs@yahoo-inc.com> 2016-11-29 16:54:00 +0100
commit: 95baadd7399ab20c1ca31c1285f6802fe8c48f62 (patch)
tree: 5c9176e955f8cb7a194aa7ab4400e13a0559f7f8 /jdisc_http_service
parent: d89285bd7b540770d2fa5fe473234c134ed825a5 (diff)
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);
+    }
+
+}
author	Bjørn Christian Seime <bjorncs@yahoo-inc.com>	2016-11-29 14:13:26 +0100
committer	Bjørn Christian Seime <bjorncs@yahoo-inc.com>	2016-11-29 16:54:00 +0100
commit	95baadd7399ab20c1ca31c1285f6802fe8c48f62 (patch)
tree	5c9176e955f8cb7a194aa7ab4400e13a0559f7f8 /jdisc_http_service
parent	d89285bd7b540770d2fa5fe473234c134ed825a5 (diff)