Reorganized Router error handling #1140

vertx-web/src/main/asciidoc/index.adoc

@@ -620,8 +620,9 @@ locale was provided by the user.
 
 If no routes match for any particular request, Vert.x-Web will signal a 404 error.
 
-This can then be handled by your own error handler, or perhaps the augmented error handler that we supply to use,
-or if no error handler is provided Vert.x-Web will send back a basic 404 (Not Found) response.
+
+A default 404 error handler is provided by Router. Anyway you can provide your own 404 error handler with
+{@link io.vertx.ext.web.Router#errorHandler}
 
 == Error handling
 

vertx-web/src/main/java/io/vertx/ext/web/Router.java

@@ -363,10 +363,24 @@ default void accept(HttpServerRequest request) {
    *
    * @param exceptionHandler  the exception handler
    * @return a reference to this, so the API can be used fluently
+   * @deprecated you should use {@link Router#errorHandler(int, Handler)} with 500 status code
    */
+  @Deprecated
   @Fluent
   Router exceptionHandler(@Nullable Handler<Throwable> exceptionHandler);
 
+  /**
+   * Specify an handler to handle an error for a particular status code. You can use to manage general errors too using status code 500.
+   * The handler will be called when the context fails and other failure handlers didn't write the reply or when an exception is thrown inside an handler.
+   * You <b>must not</b> use {@link RoutingContext#next()} inside the error handler
+   * This does not affect the normal failure routing logic.
+   *
+   * @param statusCode status code the errorHandler is capable of handle
+   * @param errorHandler error handler. Note: You <b>must not</b> use {@link RoutingContext#next()} inside the provided handler
+   * @return a reference to this, so the API can be used fluently
+   */
+  Router errorHandler(int statusCode, Handler<RoutingContext> errorHandler);
+
   /**
    * Used to route a context to the router. Used for sub-routers. You wouldn't normally call this method directly.
    *

vertx-web/src/main/java/io/vertx/ext/web/RoutingContext.java

@@ -51,6 +51,8 @@
  * <p>
  * The context also provides access to the {@link Session}, cookies and body for the request, given the correct handlers
  * in the application.
+ * <p>
+ * If you use the internal error handler
  *
  * @author <a href="http://tfox.org">Tim Fox</a>
  */
@@ -83,22 +85,36 @@ public interface RoutingContext {
    * Fail the context with the specified status code.
    * <p>
    * This will cause the router to route the context to any matching failure handlers for the request. If no failure handlers
-   * match a default failure response will be sent.
+   * match It will trigger the error handler matching the status code. You can define such error handler with
+   * {@link Router#errorHandler(int, Handler)}. If no error handler is not defined, It will send a default failure response with provided status code.
    *
    * @param statusCode  the HTTP status code
    */
   void fail(int statusCode);
 
   /**
-   * Fail the context with the specified throwable.
+   * Fail the context with the specified throwable and 500 status code.
    * <p>
    * This will cause the router to route the context to any matching failure handlers for the request. If no failure handlers
-   * match a default failure response with status code 500 will be sent.
+   * match It will trigger the error handler matching the status code. You can define such error handler with
+   * {@link Router#errorHandler(int, Handler)}. If no error handler is not defined, It will send a default failure response with 500 status code.
    *
    * @param throwable  a throwable representing the failure
    */
   void fail(Throwable throwable);
 
+  /**
+   * Fail the context with the specified throwable and the specified the status code.
+   * <p>
+   * This will cause the router to route the context to any matching failure handlers for the request. If no failure handlers
+   * match It will trigger the error handler matching the status code. You can define such error handler with
+   * {@link Router#errorHandler(int, Handler)}. If no error handler is not defined, It will send a default failure response with provided status code.
+   *
+   * @param statusCode the HTTP status code
+   * @param throwable a throwable representing the failure
+   */
+  void fail(int statusCode, Throwable throwable);
+
   /**
    * Put some arbitrary data in the context. This will be available in any handlers that receive the context.
    *

vertx-web/src/main/java/io/vertx/ext/web/impl/RouterImpl.java

@@ -16,6 +16,7 @@
 
 package io.vertx.ext.web.impl;
 
+import io.netty.handler.codec.http.HttpHeaderNames;
 import io.vertx.core.Handler;
 import io.vertx.core.Vertx;
 import io.vertx.core.http.HttpMethod;
@@ -27,6 +28,7 @@
 import io.vertx.ext.web.RoutingContext;
 
 import java.util.*;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentSkipListSet;
 import java.util.concurrent.atomic.AtomicInteger;
 
@@ -59,7 +61,6 @@ public class RouterImpl implements Router {
     return compare;
   };
 
-
   private static final Logger log = LoggerFactory.getLogger(RouterImpl.class);
 
   private final Vertx vertx;
@@ -70,7 +71,7 @@ public RouterImpl(Vertx vertx) {
   }
 
   private final AtomicInteger orderSequence = new AtomicInteger();
-  private Handler<Throwable> exceptionHandler;
+  private Map<Integer, Handler<RoutingContext>> errorHandlers = new ConcurrentHashMap<>();
 
   @Override
   public void handle(HttpServerRequest request) {
@@ -272,9 +273,19 @@ public Router mountSubRouter(String mountPoint, Router subRouter) {
     return this;
   }
 
+  @Deprecated
   @Override
   public synchronized Router exceptionHandler(Handler<Throwable> exceptionHandler) {
-    this.exceptionHandler = exceptionHandler;
+    if (exceptionHandler != null) {
+      this.errorHandler(500, routingContext -> exceptionHandler.handle(routingContext.failure()));
+    }
+    return this;
+  }
+
+  @Override
+  public Router errorHandler(int statusCode, Handler<RoutingContext> errorHandler) {
+    Objects.requireNonNull(errorHandler);
+    this.errorHandlers.put(statusCode, errorHandler);
     return this;
   }
 
@@ -294,8 +305,8 @@ Iterator<RouteImpl> iterator() {
     return routes.iterator();
   }
 
-  Handler<Throwable> exceptionHandler() {
-    return exceptionHandler;
+  Handler<RoutingContext> getErrorHandlerByStatusCode(int statusCode) {
+    return errorHandlers.get(statusCode);
   }
 
   private String getAndCheckRoutePath(RoutingContext ctx) {

vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextDecorator.java

@@ -83,6 +83,11 @@ public void fail(Throwable throwable) {
     vertx().runOnContext(future -> decoratedContext.fail(throwable));
   }
 
+  @Override
+  public void fail(int statusCode, Throwable throwable) {
+    vertx().runOnContext(future -> decoratedContext.fail(statusCode, throwable));
+  }
+
   @Override
   public boolean failed() {
     return decoratedContext.failed();

vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImpl.java

@@ -142,16 +142,22 @@ private void checkHandleNoMatch() {
       // Send back FAILURE
       unhandledFailure(statusCode, failure, router);
     } else {
-      // Send back default 404
-      response().setStatusCode(404);
-      if (request().method() == HttpMethod.HEAD) {
-        // HEAD responses don't have a body
-        response().end();
-      } else {
-        response()
-                .putHeader(HttpHeaderNames.CONTENT_TYPE, "text/html; charset=utf-8")
-                .end(DEFAULT_404);
-      }
+      Handler<RoutingContext> handler = router.getErrorHandlerByStatusCode(404);
+      if (handler == null) { // Default 404 handling
+        // Send back default 404
+        this.response()
+          .setStatusMessage("Not Found")
+          .setStatusCode(404);
+        if (this.request().method() == HttpMethod.HEAD) {
+          // HEAD responses don't have a body
+          this.response().end();
+        } else {
+          this.response()
+            .putHeader(HttpHeaderNames.CONTENT_TYPE, "text/html; charset=utf-8")
+            .end("<html><body><h1>Resource not found</h1></body></html>");
+        }
+      } else
+        handler.handle(this);
     }
   }
 
@@ -163,7 +169,13 @@ public void fail(int statusCode) {
 
   @Override
   public void fail(Throwable t) {
-    this.failure = t == null ? new NullPointerException() : t;
+    this.fail(-1, t);
+  }
+
+  @Override
+  public void fail(int statusCode, Throwable throwable) {
+    this.statusCode = statusCode;
+    this.failure = throwable == null ? new NullPointerException() : throwable;
     doFail();
   }
 

vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImplBase.java

@@ -17,6 +17,7 @@
 package io.vertx.ext.web.impl;
 
 import io.netty.handler.codec.http.HttpResponseStatus;
+import io.vertx.core.Handler;
 import io.vertx.core.http.HttpServerRequest;
 import io.vertx.core.logging.Logger;
 import io.vertx.core.logging.LoggerFactory;
@@ -134,10 +135,11 @@ protected boolean iterateNext() {
           }
           return true;
         }
-      } catch (IllegalArgumentException e) {
+      } catch (Throwable e) {
         if (log.isTraceEnabled()) log.trace("IllegalArgumentException thrown during iteration", e);
-        // Failure in handling failure!
-        unhandledFailure(400, e, route.router());
+        // Failure in matches algorithm (If the exception is instanceof IllegalArgumentException probably is a QueryStringDecoder error!)
+        if (!this.response().ended())
+          unhandledFailure((e instanceof IllegalArgumentException) ? 400 : -1, e, route.router());
         return true;
       }
     }
@@ -147,11 +149,12 @@ protected boolean iterateNext() {
 
   protected void unhandledFailure(int statusCode, Throwable failure, RouterImpl router) {
     int code = statusCode != -1 ? statusCode : 500;
-    if (failure != null) {
-      if (router.exceptionHandler() != null) {
-        router.exceptionHandler().handle(failure);
-      } else {
-        log.error("Unexpected exception in route", failure);
+    Handler<RoutingContext> errorHandler = router.getErrorHandlerByStatusCode(code);
+    if (errorHandler != null) {
+      try {
+        errorHandler.handle(this);
+      } catch (Throwable t) {
+        log.error("Error in error handler", t);
       }
     }
     if (!response().ended()) {

vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextWrapper.java

@@ -73,6 +73,11 @@ public void fail(Throwable throwable) {
     inner.fail(throwable);
   }
 
+  @Override
+  public void fail(int statusCode, Throwable throwable) {
+    inner.fail(statusCode, throwable);
+  }
+
   @Override
   public RoutingContext put(String key, Object obj) {
     inner.put(key, obj);

vertx-web/src/test/java/io/vertx/ext/web/RouterTest.java

@@ -2355,4 +2355,48 @@ public void testMultipleHandlersWithFailuresDeadlock() throws Exception {
     }
     awaitLatch(latch);
   }
+
+  @Test
+  public void testCustom404ErrorHandler() throws Exception {
+    // Default 404 handler
+    testRequest(HttpMethod.GET, "/blah", 404, "Not Found", "<html><body><h1>Resource not found</h1></body></html>");
+    router.errorHandler(404, routingContext -> routingContext
+      .response()
+      .setStatusMessage("Not Found")
+      .setStatusCode(404)
+      .end("Not Found custom error")
+    );
+    testRequest(HttpMethod.GET, "/blah", 404, "Not Found", "Not Found custom error");
+  }
+
+  @Test
+  public void testDecodingErrorCustomHandler() throws Exception {
+    String BAD_PARAM = "~!@\\||$%^&*()_=-%22;;%27%22:%3C%3E/?]}{";
+
+    router.errorHandler(400, context -> context.response().setStatusCode(500).setStatusMessage("Dumb").end());
+
+    router.route().handler(rc -> rc.next());
+    router.route("/path").handler(rc -> rc.response().setStatusCode(500).end());
+    testRequest(HttpMethod.GET, "/path?q=" + BAD_PARAM, 500,"Dumb");
+  }
+
+  @Test
+  public void testCustomErrorHandler() throws Exception {
+
+    router.route("/path").handler(rc -> rc.fail(410));
+    router.errorHandler(410, context -> context.response().setStatusCode(500).setStatusMessage("Dumb").end());
+
+    testRequest(HttpMethod.GET, "/path", 500, "Dumb");
+  }
+
+  @Test
+  public void testErrorInCustomErrorHandler() throws Exception {
+
+    router.route("/path").handler(rc -> rc.fail(410));
+    router.errorHandler(410, rc -> {
+      throw new RuntimeException();
+    });
+
+    testRequest(HttpMethod.GET, "/path", 410, "Gone");
+  }
 }