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

Better support for async handlers #3429

Closed
rubennorte opened this issue Jan 23, 2017 · 22 comments
Closed

Better support for async handlers #3429

rubennorte opened this issue Jan 23, 2017 · 22 comments
Assignees
Labels
Milestone

Comments

@rubennorte
Copy link
Contributor

@rubennorte rubennorte commented Jan 23, 2017

Given that async functions are now standard it'd be great to have better support for them in Hapi. Specifically, using them as handlers:

server.route({
  method: 'GET',
  path: '/',
  handler: async function (request, reply) {
    const data = await getData();
    const other = await getOther(data);
    reply(other);
  }
});

We can use them now as I described but any error in the handler would become an unhandled promise rejection. Catching them in the framework instead of forcing the programmers wrap the code inside a try/catch would be nice. This is what I'd like to avoid:

server.route({
  method: 'GET',
  path: '/',
  handler: async function (request, reply) {
    try {
      const data = await getData();
      const other = await getOther(data);
      reply(other);
    } catch (error) {
      reply(error);
    }
  }
});

Would you accept a PR on this? Would you also use the result of the async function to call reply or only catch the errors?

Thanks for considering this.

@nlf
Copy link
Member

@nlf nlf commented Jan 23, 2017

i thought async function support was still behind the --harmony flag? if that's the case, anything like this is unlikely to be accepted until the flag is no longer required.

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 23, 2017

i thought async function support was still behind the --harmony flag?

It is.

@rubennorte
Copy link
Contributor Author

@rubennorte rubennorte commented Jan 23, 2017

Async functions will land in Node 8 without flags (they're available without them in V8 5.5, the version that's currently being used in Chrome stable).

I don't see a reason why this cannot be implemented now to be used with generators or transpiled code. Even with regular functions that return promises would be helpful:

server.route({
  method: 'GET',
  path: '/',
  handler: function (request, reply) {
    return getData().then(getOther).then(reply);
    // Or even: return getData().then(getOther);
  }
});

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jan 23, 2017

This would have to be done by me because I need to study this more.

@kanongil
Copy link
Contributor

@kanongil kanongil commented Jan 24, 2017

As far as I know, the only thing this request requires from hapi, is to support returning a Promise from a handler by calling reply() when they fail. This is somewhat similar to how a throw is already handled.

@rubennorte
Copy link
Contributor Author

@rubennorte rubennorte commented Jan 24, 2017

I reviewed the code and I think @kanongil is right. We just need to wrap the result of the handler as a promise and call reply with any errors it might throw.

@hueniverse good. I just want Hapi to support them no matter who implements it 😄

@devinivy
Copy link
Member

@devinivy devinivy commented Jan 24, 2017

While you wait it should be trivial to write a wrapper used like,

server.route({
  method: 'GET',
  path: '/',
  handler: Safely.catch(async function (request, reply) {
    const data = await getData();
    const other = await getOther(data);
    reply(other);
  })
});

@jeff-kilbride
Copy link
Contributor

@jeff-kilbride jeff-kilbride commented Feb 21, 2017

I'm trying to achieve the same basic thing, but I also wanted centralized error handling for all of my handlers without having to manually call some sort of utility error handling function. I'm new to Hapi and just wondering if I'm going about this the right way. I'm using what I have gleaned from looking at hapi-plugin-co to wrap all handlers using onPreStart. Also, I'm using haute-couture to wire my plugins. Here's what I have:

plugins/async-handlers/extensions.js:

'use strict';

const Boom = require('boom');


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

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

            // Iterate over all routes in each connection.
            for (let 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.
                    if (p && p.catch) {
                        p.catch((e) => {

                            // Ensure the error is a Boom object and capture a message
                            // for the logs.
                            let msg;
                            if (e instanceof Error) {
                                msg = e.stack;
                                if (!e.isBoom)
                                    e = Boom.wrap(e);
                            }
                            else {
                                msg = String(e);
                                e = Boom.create(500, msg);
                            }

                            // Log it.
                            request.log(['error', 'uncaught'], msg);

                            // Explicitly call the reply() method to handle the error.
                            reply(e);
                        });
                    }
                };
            }
        }

        next();
    }
}];

