Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix(runtime-handler): fixes async handler functions throwing errors #394

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

Open
philnash wants to merge 2 commits into main
base: main
Choose a base branch
Loading
from should-not-print-in-function-runner

Conversation

@philnash
Copy link
Contributor

@philnash philnash commented Apr 4, 2022

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

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

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.
@philnash philnash requested a review from dkundel April 4, 2022 09:27
@philnash philnash changed the title (削除) fix(runtime-handler): fixes async handler functions throwning errors (削除ここまで) (追記) fix(runtime-handler): fixes async handler functions throwing errors (追記ここまで) Apr 4, 2022
}
handler(context, event, callback);
if (handler.constructor.name === 'AsyncFunction') {
await handler(context, event, callback);
Copy link
Contributor

@dkundel dkundel Apr 4, 2022

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

Copy link
Contributor Author

@philnash philnash Apr 4, 2022

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!

Copy link

changeset-bot bot commented Mar 26, 2024

⚠️ No Changeset found

Latest commit: 09612e7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

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

Reviewers

1 more reviewer

@dkundel dkundel dkundel left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /