Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Uncaught error in async endpoint crashes express #2337

Closed
mvarendorff opened this issue Mar 17, 2023 · 1 comment
Closed

Uncaught error in async endpoint crashes express #2337

mvarendorff opened this issue Mar 17, 2023 · 1 comment
Labels
status: needs-triage Possible bug which hasn't been reproduced yet

Comments

@mvarendorff
Copy link
Contributor

mvarendorff commented Mar 17, 2023

Bug Report

Any uncaught errors in an async handler function of an endpoint will crash the underlying express instance (however not the process as it seems). I understand that this is something that is flawed in express and should be fixed upstream however the Payload docs themselves use an async endpoint in the docs without any mention of this issue so I feel like it should be handled until express 5 becomes stable.

Workaround in the form of a plugin is included below.

Steps to Reproduce

  1. Add these two innocently looking endpoints to the Payload config:
endpoints: [
  {
    path: "/poof",
    method: "get",
    handler: (req, res, next) => {
      throw new Error("Poof")
    },
  },
  {
    path: "/crash",
    method: "get",
    handler: async (req, res, next) => {
      throw new Error("Crash")
    },
  },
],
  1. Open http://localhost:3000/api/poof -> The result is a 500 error saying Poof
  2. Open http://localhost:3000/api/crash -> There is no response and any further requests don't produce a response anymore either.

Other Details

After some digging, it turns out that Express 4 does not handle errors in async route handlers which will be fixed in Express 5 however that's been in beta for a year now (related issue with maintainer updates: expressjs/express#4920).

My two ideas for resolution of this would be:

  1. Use express 5 beta (although I understand that this is probably not really an option for a project of this scale)
  2. When mounting the handlers, wrap them in an async handler that takes care of catching the error and handling it by passing it to next with something like this:
const safeAsyncHandler = (handler: PayloadHandler): PayloadHandler => async (req, res, next) => {
  try {
    await handler(req, res, next);
  } catch (e) {
    next(e);
  }
};

And then using that function in here (maybe somewhere else, I haven't looked far and wide but this seemed like a reasonable guess):

function mountEndpoints(express: Express, router: Router, endpoints: Endpoint[]): void {
endpoints.forEach((endpoint) => {
if (!endpoint.root) {
router[endpoint.method](endpoint.path, endpoint.handler);
} else {
express[endpoint.method](endpoint.path, endpoint.handler);
}
});
}

Payload version: 1.6.17

Workaround (Payload plugin)

Using the function from above, it's possible to write a plugin as workaround that handles async errors gracefully:

import {type Config, Endpoint, PayloadHandler} from "payload/config";

const safeAsyncHandler = (handler: PayloadHandler): PayloadHandler => async (req, res, next) => {
  try {
    await handler(req, res, next);
  } catch (e) {
    next(e);
  }
};

const patchEndpoint = (endpoint: Endpoint): Endpoint => {
  const handler = Array.isArray(endpoint.handler)
    ? endpoint.handler.map(safeAsyncHandler)
    : safeAsyncHandler(endpoint.handler);

  return {...endpoint, handler};
}

export const safeAsyncEndpoints = (config: Config) => {
  config.endpoints = config.endpoints?.map(patchEndpoint) ?? [];
  for (const c of (config.collections ?? [])) {
    c.endpoints = c.endpoints?.map(patchEndpoint) ?? [];
  }

  return config;
}

// Usage: plugins: [safeAsyncEndpoints] in payload.config.ts
@mvarendorff mvarendorff added the status: needs-triage Possible bug which hasn't been reproduced yet label Mar 17, 2023
@jmikrut
Copy link
Member

jmikrut commented Mar 17, 2023

Hey there! This is a great topic and the amount of information you've provided here is awesome.

I think personally the way to go would be your Option 2:

When mounting the handlers, wrap them in an async handler that takes care of catching the error and handling it

But I wonder, because having a try / catch in your own handler is typically always going to be required, is this duplicative? I think best practice would be to handle your own errors (thus controlling logs, resulting REST responses, etc.) It's an interesting thought.

Would certainly clean up custom handlers if you don't have to wrap every single one in a top-level try / catch.

I would love to get more feedback on this idea. I'll take it to my team and share it with Discord.

I will also convert this to a Discussion and tag it as a Feature Request.

Thanks for all of the notes. The plugin is great too!

@payloadcms payloadcms locked and limited conversation to collaborators Mar 17, 2023
@jmikrut jmikrut converted this issue into discussion #2340 Mar 17, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
status: needs-triage Possible bug which hasn't been reproduced yet
Projects
None yet
Development

No branches or pull requests

2 participants