Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using promises in prehandlers causes exceptions to be swallowed in handlers. #3242

Closed
johnbrett opened this issue Jul 25, 2016 · 14 comments
Closed
Assignees
Labels
Milestone

Comments

@johnbrett
Copy link
Contributor

@johnbrett johnbrett commented Jul 25, 2016

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 #3102 but the fix 8babca3 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 hapi@13.5.0 on Node 6.

@devinivy
Copy link
Member

@devinivy devinivy commented Jul 25, 2016

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
Copy link
Contributor Author

@johnbrett johnbrett commented Jul 25, 2016

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
Copy link
Contributor

@Marsup Marsup commented Jul 25, 2016

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
Copy link
Contributor Author

@johnbrett johnbrett commented Jul 25, 2016

@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 #3073?

@Marsup
Copy link
Contributor

@Marsup Marsup commented Jul 25, 2016

@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
Copy link
Contributor Author

@johnbrett johnbrett commented Jul 25, 2016

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
Copy link
Member

@devinivy devinivy commented Jul 26, 2016

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

@hueniverse hueniverse closed this Aug 23, 2016
@hueniverse hueniverse assigned hueniverse and Marsup and unassigned hueniverse Aug 23, 2016
@hueniverse hueniverse added the bug label Aug 23, 2016
@hueniverse hueniverse added this to the 15.0.0 milestone Aug 23, 2016
@johnbrett
Copy link
Contributor Author

@johnbrett johnbrett commented Aug 23, 2016

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
Copy link
Contributor

@hueniverse hueniverse commented Aug 23, 2016

Someone should write a promises section in the API doc.

@Marsup
Copy link
Contributor

@Marsup Marsup commented Aug 23, 2016

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

@hueniverse hueniverse reopened this Aug 23, 2016
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 23, 2016

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
Copy link
Contributor Author

@johnbrett johnbrett commented Aug 23, 2016

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

@hueniverse hueniverse removed this from the 15.0.0 milestone Aug 26, 2016
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 26, 2016

Removed from v15 until figured out.

@mcortesi
Copy link

@mcortesi mcortesi commented Aug 26, 2016

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!!!

hueniverse added a commit that referenced this issue Nov 29, 2016
fix server not propagating errors on prehandler(promise) + handler error (#3242)
hueniverse added a commit that referenced this issue Nov 29, 2016
@hueniverse hueniverse added this to the 16.0.0 milestone Nov 29, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants