Hapi: Using promises in prehandlers causes exceptions to be swallowed in handlers.

[johnbrett]

I have the following test case, when you send a request to the server it will hang. It doesn't respond, and nothing is logged to the console (this cost me hours of debugging earlier :/)

const Hapi = require('hapi');

const server = new Hapi.Server();
server.connection({ host: 'localhost', port: 8999 });

const pre = (request, reply) => {
  return reply(Promise.resolve('FROM PRE HANDLER'))
};

server.route({
    method: 'GET',
    path:'/', 
    config: {
      pre: [{ method: pre, assign: 'test' }]
    },
    handler: function (request, reply) {

        throw new Error('this is an error ' + request.pre.test)
        return reply('hello world ' + request.pre.test);
    }
});

server.start((err) => {
    if (err) { throw err; }
    console.log('Server running at:', server.info.uri);
});

@nlf, I saw this which might be related https://github.com/hapijs/hapi/issues/3102 but the fix https://github.com/hapijs/hapi/commit/8babca3ef9271ede738b0377d628e62c94d24f04 doesn't cover this either. I'm trying to debug this further and provide a fix, but I'm not a promises expert, finding it tough to get to the bottom of it so maybe someone might be able to help!

This is with [email protected] on Node 6.

[devinivy]

Very possible #3073 and PR #3202 are related to this. It seems that promises escape the domain used by the handler. Do you mind seeing if this occurs with the fix introduced by #3202?

[johnbrett]

Hey @devinivy - thanks for the quick reply!

So this does have some effect, what happens with #3202 applied is the process logs the error and crashes. This is definitely an improvement when it comes to debugging!! Would it be possible for this to handle like an exception thrown in a handler normally? Error gets logged, 500 response sent but server doesn't crash?

[Marsup]

I've been trying to unit test the PR yesterday and noticed that. I'm not sure what is causing that yet but I need to rebind the function on the domain. This PR sure needs some love but it might not be coming from me immediately.

[johnbrett]

@Marsup cheers for the work on it so far. Will take a look, unfortunately my knowledge of domains is minimal but hopefully I can contribute something. Should we close this and track everything in https://github.com/hapijs/hapi/issues/3073?

[Marsup]

@johnbrett I pushed what I had so far, it's still missing tests for the others nextTick I added, and also I can't really explain why I need to rebind on the domain which is kind of bothering me...

[johnbrett]

Thanks @Marsup. Will dog some more. Random interesting piece of info for you, with your fix and run with babel-node this works as expected. 500 error, logs to console, server doesn't crash.

[devinivy]

Haha, very odd! Thanks for looking into that @johnbrett. I might try to pickup #3202 when I find some time. We're getting close!

[johnbrett]

Just FYI - the behaviour on this now is that the the error is logged and the process exits. The error doesn't get caught by the domain it seems. If this is the best we can do I can work around it, just want to point out it still kills the process!

Thanks for the work on this everyone!

[hueniverse]

Someone should write a promises section in the API doc.

[Marsup]

Not sure if it's better if that's the case @johnbrett, if the server bails I'd consider it a regression.

[hueniverse]

Can you guys figure out what you want to do here? I don't use promises and have no interest in diving into this.

[johnbrett]

Will try @hueniverse! :) I'll keeping trying to identify the issue...but I'm not having much luck. Domains are hard :/

[hueniverse]

Removed from v15 until figured out.

[mcortesi]

Hi all!

I've the same issue on my application and I've diggin into hapi.js code to understand what is happenning.

Here is a gist with all the cases I've found https://gist.github.com/mcortesi/744efd25b721d08b187afed75b2b5472

Typically, what breaks hapi.js expectation is using a promises in an request lifecycle method or in the handler it self. Since unhandled rejected promises are not reported to the node domain that hapi uses as a safety net to catch all errors and respond.

So, this breaks:

// No Log, No response
server.route({
    method: 'GET',
    path: '/handler/promise-throw',
    config: {
      timeout: { server: 1000 }
    },
    handler: function (request, reply) {
      return BPromise.delay(10).then(() => {
        throw new Error('Promise Throw')
      })
    }
}); 

And this:

// No Log, No response
server.route({
    method: 'GET',
    path: '/ext/promise-throw',
    config: {
      // timeout: { server: 1000 },
      ext: {
        onPreHandler: {
          method: (request, reply) => {
            BPromise.delay(10).then(() => { throw new Error('Promise Throw') });
          }
        }
      }
    },
    handler: function (request, reply) {
      reply('Everything is good!');
    }
});

But the most problematic in my opinion is when the lifecycle method doesn't fail, but wraps everything in a promise context, thus all the subsequent calls are within that context.

Like this:

// No Log, No response
server.route({
    method: 'GET',
    path: '/ext/promise-reply-continue',
    config: {
      timeout: { server: 1000 },
      ext: {
        onPreHandler: {
          method: (request, reply) => {
            BPromise.delay(10).then(() => {
              reply.continue();
            })
          }
        }
      }
    },
    handler: function (request, reply) {
      throw new Error("Handler has thrown");
    }
});

This happens because when hapi.js call the route lyfecycle methods, it does so synchronously, one by one. So the handler is within the stacktrace of the then() handler. In fact, that throw is making the promise fail, you could do a .catch() there and get the handler error. Which is weird.

Promises and domains don't play nice together, and it's discussable if they should. Rejected promises and _values that you could pass around_; and when you have an unhandled rejected promises, you don't actually know for certain that it won't be handled later by someone (you can know that only when you are garbage collecting the promise).

Ways to solve this?:

• Set a server timeout, for the case we have an uncatched error.
• Don't do a reply continue within a promise's then. You can use something as asCallback() to go from promises world to callbacks world again.
• _wrap_ everything with a function that catches promises errors.

For example, for handlers I'm playing with something like:

  const HandlerOptionsSchema = Joi.object().keys({
    f: Joi.func().required(),
  });

  function tryHandlerFactory(route, options) {

    const result = Joi.validate(options, HandlerOptionsSchema);

    if (result.error) {
      throw new Error(result.error);
    }

    return function tryHandler(req, reply) {
      Promise.try(() => options.f(req))
        .then(ret => reply(ret))
        .catch(err => reply(Boom.wrap(err)));
    };
  }

server.handler('try', tryHandlerFactory);

I'm assuming here that now my handlers, have the signature: Request -> Promise<Response>.

But i think it would be nicer if we could have a first class citizen support to promises within hapi. Since neither of these solutions are optimal. I'm happy to help implementing that if that's an option!

Sorry for the long post!!!