Uncaught async middleware error crashing server

[sderrow]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/node

SDK Version

7.29.0

Framework Version

No response

Link to Sentry event

No response

Steps to Reproduce

Here's a CodeSandbox demo of the situation.

Brief summary:
I'm using express-async-errors in order to catch errors that are thrown in async handler and automatically forward to next. This was working fine through sentry-javascript version 7.11.1 (Aug 2022). Starting with 7.12.0 up to the latest release (7.29.0), Sentry's wrappers are somehow overwriting the auto-forwarding of thrown errors in async middleware, so any errors are uncaught and crash the server. Furthermore, the behavior is not even consistent. If the async middleware is added directly as part of a route declaration, then errors within that middleware will still be forwarded. It's only when the middleware is added to an entire express router (see the CodeSandbox demo).

Expected Result

Ideally, Sentry's SDK would not interfere with express-async-errors so that errors thrown in async middleware can be caught and forwarded to next. But at a minimum, the behavior should be consistent between router and route middlewares.

Actual Result

Server is crashed when errors are thrown in async middleware preceding routers.

[AbhiPrasad]

Hey @sderrow thanks for writing in! Please see #5931 (comment) on more details on why async exceptions work this way

See #6137 (comment) for some more details. To grab a snippet:

As per the Node docs if an unhandled rejection is triggered the process should be exited: https://nodejs.org/api/process.html#warning-using-uncaughtexception-correctly. In this case, the default behavior is to exit the process, and if you register a custom handler, you should also be exiting the process. Since Sentry is registering a custom handler, we are exiting the process as a result.

The correct use of 'uncaughtException' is to perform synchronous cleanup of allocated resources (e.g. file descriptors, handles, etc) before shutting down the process. It is not safe to resume normal operation after 'uncaughtException'.

To restart a crashed application in a more reliable way, whether 'uncaughtException' is emitted or not, an external monitor should be employed in a separate process to detect application failures and recover or restart as needed.

You can set the exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered option in the OnUncaughtException integration to change this behaviour.

[sderrow]

Thanks for the quick response, @AbhiPrasad!

Not sure the onUncaughtException integration works for me because express-async-errors doesn't register an uncaughtException handler with node. It just patches express's Layer handler method.

With Sentry enabled, express-async-errors works fine on async errors thrown in all non-middleware express handlers and middleware handlers that are registered directly on route declarations, but it doesn't work for middleware registered before routers, making for a disjointed experience. I understand it's not Sentry's responsibility to place nice with express-async-errors, but does that all make sense? Am I missing something?

[AbhiPrasad]

but it doesn't work for middleware registered before routers, making for a disjointed experience

Ahh I see, that makes this clearer. It's not Sentry's responsibility here to forward the error to next. This was the behaviour that existed 7.11.1 and was changed after that (the change in #5627).

The Sentry SDK shouldn't be forwarded errors to next at all, so if there is some inconsistent behaviour here, I suspect there is something happening with express-async-errors. It might be that express-async-errors is forwarded the error themselves, but only in the route declarations, not in the middleware. I would try to follow up with them.

[sderrow]

For sure, but express-async-errors works just fine in all cases, so there's no inconsistent behavior, if I disable Sentry entirely. That's why I'm wondering if the SDK is wrapping middleware in its own way that blocks behavior of other patches.

[github-actions]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