chore(implementation): Use Jetty handlers rather than Servlets

Optimization to avoid the extra dispatch overheads of Servlets by using Eclipse Jetty Handlers directly.

Author: gregw

invoker/core/pom.xml

@@ -46,11 +46,6 @@
       <artifactId>functions-framework-api</artifactId>
       <version>1.1.0</version>
     </dependency>
-    <dependency>
-      <groupId>javax.servlet</groupId>
-      <artifactId>javax.servlet-api</artifactId>
-      <version>4.0.1</version>
-    </dependency>
     <dependency>
       <groupId>io.cloudevents</groupId>
       <artifactId>cloudevents-core</artifactId>

invoker/core/src/main/java/com/google/cloud/functions/invoker/BackgroundFunctionExecutor.java

@@ -42,12 +42,14 @@
 import java.util.TreeMap;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import javax.servlet.http.HttpServlet;
+import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+import org.eclipse.jetty.server.Request;
+import org.eclipse.jetty.server.handler.AbstractHandler;
 
 /** Executes the user's background function. */
-public final class BackgroundFunctionExecutor extends HttpServlet {
+public final class BackgroundFunctionExecutor extends AbstractHandler {
   private static final Logger logger = Logger.getLogger("com.google.cloud.functions.invoker");
 
   private final FunctionExecutor<?> functionExecutor;
@@ -223,7 +225,7 @@ private static Context contextFromCloudEvent(CloudEvent cloudEvent) {
    * for the various triggers. CloudEvents are ones that follow the standards defined by <a
    * href="https://cloudevents.io">cloudevents.io</a>.
    *
-   * @param <CloudEventDataT> the type to be used in the {@link Unmarshallers} call when
+   * @param <CloudEventDataT> the type to be used in the {code Unmarshallers} call when
    *     unmarshalling this event, if it is a CloudEvent.
    */
   private abstract static class FunctionExecutor<CloudEventDataT> {
@@ -320,7 +322,9 @@ void serviceCloudEvent(CloudEvent cloudEvent) throws Exception {
 
   /** Executes the user's background function. This can handle all HTTP methods. */
   @Override
-  public void service(HttpServletRequest req, HttpServletResponse res) throws IOException {
+  public void handle(String s, Request baseRequest, HttpServletRequest req, HttpServletResponse res)
+      throws IOException, ServletException {
+    baseRequest.setHandled(true);
     String contentType = req.getContentType();
     try {
       if ((contentType != null && contentType.startsWith("application/cloudevents+json"))

invoker/core/src/main/java/com/google/cloud/functions/invoker/HttpFunctionExecutor.java

@@ -17,14 +17,17 @@
 import com.google.cloud.functions.HttpFunction;
 import com.google.cloud.functions.invoker.http.HttpRequestImpl;
 import com.google.cloud.functions.invoker.http.HttpResponseImpl;
+import java.io.IOException;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import javax.servlet.http.HttpServlet;
+import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+import org.eclipse.jetty.server.Request;
+import org.eclipse.jetty.server.handler.AbstractHandler;
 
 /** Executes the user's method. */
-public class HttpFunctionExecutor extends HttpServlet {
+public class HttpFunctionExecutor extends AbstractHandler {
   private static final Logger logger = Logger.getLogger("com.google.cloud.functions.invoker");
 
   private final HttpFunction function;
@@ -62,8 +65,9 @@ public static HttpFunctionExecutor forClass(Class<?> functionClass) {
   }
 
   /** Executes the user's method, can handle all HTTP type methods. */
-  @Override
-  public void service(HttpServletRequest req, HttpServletResponse res) {
+  public void handle(String s, Request baseRequest, HttpServletRequest req, HttpServletResponse res)
+      throws IOException, ServletException {
+    baseRequest.setHandled(true);
     HttpRequestImpl reqImpl = new HttpRequestImpl(req);
     HttpResponseImpl respImpl = new HttpResponseImpl(res);
     ClassLoader oldContextLoader = Thread.currentThread().getContextClassLoader();
@@ -75,7 +79,7 @@ public void service(HttpServletRequest req, HttpServletResponse res) {
       res.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
     } finally {
       Thread.currentThread().setContextClassLoader(oldContextLoader);
-      respImpl.flush();
+      respImpl.close();
     }
   }
 }

invoker/core/src/main/java/com/google/cloud/functions/invoker/TypedFunctionExecutor.java

@@ -10,16 +10,20 @@
 import com.google.gson.GsonBuilder;
 import java.io.BufferedReader;
 import java.io.BufferedWriter;
+import java.io.IOException;
 import java.lang.reflect.Type;
 import java.util.Arrays;
 import java.util.Optional;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import javax.servlet.http.HttpServlet;
+import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+import org.eclipse.jetty.http.HttpStatus;
+import org.eclipse.jetty.server.Request;
+import org.eclipse.jetty.server.handler.AbstractHandler;
 
-public class TypedFunctionExecutor extends HttpServlet {
+public class TypedFunctionExecutor extends AbstractHandler {
   private static final String APPLY_METHOD = "apply";
   private static final Logger logger = Logger.getLogger("com.google.cloud.functions.invoker");
 
@@ -94,7 +98,9 @@ static Optional<Type> handlerTypeArgument(Class<? extends TypedFunction<?, ?>> f
 
   /** Executes the user's method, can handle all HTTP type methods. */
   @Override
-  public void service(HttpServletRequest req, HttpServletResponse res) {
+  public void handle(String s, Request baseRequest, HttpServletRequest req, HttpServletResponse res)
+      throws IOException, ServletException {
+    baseRequest.setHandled(true);
     HttpRequestImpl reqImpl = new HttpRequestImpl(req);
     HttpResponseImpl resImpl = new HttpResponseImpl(res);
     ClassLoader oldContextClassLoader = Thread.currentThread().getContextClassLoader();
@@ -104,7 +110,7 @@ public void service(HttpServletRequest req, HttpServletResponse res) {
       handleRequest(reqImpl, resImpl);
     } finally {
       Thread.currentThread().setContextClassLoader(oldContextClassLoader);
-      resImpl.flush();
+      resImpl.close();
     }
   }
 
@@ -114,7 +120,7 @@ private void handleRequest(HttpRequest req, HttpResponse res) {
       reqObj = format.deserialize(req, argType);
     } catch (Throwable t) {
       logger.log(Level.SEVERE, "Failed to parse request for " + function.getClass().getName(), t);
-      res.setStatusCode(HttpServletResponse.SC_BAD_REQUEST);
+      res.setStatusCode(HttpStatus.BAD_REQUEST_400);
       return;
     }
 
@@ -123,7 +129,7 @@ private void handleRequest(HttpRequest req, HttpResponse res) {
       resObj = function.apply(reqObj);
     } catch (Throwable t) {
       logger.log(Level.SEVERE, "Failed to execute " + function.getClass().getName(), t);
-      res.setStatusCode(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+      res.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR_500);
       return;
     }
 
@@ -132,7 +138,7 @@ private void handleRequest(HttpRequest req, HttpResponse res) {
     } catch (Throwable t) {
       logger.log(
           Level.SEVERE, "Failed to serialize response for " + function.getClass().getName(), t);
-      res.setStatusCode(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+      res.setStatusCode(HttpStatus.INTERNAL_SERVER_ERROR_500);
       return;
     }
   }
@@ -147,7 +153,7 @@ private static class GsonWireFormat implements TypedFunction.WireFormat {
     @Override
     public void serialize(Object object, HttpResponse response) throws Exception {
       if (object == null) {
-        response.setStatusCode(HttpServletResponse.SC_NO_CONTENT);
+        response.setStatusCode(HttpStatus.NO_CONTENT_204);
         return;
       }
       try (BufferedWriter bodyWriter = response.getWriter()) {

invoker/core/src/main/java/com/google/cloud/functions/invoker/http/HttpRequestImpl.java

@@ -33,12 +33,14 @@
 import java.util.TreeMap;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import javax.servlet.MultipartConfigElement;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.Part;
 
 public class HttpRequestImpl implements HttpRequest {
   private final HttpServletRequest request;
+  private final MultipartConfigElement multipartConfigElement = new MultipartConfigElement("");
 
   public HttpRequestImpl(HttpServletRequest request) {
     this.request = request;
@@ -81,6 +83,7 @@ public Map<String, HttpPart> getParts() {
       throw new IllegalStateException("Content-Type must be multipart/form-data: " + contentType);
     }
     try {
+      request.setAttribute("org.eclipse.jetty.multipartConfig", multipartConfigElement);
       return request.getParts().stream().collect(toMap(Part::getName, HttpPartImpl::new));
     } catch (IOException e) {
       throw new UncheckedIOException(e);

invoker/core/src/main/java/com/google/cloud/functions/invoker/http/HttpResponseImpl.java

@@ -20,13 +20,15 @@
 import java.io.BufferedWriter;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.io.Writer;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.TreeMap;
 import javax.servlet.http.HttpServletResponse;
+import org.eclipse.jetty.util.IO;
 
 public class HttpResponseImpl implements HttpResponse {
   private final HttpServletResponse response;
@@ -43,7 +45,7 @@ public void setStatusCode(int code) {
   @Override
   @SuppressWarnings("deprecation")
   public void setStatusCode(int code, String message) {
-    response.setStatus(code, message);
+    response.setStatus(code);
   }
 
   @Override
@@ -86,32 +88,92 @@ public OutputStream getOutputStream() throws IOException {
   @Override
   public synchronized BufferedWriter getWriter() throws IOException {
     if (writer == null) {
-      // Unfortunately this means that we get two intermediate objects between the object we return
-      // and the underlying Writer that response.getWriter() wraps. We could try accessing the
-      // PrintWriter.out field via reflection, but that sort of access to non-public fields of
-      // platform classes is now frowned on and may draw warnings or even fail in subsequent
-      // versions. We could instead wrap the OutputStream, but that would require us to deduce the
-      // appropriate Charset, using logic like this:
-      // https://github.com/eclipse/jetty.project/blob/923ec38adf/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java#L731
-      // We may end up doing that if performance is an issue.
-      writer = new BufferedWriter(response.getWriter());
+      writer = new NonBufferedWriter(response.getWriter());
     }
     return writer;
   }
 
-  public void flush() {
+  /** Close the response, flushing all content. */
+  public void close() {
     try {
-      // We can't use HttpServletResponse.flushBuffer() because we wrap the
-      // PrintWriter returned by HttpServletResponse in our own BufferedWriter
-      // to match our API. So we have to flush whichever of getWriter() or
-      // getOutputStream() works.
+      IO.close(getOutputStream());
+    } catch (IllegalStateException | IOException e) {
       try {
-        getOutputStream().flush();
-      } catch (IllegalStateException e) {
-        getWriter().flush();
+        IO.close(getWriter());
+      } catch (IOException ioe) {
+        // Too bad, can't close.
       }
-    } catch (IOException e) {
-      // Too bad, can't flush.
+    }
+  }
+
+  /**
+   * A {@link BufferedWriter} that does not buffer. It is generally more efficient to buffer at the
+   * lower level, since frequently total content is smaller than a single buffer and the lower level
+   * buffer can turn a close into a last write that will avoid chunking the response if at all
+   * possible. However, {@link BufferedWriter} is in the API for {@link HttpResponse}, so we must
+   * return a writer of that type.
+   */
+  private static class NonBufferedWriter extends BufferedWriter {
+    private final Writer writer;
+
+    public NonBufferedWriter(Writer out) {
+      super(out, 1);
+      writer = out;
+    }
+
+    @Override
+    public void write(int c) throws IOException {
+      writer.write(c);
+    }
+
+    @Override
+    public void write(char[] cbuf) throws IOException {
+      writer.write(cbuf);
+    }
+
+    @Override
+    public void write(char[] cbuf, int off, int len) throws IOException {
+      writer.write(cbuf, off, len);
+    }
+
+    @Override
+    public void write(String str) throws IOException {
+      writer.write(str);
+    }
+
+    @Override
+    public void write(String str, int off, int len) throws IOException {
+      writer.write(str, off, len);
+    }
+
+    @Override
+    public Writer append(CharSequence csq) throws IOException {
+      return writer.append(csq);
+    }
+
+    @Override
+    public Writer append(CharSequence csq, int start, int end) throws IOException {
+      return writer.append(csq, start, end);
+    }
+
+    @Override
+    public Writer append(char c) throws IOException {
+      return writer.append(c);
+    }
+
+    @Override
+    public void flush() throws IOException {
+      writer.flush();
+    }
+
+    @Override
+    public void close() throws IOException {
+      writer.close();
+    }
+
+    @Override
+    public void newLine() throws IOException {
+      writer.write(System.lineSeparator());
     }
   }
 }