An example plugin with an async handler:

plugins/hello/routes.js:

'use strict';

const Boom = require('boom');


module.exports = [
    {
        method: 'GET',
        path: '/hello/{name?}',
        config: {
            handler: async function (request, reply) {
                const hello = await _asyncFunc(request.params.name);

                reply(hello);
            }
        }
    },
    {
        method: 'GET',
        path: '/goodbye/{name?}',
        config: {
            handler: function (request, reply) {
                const goodbye = `Good-bye ${request.params.name || 'World'}`;

                reply(goodbye);
            }
        }
    }
];


// Private methods.
function _asyncFunc(name) {
    return new Promise((resolve, reject) => {
        if (name === 'Joe')
            reject(Boom.badRequest(`Bad Parameter: ${name}`));
        else
            setTimeout(() => resolve(`Hello ${name || 'World'}`), 2000);
    });
}

I'm using Glue to compose the server and add the plugins, and I'm currently just running it through babel-register with the following in my .babelrc:

{
  "presets": [
    ["env", {
      "targets": {
        "node": "current"
      }
    }]
  ]
}

This seems to be working. When I hit the /hello route with anything but Joe, the server waits two seconds and then responds. When I hit it with Joe, I get:

Debug: handler, error 
    {"msec":2.306112051010132,"error":"Bad Parameter: Joe","data":{"data":null,"isBoom":true,"isServer":false,"output":{"statusCode":400,"payload":{"statusCode":400,"error":"Bad Request","message":"Bad Parameter: Joe"},"headers":{}}}}

in the logs and the correct error response in the browser. I added the /goodbye route just to make sure it was working with non-async functions. However, I would like some feedback. I'm wondering if there is a better (more Hapi or more idiomatic Javascript) way to accomplish this. If somebody can confirm it works the way I think it does, maybe it will help others who are looking to use async...await syntax in their handlers.

(I apologize for the length...)

-- EDIT --

I've cleaned it up quite a bit over the last few days, as I've looked at more examples. (specifically the hapi-es7-async-handler module) I was able to remove the bluebird dependency after I realized that wrapping an async function in Promise.resolve was redundant. The above is working well for me, now. The thing I like most is having a single spot to customize / standardize any errors thrown by my async handlers.

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Feb 21, 2017

Based on this conversation I made a little plugin to handle this issue, but I saw there are already plugins for that when I was going to upload on npm, so I'm just leave my silly code and the npm one. Hope this help you guys :)

@cjnqt
Copy link

@cjnqt cjnqt commented Feb 28, 2017

An async function always return a promise. So making Hapi handlers support async functions === making Hapi handlers support Promise-returning functions.

@jeff-kilbride
Copy link
Contributor

@jeff-kilbride jeff-kilbride commented Feb 28, 2017

@cjnqt Yeah, I'm learning as I go. I've primarily been using generators / yield and bluebird's coroutine for handling async stuff up to now. This is my first foray into using the async / await syntax with babel. I've edited my example above to make the Promise handling less redundant.

@rubennorte
Copy link
Contributor Author

@rubennorte rubennorte commented Feb 28, 2017

Just for you to know, async-await has officially landed in Node.js 7.6.0.

@devinivy
Copy link
Member

@devinivy devinivy commented Mar 3, 2017

@jeff-kilbride it looks pretty good to me. One concern– I'd avoid modifying the route public interface at runtime (I realize that is the approach of hapi-plugin-co). While it seems to work right now for handlers, my impression is that there's no hard guarantee it will continue to work that way in the future. I'd opt to instead create a handler type with server.handler().

Also a question! Is there a reason you'd like to centralize error handling specifically for async handlers? If you're looking for a more general-purpose solution, centralized error handling in hapi is typically done via a onPreResponse request extension. Happy to elaborate on that in hapijs/discuss but the docs also have a nice section on this.

@jeff-kilbride
Copy link
Contributor

@jeff-kilbride jeff-kilbride commented Mar 5, 2017

@devinivy thanks for the comments. i've read the server.handler() docs and am aware of how they work. i will take another look at that.

