Skip to content

Async/await basic support #1837

Closed
Closed
@ghermeto

Description

@ghermeto
Member

Discussion: async/await basic API

What?

Add support to async functions as middleware (.pre() and .use()) and route handlers. Keeps the first hook synchronous.

Example

const restify = require('restify');

const server = restify.createServer({});

server.use(async (req, res) => {
  req.something = await doSomethingAsync();
});

server.get('/params', async (req, res) => {
  const value = await asyncOperation(req.something);
   res.send(value);
});

Middleware API

Restify implementation uses Chains for both .pre() and .use() middleware, which allows a consistent experience. All middleware right now must call next() or next(err). The proposed interface for the middleware function is:

  • fn.length === 2 (arity 2);
  • fn instanceof AsyncFunction;
  • If the async function resolves, it calls next();
  • the value resolved will be discarded;
  • if it rejects we call next(err) [error handling discussed in a section below];

Route handler API

Restify also uses Chains for both route handlers, which allows being very consistent. Right now, Restify treats route handlers just like middleware and forces the user to call next().

  • fn.length === 2 (arity 2);
  • fn instanceof AsyncFunction;
  • If the async function resolves, it calls next();
  • the value resolved will be discarded;
  • user must call res.send to send the body;
  • if it rejects we call next(err) [error handling discussed in a section below];

However, we also have the opportunity, if we want to, to improve DevEx and allow the value resolved of the promise to be sent as the body. It causes some challenges in Fastify, but there are mitigations we can apply.

For instance, we can check, inside the resolve handler for the async function, if the response has been flushed and if not we flush the response with whatever value the user returns. This approach has the drawback that if the user uses both, whatever he returns from the promise will be discarded.

Besides that, there is the question of what should happen if the user calls:

server.get('/params', async (req, res) => {
  setImmediate(() => {
      res.send(value);
  });
});

Should we wait for res.send and risk it never be called? Or should immediately call next() on resolution and let the timers being slightly off?

I propose we deal with this particular use case with documentation and user education.

If you have a callback context you should use a callback and if you have a promise or async/await context you should use the async handler.

Alternatively, we could add a new API to Response .sent which is a getter for a promise that resolves when the response is sent. This allows us to use a similar approach to Fastify:

server.get('/params', async (req, res) => {
  setImmediate(() => {
      res.send(value);
  });
  await res.sent;
});

Error handling

As of right now, Restify allows users to throw non-errors and wraps it around an InternalError after emitting the error events.

I propose that we keep the same behavior for the async handler, going through the same code path. The only exception is when the user returns the async function with a rejected promise without any value. For example:

server.get('/params', async (req, res) => {
  return Promise.reject();
});

We can't prevent users from doing that. That is a valid Javascript code, even though it is not very useful. What we can do it if the rejection value is falsy we can create an AsyncHandlerRejection VError, which will allow the user a better debugging experience.

PR

#1833

Activity

ghermeto

ghermeto commented on Jul 1, 2020

@ghermeto
MemberAuthor

A small update. After @hekike review, I noticed the error handling is inconsistent with the callback behavior. For instance, using callbacks we can re-route the request if we pass a string to next():

const restify = require('restify');
const server = restify.createServer({});

server.use(restify.plugins.queryParser({mapParams:true}));

server.get('/route', (req, res, next) => {
   next('getresult');
});

server.get('/string', (req, res, next) => {
   next('invalid');
});

server.get('/number', (req, res, next) => {
   next(1);
});

server.get('/error', (req, res, next) => {
   next(new Error(''));
});

server.get('/result', (req, res, next) => {
   res.send(req.query);
   next();
});

server.listen(8081, 'localhost', function () {
   console.info('server started');
});

The result of curl http://localhost:8081/error?id=1 is:

{"code":"Internal","message":"Error"}

The result of curl http://localhost:8081/number?id=1 is:

{"code":"Internal","message":"1"}

The result of curl http://localhost:8081/string?id=1 is:

{"code":"Internal","message":"RouteMissingError: Route by name doesn't exist"}

The result of curl http://localhost:8081/route?id=1 is:

{"id":"1"}

