9.X - async/await #1833

ghermeto · 2020-06-27T10:16:49Z

Pre-Submission Checklist

Ran the linter and tests via make prepush
Included comprehensive and convincing tests for changes

Changes

What does this PR do?

This PR has two commits:

drops support to node 8.x (EOL);
adds support for async/await functions on pre, use and route handlers;

Example of async handling:

    const restify = require('restify');
    const server = restify.createServer();
    
    server.pre(async function (req, res) {
        await something();
        doSomethingElse();
    });

    server.get('/async', async function (req, res) {
        const body = await thatThing();
        res.send(body);
    });

Basically, the Chain call will check if the handler return is a Promise (by checking the existence of .then, as Falcor Router does it) and resolves by calling next or rejects by calling next with error.

.

ghermeto · 2020-06-27T10:30:17Z

cc/ @josephharrington

ghermeto · 2020-06-28T01:29:37Z

ops... forgot to add examples on the docs (and build the docs)... just added.

ghermeto · 2020-06-28T02:29:18Z

doh! the 3rd commit with async/await docs made this PR diff looks huge... if you can go into each individual commit to add your comments, I appreciate :)

ghermeto · 2020-06-28T06:12:11Z

cc /@cprussin

DonutEspresso · 2020-07-01T01:28:47Z

Nit: For future, would recommend separate, smaller PRs for easier reviewing/accounting. There's a lot going on here (style changes, docs, Node 8.x EOL, etc) and its a bit hard to parse it all at once.

A few questions that might influence the direction of the PR:

Do we plan on supporting regular functions that return promises? Depending on the answer may have further implications to discuss, it doesn't look like we make a distinction at the moment.
Do we plan on supporting mixed mode middleware? e.g., some callback, some async/await. I think the answer is yes, but good to be explicit.
I think the rejection contract could benefit from a bit more clarity/discussion. I want to say we should only allow rejecting with Error objects, but we can't enforce that at server startup. What are some of the ecosystem best practices here?
Async/await is likely to introduce some race conditions. e.g., imagine a res.write call in a setInterval loop. It looks like fastify has some clear rules around this to ensure things don't go off the rails. Anything that applies to us or that we can learn from?

ghermeto · 2020-07-01T03:10:25Z

Nit: For future, would recommend separate, smaller PRs for easier reviewing/accounting. There's a lot going on here (style changes, docs, Node 8.x EOL, etc) and its a bit hard to parse it all at once.

Yes, I broke them into different commits, but you are right. After I added the documentation commit I realized it should have waited. I won't send multiple commits next time.

A few questions that might influence the direction of the PR:

Do we plan on supporting regular functions that return promises? Depending on the answer may have further implications to discuss, it doesn't look like we make a distinction at the moment.

I followed the ecosystem. I guess we could check handler.contructor.name === 'AsyncFunction', but it is not recommended. For instance, fastify only checks the constructor name to verify the arity of the handler when inserted, but later they just do this same check for then. I wouldn't oppose check for the constructor & arity, but I believe is unnecessary, since the result must be treated like any Promise.

Do we plan on supporting mixed mode middleware? e.g., some callback, some async/await. I think the answer is yes, but good to be explicit.

yes, this allows mixed mode. Like the other frameworks that support both models.

I think the rejection contract could benefit from a bit more clarity/discussion. I want to say we should only allow rejecting with Error objects, but we can't enforce that at server startup. What are some of the ecosystem best practices here?

I used the same approach as fastify. You are right, we can't enforce it and it deviates from the current behavior. I don't think changing the error handling behavior should be coupled with this PR. Right now, an async error will be treated exactly like a callback error.

What I can do is check for if err instanceof Error or err instanceof VError and, if not, I can force wrap it under an AsyncHandlerRejection (or AsyncHandlerError), but that will differ from the callback behavior that allows user to throw anything.

Async/await is likely to introduce some race conditions. e.g., imagine a res.write call in a setInterval loop. It looks like fastify has some clear rules around this to ensure things don't go off the rails. Anything that applies to us or that we can learn from?

The biggest problem with fastify happens because they allow the user to both return the function with the body or use reply.send. It provides a nicer DevEx but comes with trade-offs. You can't do this in restify. You still need to call res.send, but the setInterval is an interesting case.

We should recommend using async/await syntax on a promise or async/await context. If you use any code that relies on callbacks and still want to use async handlers, you should convert these to promises outside the handler. Or better yet, avoid using async handlers. I guess that is something to append to the documentation. Maybe on a follow-up PR to add more docs around it?

ghermeto · 2020-07-01T06:13:25Z

@DonutEspresso Started issue #1837 to discuss this further.

hekike · 2020-07-01T15:05:15Z

docs/_api/formatters.md

@@ -50,7 +50,7 @@ Format a response for being sent over the wire

 Type: [Function][9]

-**Parameters**
+#### Parameters


Could you please generate docs as a separate commit outside of this PR?

hekike · 2020-07-01T15:06:28Z

lib/chain.js

+                    name: 'AsyncHandlerRejection',
+                    info: { handler: handler._name }
+                },
+                `Async middleware ${handler._name} rejected without an error`


Please avoid dynamic input in messages.

hekike · 2020-07-01T15:09:03Z

lib/chain.js

+                `Async middleware ${handler._name} rejected without an error`
+            );
+        }
+        next(error);


nit: add return to be consistent and to highlight that this fn ends here

hekike · 2020-07-01T15:10:00Z

lib/chain.js

+                `Async middleware ${handler._name} rejected without an error`
+            );
+        }
+        next(error);


I think it's expected with async-await but this will catch syntax errors etc.
Maybe explain in the 9.x migration guide.

Should the migration guide be part of a separate PR, with the docs?

hekike · 2020-07-01T15:10:09Z

lib/chain.js

+            error = new errors.VError(
+                {
+                    name: 'AsyncHandlerRejection',
+                    info: { handler: handler._name }


Please add cause

what should add here as a cause? This error is created whenever the user rejects an async function without passing it anything:

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

Do you have suggestions for cause?

You know... just got a mistake in my code... I wasn't wrapping synthetic errors because I thought Restify would throw anything, but actually, if the value passed to next() is a string, it will try to route it, for instance:

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"}

I will update the code to always wrap rejections and update #1837 .

FWIW trying to route on next with a string is IMHO an awful barnacle of Restify, it's worth considering deprecating it--it's essentially a GOTO with all the pain that comes along.

wrapping all non-errors in AsyncHandlerError actually turned out pretty nice, because if nothing is specified, the cause is undefined.

and I just discovered cause must be an instance of Error, so I'm moving the cause to inside the info.

hekike · 2020-07-01T15:12:37Z

package.json

@@ -90,7 +90,7 @@
    "report-latency": "./bin/report-latency"
  },
  "engines": {
-    "node": ">=8.3.0"
+    "node": ">=10.21.0"


Please add a migration guide like 6to7 and highlight Node version bump.

Should the migration guide be a separate PR?

yes, please. or you can add related pieces in this PR

hekike · 2020-07-01T15:13:02Z

package.json

@@ -90,7 +90,7 @@
    "report-latency": "./bin/report-latency"
  },
  "engines": {
-    "node": ">=8.3.0"
+    "node": ">=10.21.0"


I think we need to change Travis CI minimum node test version as well

hekike · 2020-07-01T15:16:42Z

lib/chain.js

+        if (!error) {
+            error = new errors.VError(
+                {
+                    name: 'AsyncHandlerRejection',


Please define new error types in: https://github.com/restify/node-restify/blob/master/lib/errorTypes.js

mmarchini · 2020-07-01T18:21:15Z

@ghermeto if you want some help I can break this PR in three and force-push only one commit here. It might be easier for folks to review.

ghermeto · 2020-07-01T22:45:38Z

thank you @mmarchini. I force pushed removing the generated docs and added some changes Peter requested.
I kept the jsdoc updates as it made sense to be on the same PR, now I'm missing the migration guide.

DonutEspresso · 2020-07-02T01:58:57Z

I followed the ecosystem. I guess we could check handler.contructor.name === 'AsyncFunction', but it is not recommended.

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.

I think the rejection contract could benefit from a bit more clarity/discussion. I want to say we should only allow rejecting with Error objects, but we can't enforce that at server startup. What are some of the ecosystem best practices here?

I used the same approach as fastify. You are right, we can't enforce it and it deviates from the current behavior. I don't think changing the error handling behavior should be coupled with this PR. Right now, an async error will be treated exactly like a callback error.

What I can do is check for if err instanceof Error or err instanceof VError and, if not, I can force wrap it under an AsyncHandlerRejection (or AsyncHandlerError), but that will differ from the callback behavior that allows user to throw anything.

Sorry! I wasn't very clear, I was suggesting that we formalize the next() contract, 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 · 2020-07-02T02:06:25Z

What I can do is check for if err instanceof Error or err instanceof VError and, if not, I can force wrap it under an AsyncHandlerRejection (or AsyncHandlerError), but that will differ from the callback behavior that allows user to throw anything.

What do you mean by callback behavior that allows user to throw anything?

DonutEspresso · 2020-07-02T02:08:57Z

Oops, sorry, maybe I should have left my comments in #1837!

ghermeto · 2020-07-02T02:16:24Z

I will reply to your comments on #1837

retrohacker

🎉 looks good!

daviande · 2020-07-06T22:25:31Z

docs/guides/8to9guide.md

+
+### Drops support for Node.js `8.x`
+
+Restify requires Node.js version `>=10.21.0`. 


Is there anything in particular for 10.21.0 instead of just 10.x?

I guess not... I will update it

mmarchini

Overall LGTM (I didn't review tests yet), although I'm still processing the need to call req.send and a few other things. I'll leave design-related comments in the issue.

What I can do is check for if err instanceof Error or err instanceof VError and, if not, I can force wrap it under an AsyncHandlerRejection (or AsyncHandlerError), but that will differ from the callback behavior that allows user to throw anything.

Async functions and Promises don't have an equivalent to "callback throw behavior". A rejection is the equivalent of calling a callback passing an Error object to it (so in our case, calling next(err)).

docs/guides/8to9guide.md

mmarchini · 2020-07-06T23:25:46Z

docs/guides/8to9guide.md

+- `fn.length === 2` (arity 2);
+- `fn instanceof AsyncFunction`;
+- if the async function resolves, it calls `next()`;
+- any value resolved will be discarded;


Can you elaborate on "any value resolved will be discarded"?

do you think it would read better "any value returned by the async function?". I will make the change.

mmarchini · 2020-07-06T23:28:30Z

docs/guides/8to9guide.md

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


This looks like a middleware example?

mmarchini · 2020-07-06T23:29:02Z

docs/guides/8to9guide.md

+The `8.x` API allows re-routing when calling `next()` with a string value. If the string matches a valid route,
+it will re-route to that handler. The same is valid for resolving a async function. Whatever value it returns,
+will be sent to `next()` and the behavior will be exactly the same.  


How do users call next() if it is not passed as an argument to async functions?

Ah, I see, if the async function returns a string it will reroute. I guess it makes sense for rerouting, I was thinking of cross-route calls instead.

docs/guides/8to9guide.md

package.json

docs/guides/8to9guide.md

mmarchini · 2020-07-06T23:37:58Z

lib/errorTypes.js

@@ -5,5 +5,6 @@ var errors = require('restify-errors');
 // This allows Restify to work with restify-errors v6+
 module.exports = {
    RequestCloseError: errors.makeConstructor('RequestCloseError'),
-    RouteMissingError: errors.makeConstructor('RouteMissingError')
+    RouteMissingError: errors.makeConstructor('RouteMissingError'),
+    AsyncHandlerError: errors.makeConstructor('AsyncHandlerError')


The name is AsyncHandlerError but we also use it on middlewares. Wouldn't it cause confusion to users? Maybe a more generic AsyncError would fit better?

lib/server.js

ghermeto · 2020-07-08T21:30:38Z

I spoke with @hekike and he said I can merge this.

Just want to make sure @mmarchini (who had comments) and others can take a look at it before I do.

cc/ @m5m1th @josephharrington

m5m1th

Nice! Might be worth dogfooding this a bit and getting a feel for the ergonomics and possible gotchas before wide adoption.

ghermeto · 2020-07-10T04:24:34Z

ok, since no new objection was raised, and all comments were addressed, I'm merging this code. We can always submit fixes before we make a new release.

BREAKING CHANGE: drops suppoprt to node 8 and updates linting rules

BREAKING CHANGE: adds async/await support to pre, use and handler chains

nikeee · 2021-03-25T22:03:07Z

What happens if the handler calls next() besides being async? For example:

server.get('/async', async function (req, res, next) {
    const body = await thatThing();
    res.send(body);
    next();
});

Is this valid?

ghermeto requested review from misterdjules, retrohacker, m5m1th, hekike, DonutEspresso, mmarchini and daviande June 27, 2020 10:16

ghermeto mentioned this pull request Jul 1, 2020

Async/await basic support #1837

Closed

hekike requested changes Jul 1, 2020

View reviewed changes

hekike reviewed Jul 1, 2020

View reviewed changes

ghermeto force-pushed the 9.x branch from 8745fcd to c6200f0 Compare July 1, 2020 19:56

retrohacker approved these changes Jul 6, 2020

View reviewed changes

daviande approved these changes Jul 6, 2020

View reviewed changes

mmarchini reviewed Jul 7, 2020

View reviewed changes

m5m1th approved these changes Jul 9, 2020

View reviewed changes

ghermeto added 2 commits July 9, 2020 21:27

chore: drop support for node 8

75e8bb5

BREAKING CHANGE: drops suppoprt to node 8 and updates linting rules

feat: async/await support

2e388dd

BREAKING CHANGE: adds async/await support to pre, use and handler chains

ghermeto force-pushed the 9.x branch from 681ca57 to 2e388dd Compare July 10, 2020 04:31

ghermeto merged commit 12be9e2 into master Jul 10, 2020

mmarchini mentioned this pull request Aug 13, 2020

Restify does not handle Promise rejection #1853

Open

nikeee mentioned this pull request Mar 25, 2021

Restify doesn't provide a way to catch Promise rejections in a middleware #1304

Closed

connyay mentioned this pull request Jan 30, 2023

feat(restify): Add v9 typings. DefinitelyTyped/DefinitelyTyped#64087

Closed

8 tasks


		### Drops support for Node.js `8.x`

		Restify requires Node.js version `>=10.21.0`.

9.X - async/await #1833

9.X - async/await #1833

Conversation

ghermeto commented Jun 27, 2020

Pre-Submission Checklist

Changes

ghermeto commented Jun 27, 2020

ghermeto commented Jun 28, 2020 • edited Loading

ghermeto commented Jun 28, 2020 • edited Loading

ghermeto commented Jun 28, 2020

DonutEspresso commented Jul 1, 2020 • edited Loading

ghermeto commented Jul 1, 2020 • edited Loading

ghermeto commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hekike Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cprussin Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hekike Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmarchini commented Jul 1, 2020

ghermeto commented Jul 1, 2020

DonutEspresso commented Jul 2, 2020 • edited Loading

DonutEspresso commented Jul 2, 2020

DonutEspresso commented Jul 2, 2020

ghermeto commented Jul 2, 2020

retrohacker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmarchini left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghermeto commented Jul 8, 2020

m5m1th left a comment

Choose a reason for hiding this comment

ghermeto commented Jul 10, 2020

nikeee commented Mar 25, 2021 • edited Loading

ghermeto commented Jun 28, 2020 •

edited

Loading

ghermeto commented Jun 28, 2020 •

edited

Loading

DonutEspresso commented Jul 1, 2020 •

edited

Loading

ghermeto commented Jul 1, 2020 •

edited

Loading

hekike Jul 1, 2020 •

edited

Loading

cprussin Jul 1, 2020 •

edited

Loading

hekike Jul 1, 2020 •

edited

Loading

DonutEspresso commented Jul 2, 2020 •

edited

Loading

mmarchini left a comment •

edited

Loading

nikeee commented Mar 25, 2021 •

edited

Loading