As far as the error stuff, i guess it would be better phrased as centralized error catching rather than error handling. Using generators in express, i was doing a lot of try...catch stuff inside my handler code. So, i would have:

foobar: co(function *(req, res) {
    try {
        [... handler code ...]
    }
    catch(e) {
        handleError(res, e);
    }
})

With the setup above, i don't need the try...catch statements in the handlers anymore. I still use onPreResponse to convert some errors to html responses, etc...

@devinivy
Copy link
Member

@devinivy devinivy commented Mar 10, 2017

I've published a hapi multi-tool that includes a helper for async handlers. If you're interested– https://github.com/devinivy/toys/blob/master/API.md#toyshandlerasynchandler

@rubennorte
Copy link
Contributor Author

@rubennorte rubennorte commented Mar 18, 2017

This would have to be done by me because I need to study this more.

@hueniverse could you study this? If you are busy maybe you can define the strategy Hapi should follow on this so anyone can do a PR to implement it.

@dwmkerr
Copy link
Contributor

@dwmkerr dwmkerr commented May 29, 2017

@rubennorte I think a PR I opened (#3486) would provide most of what you need. Specifically:

  1. It allows you to do this: #3429 (comment) safely
  2. It covers @kanongil's thoughts on rejections #3429 (comment)

It's a fairly small change to the API, it essentially just means that if you return a promise in a handler, hapi will give you a safety net by ensuring any unhandled promise rejection is wrapped in a boom object and returned to the caller.

@ebickle
Copy link

@ebickle ebickle commented May 31, 2017

Now that Node.js 8.0 is out, the priority of this has likely gone up.

I started as an developer working with Hapi on Node 7.10 (and now 8.0) building out a prototype enterprise app. With Hapi's built-in support for promises I fully expected that Hapi would have registered a continuation on the end of any promise from a route handler function and "do the right thing".

Right now, the API surface is uneven:

  • Throwing an error from a non-async route handler function is caught by Hapi and converted into an HTTP 500.
  • Throwing an error from an async route handler function is not caught by Hapi and results in an unhandled promise error. (Crashes process in future Node.js versions).
  • Passing a promise that resolves to an error to reply() is caught by Hapi and converted into an HTTP 500.
  • Returning a promise that resolves to an error from a route handler function is not caught by Hapi and results in an unhandled promise error. (Crashes process in future Node.js versions).

It's unintuitive, regardless of async/await usage. Hapi is already catching errors; catching promise rejections returned from handler functions isn't much different and fits with the way promises work across the rest of the API surface area.

@devinivy
Copy link
Member

@devinivy devinivy commented May 31, 2017

@ebickle This is being handled actively in #3486. While domains are deprecated, there's also not another solution currently available– they are knowingly used by the framework (despite their deprecation), and you can also configure hapi to not use them if so desired. Once #3486 lands I think we'll be up to speed– does that sound right to you?

@ebickle
Copy link

@ebickle ebickle commented May 31, 2017

@devinivy Sounds right to me, thanks!

@dwmkerr
Copy link
Contributor

@dwmkerr dwmkerr commented Jun 1, 2017

@ebickle Yes I'm working on PR #3486 to try and give some level of cover (thanks for sharing the input @devinivy!), but am super interested in helping to make sure that Hapi be as robust as possible in this regard! Ideally with the PR we cover some of the easy error handling cases, but there are still plenty of edge cases which can trip people up (which is not a Hapi issue as much as just the general complexity of dealing with error scenarios in async code)

@hueniverse hueniverse added this to the 16.4.0 milestone Jun 5, 2017
@hueniverse hueniverse self-assigned this Jun 5, 2017
@hueniverse hueniverse closed this Jun 5, 2017
@masmau
Copy link

@masmau masmau commented Sep 20, 2017

Is it correct/possible to have the same behaviour with prerequisites?

Tested with v.16.6.0:

  • async handler without try...catch: returns a 500 status code
  • async pre without try...catch: unhandled promise rejection

Thanks

@Marsup Marsup added feature and removed request labels Sep 20, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 21, 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