Now, it doesn't make sense we try to re-route the request on a rejected promise and, at the same time, we can't prevent users to reject the async function without giving an error object back, so I believe the path forward is to wrap any value rejected that is not an instance of Error with an AsyncHandlerRejection error.

It opens the question... do we want to be able able to re-route using async/await when the promise is resolved? We could check if the value resolved is a string, but this will lead to confusion since it is not what other frameworks are doing. I believe if we want to have re-routing with async/await we need to find a more explicit way moving forward.

ghermeto

ghermeto commented on Jul 1, 2020

@ghermeto
MemberAuthor

Open questions:

1) What do to with the value resolved (returned) by the async function?

The options are:

  1. always discard the value;
  2. send it as the body;
  3. use to re-route to a different handler;

1. always discard the value

Users will still have to use res.send() to send the message body.

PROS:

  • It is the safest choice, providing a consistent and predictable experience;

CONS:

  • The lost opportunity of a better DevEx;
  • we will have to find an alternative to re-route (if it is indeed needed);

2. send it as the body

Users can use res.send() to send the message body or they can return a value on the async function.

PROS:

  • Better UX experience;
  • Consistent with experiences in other frameworks;

CONS:

  • Possible race condition if the user tries to call res.send (in an async callback context) and also return a value;
  • We will have to find an alternative to re-route (if it is indeed needed);

3. use to re-route to a different handler

Users still use res.send() to send the message body if they return a string, we try to match to a different route handler.

PROS:

  • Allow to re-route requests;

CONS:

  • Unexpected and deviates from the ecosystem;
  • Error-prone. What happens if something else that is not a string is returned?

2) How to handle when the user calls res.send() in the context of an async callback inside an async function?

For example:

server.get('/params', async (req, res) => {
  setImmediate(() => {
      res.send(value);
  });
  await res.sent;
});

The options are:

  1. we document the issue;
  2. resolve a promise when the response is sent;

1. document approach

we document the use case and recommend users to convert callbacks into promises (outside the async function) or to use the callback alternative instead of async function;

PROS

  • less effort;
  • keeps a clean separation of callbacks and promises (async/await);

CONS

  • can cause hard to debug errors if users don't follow recommendations;

2. resolve a promise when the response is sent

We implement a promise in the Response that resolves when the response is sent and users can wait for it:

server.get('/params', async (req, res) => {
  setImmediate(() => {
      res.send(value);
  });
  await res.sent;
});

PROS

  • better experience;
  • same approach as Fastify (consistent with ecosystem);

CONS

  • can cause hard to debug errors if users don't wait;
ghermeto

ghermeto commented on Jul 1, 2020

@ghermeto
MemberAuthor
hekike

hekike commented on Jul 1, 2020

@hekike
Member
  1. I'd warn log the return value then discard. It's a non-supported feature.
  2. I wouldn't put too much effort into it and go with documentation
DonutEspresso

DonutEspresso commented on Jul 2, 2020

@DonutEspresso
Member

I'm copying my response over from the original PR, but my belief is - next is the contract we support today, and async await is sugar on top of that. My preference is that we formalize the next() contract (if we haven't already?), and the promise pathways that lead up to that. Essentially, there's three ways to cede control back to restify through next today, and this PR adds a variation for each of those contracts:

next()
    With async functions, a promise that resolves no value
next('fooRoute')
    With async functions, a promise that resolves 'fooRoute'
next(new Error('boom'))
    With async functions, a promise that rejects with an Error object

I think there's a good argument to be made that anything that is not one of those three should cause a runtime exception. But I feel less strongly than I do about at least formalizing and documenting it.

DonutEspresso

DonutEspresso commented on Jul 2, 2020

@DonutEspresso
Member

I was also thinking about this scenario:

server.get('/params', async (req, res) => {
  setImmediate(() => {
      res.send(value);
  });
});

Is that actually any different than this:

server.get('/params', (req, res next) => {
  setImmediate(() => {
      res.send(value);
  });
  return next();
});

It's the same footgun, just in promise form.

ghermeto

ghermeto commented on Jul 2, 2020

@ghermeto
MemberAuthor

@DonutEspresso from your comment on the PR:

Yeah, that makes sense. My concern is that this opens up a new footgun with promise usage. The following handler will just hang forever:

function(req, res) {
return FuncThatWasSupposedToReturnPromiseButDidnt();
}

I haven't used promises enough to know what's the best way to handle this. Is it common to do runtime assertions on thenables? I'd much rather the server blew up than fail silently. One way to enforce this is by saying async functions should only have an arity of 2, and we don't pass next to those functions.

The same issue also happens right now. This will hang forever:

server.get('/err', (req, res) => {
   return 1;
});

Currently, there is no arity check. We could, do like Fastify and check the arity and fn.constructor.name === 'AnsycFunction' when the user is registering the handler. This way, he will be forced to add an async function if he just uses req and res on his function.

But we can't prevent them to do this:

server.get('/err', async (req, res) => {
   return Promise.resolve('foo');
});

Thoughts?

DonutEspresso

DonutEspresso commented on Jul 2, 2020

@DonutEspresso
Member

The same issue also happens right now. This will hang forever:

Ugh, good point.

Currently, there is no arity check. We could, do like Fastify and check the arity and fn.constructor.name === 'AnsycFunction' when the user is registering the handler. This way, he will be forced to add an async function if he just uses req and res on his function.
What are the downsides to that approach? Normal functions that return promises won't be supported? If that reduces complexity could be worthwhile.

But we can't prevent them to do this:

server.get('/err', async (req, res) => {
   return Promise.resolve('foo');
});

I stared at this for a long time but don't know what's wrong with it. What am I missing? 😅

ghermeto

ghermeto commented on Jul 2, 2020

@ghermeto
MemberAuthor

I'm copying my response over from the original PR, but my belief is - next is the contract we support today, and async await is sugar on top of that. My preference is that we formalize the next() contract (if we haven't already?), and the promise pathways that lead up to that. Essentially, there's three ways to cede control back to restify through next today, and this PR adds a variation for each of those contracts:

next()
    With async functions, a promise that resolves no value
next('fooRoute')
    With async functions, a promise that resolves 'fooRoute'
next(new Error('boom'))
    With async functions, a promise that rejects with an Error object

I think there's a good argument to be made that anything that is not one of those three should cause a runtime exception. But I feel less strongly than I do about at least formalizing and documenting it.

Ok, that was one of my open questions above. What to do with the resolved value. We can certainly use to re-route (was one of the options). And we need to be sure that is the API we want. @cprussin added a comment that he would like to see the re-routing deprecated. I think on an async/await context, using the return value for re-routing will be unexpected and might lead confusion since other frameworks use the return value to send the body.

Is deprecating the re-routing an option?

ghermeto

ghermeto commented on Jul 2, 2020

@ghermeto
MemberAuthor

I stared at this for a long time but don't know what's wrong with it. What am I missing? 😅

Technically nothing, but this:

async function () {
  return Promise.resolve();
}

and

async function () {
  await Promise.resolve();
  return; 
}

are essentially different because the first one will execute completely in the same clock turn, at the ends resolving to a promise, which is "essentially the same" as:

function () {
  return Promise.resolve();
}

On the second, the function execution will pause on the await and resume after the promise is resolved. The power of async functions is on await. If you are not awaiting anything, you would be better off just using a regular function that returns a promise.

ghermeto

ghermeto commented on Jul 2, 2020

@ghermeto
MemberAuthor

I was also thinking about this scenario:

server.get('/params', async (req, res) => {
  setImmediate(() => {
      res.send(value);
  });
});

Is that actually any different than this:

server.get('/params', (req, res next) => {
  setImmediate(() => {
      res.send(value);
  });
  return next();
});

It's the same footgun, just in promise form.

Yes. It would work exactly as it is right now (just in a promise form).

However, I have to say that more I think about it, more I like the rise to resolve a promise when the response is sent:

server.get('/params', async (req, res) => {
  setImmediate(() => {
      res.send(value);
  });
  await res.sent;
});

it gives an elegant solution for this problem.

DonutEspresso

DonutEspresso commented on Jul 2, 2020

@DonutEspresso
Member

Thanks. This is all really good context. Why are promises so hard 😄

I'd suggest we agree on the scope of the effort here so we aren't trying to solve all the problems at once. That's probably why I started by thinking of this effort as a promisified version of the existing next API. There are some concerns with the next API that aren't new (e.g., string based routing via next); if we think that's a problem I'd suggest we get alignment on whether we should address it in a separate PR, and land it first so that this PR doesn't have to deal with it.

Then there are the footguns. There's a lot of interesting ideas being proposed here to tackle those! While we can address some of the promise footguns in a meaningful way that we couldn't with the callback footguns, I'm interested in hearing the group's take on whether or not we need to address those now.

I personally think with some time and operational experience under our belt we can make a more informed decision later down the road. I'm empathetic to the desire to deliver a great devex, but I see this as a MVP, it gets us going while giving us time to learn and think through the implications of a diverging feature sets between the callback and promise experiences. Thoughts?

ghermeto

ghermeto commented on Jul 2, 2020

@ghermeto
MemberAuthor

I can get behind this idea. Just because we merged the PR doesn't need we must release it, so we can continue these discussions on a different issue.

So, just to confirm:

  1. add arity check for AsyncFunction: check if fn.length === 2 && fn.contructor.name === 'AsyncFuntion' and fn.length === 3 && fn.contructor.name !== 'AsyncFuntion'.
  2. if async function resolves to a string, try to re-route;
  3. if async function resolves to something different then string let if fall the same behavior as of right now (InternalError);
  4. If rejected with Error, use the error class provided;
  5. if rejected with something else, wrap into AsyncHandlerError;

We keep discussing:

  1. deprecating re-routing with string for both callback and async;
  2. resolve a promise when the response is sent (Response.sent);

PS: I think my async/await example above was flawed because in the example the promise resolves immediately. The only difference there was that one will suspend the function and restore it on the next turn, and the other doesn't, but the scheduling is probably the same... ¯\_(ツ)_/¯

cprussin

cprussin commented on Jul 2, 2020

@cprussin
Contributor

If you are not awaiting anything, you would be better off just using a regular function that returns a promise.

@ghermeto maybe this is splitting hairs over semantics but just wanted to point out that I've found it very common (and pleasant) to mark functions that simply return a promise as async just to add clarity around the contract of those functions, e.g.;

const myFunction = async () => doSomething();

adds some clarity over:

const myFunction = () => doSomething();

Even though the two are functionally identical (assuming doSomething returns a promise), the former provides added clarity about what myFunction returns.

It's poor man's types ;)

cprussin

cprussin commented on Jul 2, 2020

@cprussin
Contributor

deprecating re-routing with string for both callback and async;

Yes please! IMHO this has got to be one of the nastiest surprises in restify and I have yet to find a legitimate use case for it (that is, one that isn't simply enabling poorly structured code)

ghermeto

ghermeto commented on Jul 2, 2020

@ghermeto
MemberAuthor

Yes please! IMHO this has got to be one of the nastiest surprises in restify and I have yet to find a legitimate use case for it (that is, one that isn't simply enabling poorly structured code)

I agree. I didn't even remember that was a thing and we will keep this discussion going. I don't think anyone uses it. To keep this PR manageable and moving forward, I agree with @DonutEspresso assessment we should treat this PR as an MVP and keep it consistent with the current API.

I updated #1833 to the following contract:

  • add arity check for AsyncFunction: check if fn.length === 2 && fn.contructor.name === 'AsyncFuntion' and fn.length === 3 && fn.contructor.name !== 'AsyncFuntion'.
  • if async function resolves to a string, try to re-route;
  • if async function resolves to something different then string let if fall the same behavior as of right now (InternalError);
  • If rejected with Error, use the error class provided;
  • if rejected with something else, wrap into AsyncHandlerError;

and added the migration guide @hekike requested.

PS: PR builds currently fail on #1833 because master is broken (unrelated)

self-assigned this
on Jul 2, 2020
mmarchini

mmarchini commented on Jul 7, 2020

@mmarchini
Contributor

However, we also have the opportunity, if we want to, to improve DevEx and allow the value resolved of the promise to be sent as the body. It causes some challenges in Fastify, but there are mitigations we can apply.

I think the DevEx that Fastify provides is quite good, and the risk of misuse seems similar to forgetting to call next(). I'd go further and say that if we had to choose between letting users use req.send and the resolved value on async functions/functions returning Promises, I'd choose the resolved value, as it is more idiomatic with how Promises are used in the ecosystem. I also like req.sent being a Promise that is resolved when req.send is called though.

It opens the question... do we want to be able able to re-route using async/await when the promise is resolved? We could check if the value resolved is a string, but this will lead to confusion since it is not what other frameworks are doing. I believe if we want to have re-routing with async/await we need to find a more explicit way moving forward

I'd be +1 on not including rerouting with async/await, and deprecating the rerouting feature on 9.x. If rerouting is still desirable, maybe we should consider a more explicit API like:

server.get('/params', async (req, res) => {
  return async req.callOtherRoute('/some-other-route', res);
});

This would still allow rerouting, but would also allow users to call other routes if necessary without actually rerouting. Just a suggestion, and something we can think after async/await support lands.

How to handle when the user calls res.send() in the context of an async callback inside an async function?

We can probably identify during runtime if send and return are called for a single request. IMO it should throw an error/warn with enough information to make debugging easy.

We can't prevent users from doing that. That is a valid Javascript code, even though it is not very useful. What we can do it if the rejection value is falsy we can create an AsyncHandlerRejection VError, which will allow the user a better debugging experience.

While I like this, it's worth noting that the stack trace in this case won't show the handler.

Why are promises so hard 😄

I feel like Promises get harder the more we try to fit them on callback-based design decisions 😄. They can work reasonably well and are easier to understand if used on Promise-based code design.

ghermeto

ghermeto commented on Jul 7, 2020

@ghermeto
MemberAuthor

I'd be +1 on not including rerouting with async/await, and deprecating the rerouting feature on 9.x. If rerouting is still desirable, maybe we should consider a more explicit API like:

server.get('/params', async (req, res) => {
  return async req.callOtherRoute('/some-other-route', res);
});

This would still allow rerouting, but would also allow users to call other routes if necessary without actually rerouting. Just a suggestion, and something we can think after async/await support lands.

I love it and I'm not sure it needs to be an async function. The function could only set a redirection flag. This way it would work for both async and callback handler styles. What do you think?

We can probably identify during runtime if send and return are called for a single request. IMO it should throw an error/warn with enough information to make debugging easy.

I like this idea.

While I like this, it's worth noting that the stack trace in this case won't show the handler.

It doesn't, but if the async function is named, the name of the function will be available on the jse_info field of the error response (which is something)...

I feel like Promises get harder the more we try to fit them on callback-based design decisions 😄. They can work reasonably well and are easier to understand if used on Promise-based code design.

true

mmarchini

mmarchini commented on Jul 7, 2020

@mmarchini
Contributor

It doesn't, but if the async function is named, the name of the function will be available on the jse_info field of the error response (which is something)...

Can we also add the route and HTTP method? If the function is not named this would help identify where the rejection came from.

ghermeto

ghermeto commented on Jul 7, 2020

@ghermeto
MemberAuthor

Can we also add the route and HTTP method? If the function is not named this would help identify where the rejection came from.

I can certainly try 😉...

ghermeto

ghermeto commented on Jul 7, 2020

@ghermeto
MemberAuthor

done! 🎉🎉

@mmarchini this is the jse_info looks now:

{
      info: {
          cause: error,
          handler: handler._name,
          method: req.method,
          path: req.path ? req.path() : undefined
      }
}

right now the path function is always defined, but I decided to code defensively and check for the function.

It turned out pretty nice this way because even if the user doesn't provide a named function for the handler, Restify will name the function (like pre-0, pre-1, etc). The combination of the name, route, and method will make it pretty easy to find the rejection.

ghermeto

ghermeto commented on Jul 10, 2020

@ghermeto
MemberAuthor

PR #1833 was merged. I will close this ticket and open a new one to discuss the deprecation of the re-routing through a string parameter for both callback and async models.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

    Development

    No branches or pull requests

      Participants

      @ghermeto@cprussin@retrohacker@m5m1th@hekike

      Issue actions

        Async/await basic support · Issue #1837 · restify/node-restify