-
Notifications
You must be signed in to change notification settings - Fork 59
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
fix(runtime-handler): fixes async handler functions throwing errors #394
base: main
Are you sure you want to change the base?
Conversation
g errors When a Twilio Function deemed an async function throws an error, the handler doesn't catch it and instead throws an unhandled promise rejection error. Also, when constructing the context and the checks for valid account sids and auth tokens in the getTwilioClient function, we say that it should print the error. However, passing the logger to the context serializes and deserialises it, since it is being passed to another process, and and it is no longer an instance of Logger. This causes another error, trying to call on the logger's error function that no longer exists. So, this PR does 2 things, it checks the handler function to see if it is an async function and calls it with await so that it can catch the error. And it sets shouldPrintMessage to true only if there is a logger object that has an error function.
@@ -102,7 +102,11 @@ process.on( | |||
`Could not find a "handler" function in file ${functionPath}` | |||
); | |||
} | |||
handler(context, event, callback); | |||
if (handler.constructor.name === 'AsyncFunction') { | |||
await handler(context, event, callback); |
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.
Is this causing any issues in terms of functionality parity with production Functions? I'm worried whether this will await things that real Functions would ultimately cancel
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 don't think so. Once you call the callback
(with either a success or error) messages are passed up to the parent and the process ended. This just exists to catch errors thrown by the function and handle them by also passing to the parent and closing the process.
One thing that we don't have in the runtime-handler here is cancellation of functions after 10 seconds, but that's true for non-async functions too.
I guess one question is, what happens in production Functions if you have an async
function that throws an error for some reason?
One other thing, this actually allows for the dev runtime-handler to render the descriptive error page that you implemented. So in terms of parity with prod functions, I'm not sure where that lies!
|
When a Twilio Function deemed an async function throws an error, the
handler doesn't catch it and instead throws an unhandled promise
rejection error.
Also, when constructing the context and the checks for valid account
sids and auth tokens in the getTwilioClient function, we say that it
should print the error. However, passing the logger to the context
serializes and deserialises it, since it is being passed to another
process, and and it is no longer an instance of Logger. This causes
another error, trying to call on the logger's error function that no
longer exists.
So, this PR does 2 things, it checks the handler function to see if it
is an async function and calls it with await so that it can catch the
error. And it sets shouldPrintMessage to true only if there is a logger
object that has an error function. In the case of receiving an error
from the forked process, this now logs the error with the original
logger in the parent process.
Contributing to Twilio