Vertx-web: fail() doesn't seem to immediately fail a route

[DanielJoyce]

Version

• vertx3-full: 3.3.2

used from Javascript

Context

fail() doesn't seem to immediately fail a route. This run counter to every other web framework I've used

Steps to reproduce

export function getParamAs<T>(name: string, ctxt: RoutingContext, required = false, def: T = null, converter: (string) => T): string | T {
  let v = ctxt.request().getParam(name);
  if (v !== null && v !== undefined) {
    if (converter !== null && converter !== undefined) {
      return converter(v);
    }
    return v;
  } else {
    if (def !== null && def !== undefined) {
      return def;
    } else {
      if (required) {
        // Necessary parameter missing
        ctxt.fail(400);
        // Apparently .fail() doesn't work as it does in nearly every other framework,
        // the ctxt still proceeds to process the rest of the handler that called this function!!!

        // Throwing fails and aborts the handler as expected.
        //throw new Error("Missing required param");
        // return null;
      } else {
        return null;
      }
    }
  }
}

router.post("/derp").handler((ctxt) => {
  // derp is required, so getParamAs will fail the ctxt if not set
  let derp = getParamAs<string>("derp", ctxt, true);  
  // ... EXTRA STUFF... Still executes
  ctxt.response().end("Will cause your error handler to fail to write a response because this already executed!");
});

router.post("/derp2").handler((ctxt) => {
  // derp is required, so getParamAs will fail the ctxt if not set
  let derp = getParamAs<string>("derp", ctxt, true);
  if(!ctxt.failed()){
    // This seems bizzarely messy and unintuitive!
  }
});

Even if you call fail(500), EXTRA STUFF is still executed. Most other frameworks, things like .fail() or notFound() immediately abort the current handler via a exception under the hood, and then call the error handler.

I bumped into this because I made helper functions to parse required params, and if a param is missing, call ctxt.fail(), but that didn't immediately abort further processing in the handler.

[DanielJoyce]

True, that is a way to fix it. But most every other java framework I've used, fail() or notFound(), or any other method that aborts a request does so with a exception under the hood, so testing is not needed. Play does this. Many python ones do this.

fail() should fail, and prevent all further processing without explicit checking. I mean thats the definition of 'fail'. Not 'kinda-fail' but 'fail'.

If this is a design choice, then it should be documented more.

[DanielJoyce]

That, or vertx should provide methods that throw exceptions for common cases, and the default error handler can then process them.

notFound(customMessage)

abort(statusCode, msg)

[alexlehm]

sorry, i removed my previous reply since I was thinking more about java than javascript (though the question applies of course).
I haven't programmed in vertx-web much yet, maybe it would be best if we wait if Paulo has a suggestion

[tsegismont]

Vert.x is a non blocking toolkit, so it won't throw an exception if you
fail the routing context. As for the comparison with other frameworks, if
you use a @Suspended AsyncResponse in JAX-RS, execution doesn't stop after
the response is resumed. Here is an example https://git.io/vMg6S

If you want to extract common request params validation code, you could add
a specific handler to every route. If validation fails, simply don't call

next and the next handler (with your business logic) won't be invoked.

See
http://vertx.io/docs/vertx-web/java/#_handling_requests_and_calling_the_next_handler

2017-01-12 23:59 GMT+01:00 Daniel [e-mail]:

True, that is a way to fix it. But most every other java framework I've
used, fail() or notFound(), or any other method that aborts a request does
so with a exception under the hood, so testing is not needed. Play does
this. Many python ones do this.

fail() should fail, and prevent all further processing without explicit
checking. I mean thats the definition of 'fail'. Not 'kinda-fail' but
'fail'.

If this is a design choice, then it should be documented more.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/vert-x3/vertx-web/issues/510#issuecomment-272311164,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABblto4qW1HRhfRx1ss65EUSkYhgbcHxks5rRrBTgaJpZM4LiU6I
.

[DanielJoyce]

I don't see how throwing an exception matters compared to blocking/sync/async. If its a performance concern, for convenience methods you can suppress stack trace generation.

[tsegismont]

It matters because in blocking workflows you can use exceptions to abort a
process, in non blocking workflows you can't (or chances are high for some
handlers to never get called).

2017-01-13 0:25 GMT+01:00 Daniel [e-mail]:

I don't see how throwing an exception matters compared to
blocking/sync/async. If its a performance concern, for convenience methods
you can suppress stack trace generation.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/vert-x3/vertx-web/issues/510#issuecomment-272316255,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABbltlsWRJ5Tn42qdjIiLvYlIWIAJzvPks5rRrZRgaJpZM4LiU6I
.

[DanielJoyce]

Vertx already makes no guarantees that if a handler raises an exception, other subsequent handlers for the same route will be called. It can't. A exception is treated as a fail that just happens to immediately end processing of the current route.

So again, still not an issue. If your jdbc layer raises an exception, the current handler will end immediately and subsequent ones will not be called.

[DanielJoyce]

// Using fail()
router.get('/fail').handler(ctxt =>{
  console.log("I FAILED!");
  ctxt.fail(400);
  ctxt.response().end("But I will make this barf in the logs!!!");
  // This is what you see in the logs   
  // FailHandler: Response has already ended! 
  // {"message":"Response has already been written","name":"java.lang.IllegalStateException: Response has already been written"} 
});
router.get('/fail').handler(ctxt =>{
  ctxt.response().end("Should not get called!");
  console.log("Should not get called!");
});
router.get('/fail').handler(ctxt =>{
  ctxt.response().end("Should not get called 2!");
  console.log("Should not get called 2!");
});


// Throwing exception
router.get('/except').handler(ctxt =>{
  console.log("Same as above but immedate abort of this handler")
  throw new Error("ABORT!!!");
  // Unreachable! Hah!
  //ctxt.response().end("NO BARFY, nothing in logs!!");
});
router.get('/except').handler(ctxt =>{
  ctxt.response().end("Should not get called!");
  console.log("Should not get called!");
});
router.get('/except').handler(ctxt =>{
  ctxt.response().end("Should not get called 2!");
  console.log("Should not get called 2!");
});

So again, fail() keeps subsequent routes from being called, as does throwing an exception. But an exception immediately ends processing where fail needs more explicit checking otherwise it may conflict with the error handler trying to write to an already sent response.

[pmlopes]

This is not an issue, it is by design. The idea of fail is to start the error handling procedure in vertx-web while allowing you to keep running some code say for example, clean up.

The envisioned use was that users would do some logical decision like:

if (some_condition) {
  ctx.response().end("OK");
} else {
  ctx.fail("Oops!");
}

Now say that you want to log that:

if (some_condition) {
  ctx.response().end("OK");
} else {
  ctx.fail("Oops!");
  getMeALogger().error("Whoops!!!");
}

Now if we rely on Exceptions we can't do this "sort of" finally code. You showed us that probably the vision we had for the API was not obvious and we could try to enhance the documentation.

Now if you want to rely on exceptions, won't the exception handler do what you're looking for? see: http://vertx.io/docs/apidocs/io/vertx/ext/web/Router.html#exceptionHandler-io.vertx.core.Handler-

[suspended]

Don't tag me. Thanks