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

Provide cover for unhandled rejections #3486

Merged
merged 3 commits into from Jun 5, 2017
Merged

Provide cover for unhandled rejections #3486

merged 3 commits into from Jun 5, 2017

Conversation

@dwmkerr
Copy link
Contributor

@dwmkerr dwmkerr commented May 18, 2017

This small change allows an extra degree of protection for those who might be using promises in their handlers.

Problem Statement

Developers who are using promises in their handlers will often write handlers which look like this:

function handler(request, reply) {
  doSomething
    .then((interimResult) => {
      return somethingElse(interimResult);
    })
    .then((result) => {
      reply(result.whatever);
    });

The challenge is that this opens up lots of opportunities for exceptions at different points to lead to uncaught promise rejections. For example:

function handler(request, reply) {
  doSomething
    .then((interimResult) => {
      throw new Error('whoops...');
    })
    .then((result) => {
      reply(result.whatever);
    });

will lead to a timeout of the request.

Suggested Change

Rather than attempting some kind of large scale change which adds more support for promises (which is not needed, as reply can take a promise), I propose allowing the handler to return a promise. If the promise is returned, then hapi can register a catch and reply with any error. This means that if the developer goofs up and forgets to catch (all to easy with the promise spec), hapi at least will attempt to deal with the error in the same way it would as if an exception was thrown.

This PR adds the support, updates the docs and tests.

Questions

Do we need this?

It is possible to rewrite the dodgy code as:

function handler(request, reply) {
  reply(doSomething
    .then((interimResult) => {
      return somethingElse(interimResult);
    })
    .then((result) => {
      return result.whatever;
    }));

However this tends to feel a little unnatural, as the natural reading order is more 'do work, reply with the result'. Fairly subjective but I've seen quite a few devs on teams default to doing it the way described in the problem statement, rather than this. It also makes doing things like changing the status code conditionally based on the result near impossible. (CMIIW!)

Of course, people could just avoid promises (which might not be a bad idea given some of the issues around how they can swallow exceptions), but at least an appreciable proportion of people are using them heavily.

Should we ever have 'partial promise' support

Really, if you are returning a promise in the handler it might make more sense to simply not even have the reply function used, and change the spec to say if you return a promise, hapi with reply with the result or if there is an error, wrap it in Boom and then reply with that.

This would perhaps be cleaner in some cases:

function handler(req, reply) {
  return doSomething().then(r = doSomethingElse(r));
}

But is a larger structural change. Also, it still means we cannot easily change status codes, add headers etc.

I think this is a happier middle group - hapi's API stays the same, but it has a 'safety net' for promises.

That's It!

I'd love to get any feedback on this as a potential feature. It would certainly help myself and my colleagues on some of the projects we're on at the moment, I believe it allows us to provide a safety net for potentially nasty issues (promise black holes are a pain) but am happy to take any input for changes!

Related Issues

@devinivy
Copy link
Member

@devinivy devinivy commented May 18, 2017

Hey, just wanted to share– there are two things that can help you think about this and code around this in the interim.

  1. You can reply with a pre-generated response– this way, you can turn promise results into responses with status codes, cookies, headers, etc. See request.generateResponse().
  2. I have opted to handle general-purpose promise rejections with a helper that you can use, Toys.handler(). That's designed for use with async/await, but really works with any promise-returning function.

My impression is that there is not any intention of porting this feature into hapi land, but we shall see :)

@dwmkerr
Copy link
Contributor Author

@dwmkerr dwmkerr commented May 18, 2017

Thanks @devinivy, I like the tips! I'll look into both as an interim solution. I'd like to stay as close to hapi semantics as I can and avoid wrapping it with other code, but definitely have to try and find a way to reduce the likelihood of those unhandled rejections occurring.

I'm a fan of promises in how they can make some code a little easier to reason about, but the extra effort required to make sure people are getting all of the error conditions right is a nightmare!

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented May 19, 2017

Can you not add a process.on('unhandledRejection', (err, promise) => {}) to catch that behaviour?

@dwmkerr
Copy link
Contributor Author

@dwmkerr dwmkerr commented May 19, 2017

Hi @AdriVanHoudt,

The challenge with this is that there is no way to then actually call reply and successfully continue with the request pipeline. What I am aiming for is a way for an unhandled rejection to go through the same workflow as an unhandled exception. (i.e. with this approach I can still use onPreReply etc etc to transform the error if needed)

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented May 19, 2017

Ok but normally you should catch this in testing no?
Btw I think the proposed PR is not bad, less ways to shoot yourself in the foot is 👌

Copy link
Contributor

@kanongil kanongil left a comment

I definitely approve of handling returned Promise rejection values, like you suggest. This should mirror the way that regular throw's in the handler function are handled.

lib/handler.js Outdated

// Give hapi a fighting chance to deal with uncaught rejections.
if (Promises.isThennable(handlerResult)) {
handlerResult.catch(reply);
Copy link
Contributor

@kanongil kanongil May 19, 2017

It's probably better to use .then(null, reply) instead of catch(), since we haven't tested that the catch() method exists.

Copy link
Contributor

@kanongil kanongil May 19, 2017

It also needs to handle rejections where the object is not an instanceof Error, like this code:

hapi/lib/response.js

Lines 471 to 480 in b4aeaf7

const onError = (source) => {
if (!(source instanceof Error)) {
const err = new Error('Rejected promise');
err.data = source;
return next(Boom.wrap(err));
}
return next(Boom.wrap(source));
};
.

Copy link
Contributor Author

@dwmkerr dwmkerr May 19, 2017

Hi @kanongil yes it needs to use .then, goofy mistake on my side. For the second point that's a great spot, am checking on this now.

Copy link
Contributor Author

@dwmkerr dwmkerr May 19, 2017

fixed, also added support for wrapping the object in an error needed to maintain parity with the unhandled exceptions. would welcome advice on whether you think the logic to wrap an error in a boom object is general enough to extract into its own function to avoid duplication of logic @kanongil. It's a few line and might be clearer left inline, but am happy to extract it (perhaps into promise.js as a boomReject function or something)

API.md Outdated
return badPromise()
.then((result) => { reply(result); });
}
```
Copy link
Contributor

@kanongil kanongil May 19, 2017

Bad indentation.

Copy link
Contributor Author

@dwmkerr dwmkerr May 19, 2017

On it

API.md Outdated
new Promise(() => {
setTimeout(() => throw new Error(), 1000);
Copy link
Contributor

@kanongil kanongil May 19, 2017

The setTimeout seems redundant, and confuses the example.

Copy link
Contributor

@AJamesPhillips AJamesPhillips May 19, 2017

I guess it's to show that errors thrown from asynchronous functions inside Promises are caught and handled correctly. By extension, synchronous errors will be caught and handled correctly too. (?) Happy to be corrected.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/catch#Gotchas_when_throwing_errors

Copy link
Contributor Author

@dwmkerr dwmkerr May 19, 2017

@kanongil @AJamesPhillips yes that's correct, it's to make it clear and explicit that this method will work even if exception is thrown in a genuinely async context (a nextTick could be used if the fact that it is a timeout might be misleading people into thinking it is a timing related issue)

Maybe there's something I'm missing here, but unless there's some magic going on behind the scenes with domains this should result in resolving with undefined and triggering an uncaughtException on the process.

Copy link
Contributor Author

@dwmkerr dwmkerr May 19, 2017

Let me make sure the test case is exactly right for this to confirm everything, working on the changes now

Copy link
Contributor Author

@dwmkerr dwmkerr May 19, 2017

@TimBeyer thanks for the heads up, you are quite right. In this case the following is hit:

https://github.com/hapijs/hapi/blob/master/lib/protect.js#L55

And so this case is handled. This feature will only solve for unhandled rejections and exceptions thrown synchronously in a promise constructor, I'm updating the tests to correctly reflect this.

Copy link
Contributor Author

@dwmkerr dwmkerr May 19, 2017

Added a test case showing that the setTimeout example is already handled by the protect mechanism, to keep things clearer. Also attempted to make the api documentation sharper.

API.md Outdated
}
// You *should* catch the rejection yourself! But hapi will
// return the error for you...
Copy link
Contributor

@kanongil kanongil May 19, 2017

Why? You should only need to catch it, if you want to handle it in a special way.

Copy link
Contributor Author

@dwmkerr dwmkerr May 19, 2017

I'll make the comment clear, what I meant was that if you are orchestrating a promise chain, you should pretty much always use a catch (unless you return the promise to something else which can catch it). In this case as hapi would be able to catch it, the comment is misleading

@dwmkerr
Copy link
Contributor Author

@dwmkerr dwmkerr commented May 19, 2017

@AdriVanHoudt agreed, tests should cover these scenarios, but I think it still adds a layer of safety as even for experienced developers it can be easy to miss an edge case with a promise


return new Promise((resolve, reject) => {

throw new Error('This should be rejected...');
Copy link
Contributor

@AJamesPhillips AJamesPhillips May 19, 2017

I'd add a test for your setTimeout(() => throw new Error(), 1000); too from above.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/catch#Gotchas_when_throwing_errors

Copy link
Contributor Author

@dwmkerr dwmkerr May 19, 2017

Good call! Adding now

Copy link
Contributor Author

@dwmkerr dwmkerr May 19, 2017

Added the test, it's there now :)

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented May 19, 2017

@dwmkerr 👌 as @kanongil pointed out this actually matches the behaviour with callbacks so that is even an extra for me

@dwmkerr
Copy link
Contributor Author

@dwmkerr dwmkerr commented May 23, 2017

Hi @AdriVanHoudt yes it now goes through the same process as with callbacks, transforming the error by wrapping it in Boom as needed, so the behaviour of both is very close now 😄

handlerResult.then(null, (error) => {

// Unhandled rejections are always Boom-ified, just like uncaught exceptions.
if (error instanceof Error) {
Copy link
Contributor

@hueniverse hueniverse May 30, 2017

Is it common practice to reject a promise with a non-Error object?!

Copy link
Member

@nlf nlf May 30, 2017

i sure hope not..

Copy link
Member

@devinivy devinivy May 30, 2017

You certainly aren't supposed to, but out in the wild it happens. As to whether hapi should deal with the rejection of a non-error, I can't say. I would probably treat it the same way hapi deals with a non-error thrown in a handler.

Copy link

@atrauzzi atrauzzi May 31, 2017

Yeah, can happen. Have seen strings thrown in the past.

Copy link
Contributor Author

@dwmkerr dwmkerr May 31, 2017

@hueniverse it is definitely non-conventional to reject a non-error object. If done deliberately it would definitely be an anti-pattern. However, there are cases where it will slip in by mistake, here's the most common scenario:

function doSomethingAsynchronous(input) {
  return new Promise((resolve, reject) => {
    callbackToLibrary(input, (err, result) => {
      if (err) return reject(err);  // uh-oh....
      return resolve(result);
    });
  });
}

In this case, if the library you are using does something goofy like callback with a non-error object, the promise will reject with the non-error object.

I think that with the handling of thrown exceptions in domains we have the same behaviour (because Boom.wrap covers whatever is thrown, ensuring that even if what is thrown is not an error, it gets wrapped in one).

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented May 30, 2017

Should this be the default behavior? Do people who use promises consider this a breaking change?

@hueniverse hueniverse self-assigned this May 30, 2017
@corbinu
Copy link

@corbinu corbinu commented May 30, 2017

Just one user here but I do use promises with hapi. I would like this as the default behavior but would consider it a breaking change :/

@nlf
Copy link
Member

@nlf nlf commented May 30, 2017

i'm curious why you would consider this a breaking change @corbinu. are you using a handler somewhere in the request lifecycle to intercept promise rejections somehow?

that's the only situation i can think of off the top of my head that would cause this patch to lead to undesirable results. i'm definitely all ears if i'm missing something, though.

@corbinu
Copy link

@corbinu corbinu commented May 30, 2017

I am not but assumed somebody might be you guys know your users better than I do though :)

@nlf
Copy link
Member

@nlf nlf commented May 30, 2017

just checking, thought maybe you had a specific use case in mind. i personally can't think of one, and unless someone else can IMO i'm not sure i'd call this a breaking change.

to me this is very similar to having handlers run through domains, the intent isn't to alter behavior, but to make sure that a response is sent even if an unexpected failure occurs. some sort of error handling plugin is the only thing i can think of this change would interfere with, and an error handling plugin that only catches rejected promises seems like the sort of thing a user would be happy to get rid of in favor of native support.

TL;DR - unless someone has a strong objection or legitimate use case for the existing behavior, i don't believe this is a breaking change and i do believe it's a good idea to enable by default

@devinivy
Copy link
Member

@devinivy devinivy commented May 30, 2017

I agree that this isn't a breaking change. If you're already catching your own rejections, that should still work just fine.

@dwmkerr
Copy link
Contributor Author

@dwmkerr dwmkerr commented May 31, 2017

@nlf that's essentially the idea, that we get a similar opportunity to cover unhandled rejections in the same way we can with uncaught exceptions in domains. In Node 8 domains will be aware of native unhandled rejections interestingly (which should make it easier to be resilient in this situation, even if the user doesn't return the promise there's a chance to catch it).

@devinivy yep, if you do:

  return doSomething().
    then((result) => reply(result))
    .catch((err) => console.log("Don't touch my error..."));
}

Then the extra catch I added will not be called, so if you already catch your own rejections, there is no change to behaviour.

@rubennorte
Copy link
Contributor

@rubennorte rubennorte commented May 31, 2017

In Node 8 domains will be aware of native unhandled rejections interestingly

@dwmkerr not exactly. It just executes the functions passed to then and catch with the proper domain, which wasn't happening before and forced users to use another Promise implementation with support for it.

@jeff-kilbride
Copy link
Contributor

@jeff-kilbride jeff-kilbride commented May 31, 2017

I was using generators with a different framework and am now using promises and async / await with Hapi. I would love to see better built-in support for unhandled rejections. I am currently using onPreStart in a plugin to walk through the handlers and manually attach a catch to any that return a promise:

module.exports = [{
    type: 'onPreStart',
    method: function (server, next) {

        // Iterate over all connections.
        const connections = server.table();
        for (const conn of connections) {

            // Iterate over all routes in each connection.
            for (const route of conn.table) {
                if (typeof route.settings.handler !== 'function')
                    continue;

                // Store the original handler function.
                const h = route.settings.handler;
                route.settings.handler = (request, reply) => {

                    // Call the original handler.
                    const p = h(request, reply);

                    // If the handler returns a promise, attach a `catch()` method.
                    p && p.catch && p.catch(reply);
                };
            }
        }

        next();
    }
}];

This isn't the best implementation, but it works. However, with this PR, it looks like this won't be necessary anymore. Correct? It looks like you're doing something similar, but at the framework level -- which is great. 👍

Just wondering if I'm missing anything or if I'll be able to get rid of the above code once this lands.

@dwmkerr
Copy link
Contributor Author

@dwmkerr dwmkerr commented Jun 2, 2017

@rubennorte thanks for the clarification!

@dwmkerr
Copy link
Contributor Author

@dwmkerr dwmkerr commented Jun 2, 2017

@jeff-kilbride Correct, you will no longer need the code shown, however there is one subtle caveat. With your code, if a handler does this:

function handler(request, reply) {
  return new Promise((reply, reject) => {
    throw "The cake is a lie";
  });
}

Your current work-around will lead to reply("The cake is a lie"). With the implementation I have added, Hapi will check whether the rejected value is an error, if it is not it will wrap it in a Boom object, meaning you essentially have: reply(Boom.wrap("The cake is a lie").

In short:

Before: reject('the cake is a lie') -> HTTP 200, with content 'the cake is a lie'
After: reject('the cake is a lie) -> HTTP 500, with hapi error content

This is to maintain parity with how hapi handles uncaught exceptions (wrapping them in errors).

@hueniverse hueniverse added this to the 16.3.2 milestone Jun 5, 2017
@hueniverse hueniverse merged commit 00d04fb into hapijs:master Jun 5, 2017
2 checks passed
hueniverse added a commit that referenced this issue Jun 5, 2017
@cur3n4
Copy link

@cur3n4 cur3n4 commented Jun 12, 2017

@jeff-kilbride I am doing something similar and it is working fine. The only difference is that my handler method is not static.
I had a similar issue where the hapi library being used was the one under glue and not the main one.

@jeff-kilbride
Copy link
Contributor

@jeff-kilbride jeff-kilbride commented Jun 12, 2017

@cur3n4 Thanks for the heads up! It looks like glue could be the issue. I added the server.version variable to my startup string and got:

Hapi server version 16.3.0 started at http://7b72193a91a0:4000

I have hapi as a dependency in my package.json file:

    "hapi": "^16.4.3",

How do I get glue to use the correct version? Were you able to fix this?

@cur3n4
Copy link

@cur3n4 cur3n4 commented Jun 12, 2017

In my package.json I have glue ^4.1.0. That did it for me.

@jeff-kilbride
Copy link
Contributor

@jeff-kilbride jeff-kilbride commented Jun 12, 2017

I found it. I needed to rebuild my docker containers after upgrading hapi. 🙄

Looks like everything is working as it should, now, without my extension! 🎉

@cur3n4 Thanks for the push in the right direction! I'm deleting my previous post, so it doesn't cause any confusion...

@dwmkerr
Copy link
Contributor Author

@dwmkerr dwmkerr commented Jun 19, 2017

@cur3n4 thanks for the pointers, I've been busy so haven't been able to get to this until now. @jeff-kilbride delighted it works for you and you can chop out some extra code! Doing the same in my project now 😄

@krazik
Copy link

@krazik krazik commented Aug 30, 2017

So I have Hapi 16.5.2 when is this going to be included?

@jeff-kilbride
Copy link
Contributor

@jeff-kilbride jeff-kilbride commented Aug 30, 2017

It's been included since 16.4.0.

@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet