-
Notifications
You must be signed in to change notification settings - Fork 210
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
chore(async): Refactor auth server routes to async/await #2286
Conversation
819ebc8
to
3eda0f6
Compare
4be4a7e
to
d22cc8c
Compare
7c01fed
to
38427e6
Compare
Phil suggested we just land these refactorings as they are completed, since the changes are going to bit rot otherwise. I've copied the checklist into #2305 for future work. I reviewed the changes other than password.js, could use a review for that one (and any others people have time to glance at). |
uid, | ||
}; | ||
|
||
await mailer.sendPostRemoveTwoStepAuthNotification( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to have @rfk verify this should be waited on here, as previously the promise was not returned to be waited on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm; to be honest I don't recall. Most of the places where we deliberately don't await a promise have a comment about why. I guess the only reason not to wait here would be to avoid a mail-sending error from "failing" the entire request, making it look like we failed to update the account when we really did. If we're worried about that, I think it would be better to explicitly await and then catch-and-ignore the error.
// cases where the user started creating a token but never completed. | ||
if (!token.verified) { | ||
await db.deleteTotpToken(sessionToken.uid); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lost the else statement here, exists should only be true if the token is verified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks OK to me 👍
oldAuthPW, | ||
emailRecord.authSalt, | ||
emailRecord.verifierVersion | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it was like this already, but I don't understand why we need to create a second Password
instance here with same parameters; could we just re-use the value of password
from above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can clean this up (and the other spots you called out). Thanks for the suggestions 👍
} | ||
}); | ||
const devices = await request.app.devices; | ||
devicesToNotify = devices; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could use devicesToNotify = await request.app.devices
directly here rather than using a separate const
?
return result; | ||
}); | ||
}); | ||
const hex = await random.hex(32); // TODO why is this async? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: the TODO, crypto.randomBytes
has an async mode that IIRC is actually worth doing if you're calling it a lot, to avoid janking the event loop.
}); | ||
} | ||
const accountData = await db.account(passwordChangeToken.uid); | ||
account = accountData; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, I don't think we need to intermediate const
here.
uid, | ||
}; | ||
|
||
await mailer.sendPostRemoveTwoStepAuthNotification( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm; to be honest I don't recall. Most of the places where we deliberately don't await a promise have a comment about why. I guess the only reason not to wait here would be to avoid a mail-sending error from "failing" the entire request, making it look like we failed to update the account when we really did. If we're worried about that, I think it would be better to explicitly await and then catch-and-ignore the error.
ea40561
to
6cda660
Compare
86e1038
to
a65c70b
Compare
a65c70b
to
3268114
Compare
Promises and async/await: an asynchronous adventure
We're going to convert some auth-server routes from promises to async. The hapi route handlers are already async functions, so hopefully this should be pretty straightforward 🤞.
Preliminaries
Bluebird (our promise library) is not compatible with async/await: petkaantonov/bluebird#1434
However, the auth route handlers are already async functions, so we might not need to worry about this for today. At least, the tests passed when I made some changes 😬
Converting Promise code to async/await code
The
await
keyword unwraps a Promise, converting a resolved promise into its value, and a rejected promise into an error.The
async
keyword wraps a function in a Promise: return value becomes a resolved promise, thrown error becomes a rejected promise.Remember: you can mix async and Promise code, because async functions return Promises. You don't have to convert a whole chain of functions at once.
Some nice things about async/await
Removes promise nesting due to conditional responses to intermediate values
Allows sync and async calls to be handled by the same error handling code
Escape from the 'pyramid of doom'
TODO other nice stuff
Some gotchas:
Things get a little tricky with error handling
return foo()
(returns rejected promise) vsreturn await foo()
(throws). Simpler to understand if you separate out assigning the awaited value and returning the value.If you want to parallelize, don't await serially. Instead, assign the promises and
await Promise.all(slowThings)
.More at MDN: https://mdn.io/asyncawait
A crude replacement algorithm
When you see a promise chain, turn the function outside the then/catch chain into an async function.
If you have a
.catch
callback, move the.then
and.catch
callbacks into a try-catch block.Then, await the promise instead of chaining to get to the next step.
If a value is passed from promise to promise, assign its awaited value.
If the value of the whole promise chain is returned,
return foo().then(bar)
, split this into assignment and returning the awaited value:const result = await foo().then(bar); return result
.Auth server routes: how deeply nested?
We have a few that are 8+ levels deep:
Let's walk through one together
I've started hacking on password.js to open this PR. We can make some more changes together.
Do the thing
Grab a file and put your name next to it, let's get to hackin.
Open PRs against this
async-conversion
branch, not against master 👍