Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16a6ea2
fix(runtime-handler): fixes async handler functions thrownin
philnash Apr 4, 2022
09612e7
Merge remote-tracking branch 'origin/main' into should-not-print-in-f…
victoray Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/runtime-handler/src/dev-runtime/internal/functionRunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ process.on('uncaughtException', (err, origin) => {

process.on(
'message',
({ functionPath, event, config, path }: FunctionRunnerOptions) => {
async ({ functionPath, event, config, path }: FunctionRunnerOptions) => {
try {
setRoutes(config.routes);
constructGlobalScope(config);
Expand Down Expand Up @@ -110,7 +110,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);
Copy link
Member

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

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!

} else {
handler(context, event, callback);
}
} catch (err) {
if (process.send) {
process.send({ err: serializeError(err) });
Expand Down
11 changes: 6 additions & 5 deletions packages/runtime-handler/src/dev-runtime/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,13 +157,13 @@ export function constructContext<T extends {} = {}>(
}> {
function getTwilioClient(opts?: TwilioClientOptions): TwilioClient {
checkForValidAccountSid(env.ACCOUNT_SID, {
shouldPrintMessage: true,
shouldPrintMessage: logger ? !!logger.error : false,
shouldThrowError: true,
functionName: 'context.getTwilioClient()',
logger: logger,
});
checkForValidAuthToken(env.AUTH_TOKEN, {
shouldPrintMessage: true,
shouldPrintMessage: logger ? !!logger.error : false,
shouldThrowError: true,
functionName: 'context.getTwilioClient()',
logger: logger,
Expand Down Expand Up @@ -221,12 +221,13 @@ export function handleError(
err: Error | string | object,
req: ExpressRequest,
res: ExpressResponse,
functionFilePath?: string
functionFilePath?: string,
logger?: LoggerInstance
) {
res.status(500);
if (isError(err)) {
const cleanedupError = cleanUpStackTrace(err);

logger?.error(cleanedupError.toString());
if (req.useragent && (req.useragent.isDesktop || req.useragent.isMobile)) {
res.type('text/html');
res.send(wrapErrorInHtml(cleanedupError, functionFilePath));
Expand Down Expand Up @@ -334,7 +335,7 @@ export function functionPathToRoute(

if (err) {
const error = deserializeError(err);
handleError(error, req, res, functionPath);
handleError(error, req, res, functionPath, config.logger);
}

if (reply) {
Expand Down
Loading