Unhandled promise rejection causes app to crash with Node requestHandler registered

[rchl]

Environment

self-hosted (https://develop.sentry.dev/self-hosted/)

Version

No response

Link

No response

DSN

No response

Steps to Reproduce

1. Clone https://github.com/rchl/sentry-crash-unhandled-promise-rejection
2. Run npm i and npm run start

Expected Result

The script should keep running after HTTP request finishes.

Actual Result

After the HTTP request finishes, the app crashes.

[getsantry]

Assigning to @getsentry/support for routing, due by Monday, January 30th at 2:03 pm (yyz). ⏲️

[rchl]

More details:

The app starts an HTTP server which registers Sentry's request handler:

https://github.com/rchl/sentry-crash-unhandled-promise-rejection/blob/ec3a068fec00c9d7265d6f395b5076ca6b86f3bc/index.mjs#L15-L15

With this handler registered, unhandled promise rejection crashes the app.

Without the handler registered, the app keeps running and Node prints this text in the console:

This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason

The fact that with Sentry the app crashes is IMO a bug. It should work the same as without Sentry.

[getsantry]

Routing to @getsentry/team-web-sdk-frontend for triage, due by Tuesday, January 31st at 2:30 pm (sfo). ⏲️

[lforst]

Hi, trying out your example there are a few observations:

1. Running the example without any Sentry code still crashes the app.
2. Running the example with Sentry.init will prevent the app from crashing
3. Running the example with Sentry.init + the Sentry middlware will again crash the app

IMO only observation 2 from above is problematic and we somehow need to fix.

The reason your app is crashing at all is because you're registering an async handler in express which express doesn't support. Read more here: https://expressjs.com/en/advanced/best-practice-performance.html#use-promises

[rchl]

Good point about it still crashing without Sentry at all, missed that part. But this is minimized case from a Nuxt project where it doesn't crash without Sentry but does with Sentry+request handler. So there might be something more to it that I'd have to check.

The reason your app is crashing at all is because you're registering an async handler in express which express doesn't support. Read more here: https://expressjs.com/en/advanced/best-practice-performance.html#use-promises

The point of this example is to show the difference in how Unhandled Promise rejection is handled with and without Sentry.

I know that express doesn't technically support async handlers and it only cares that res.end() or similar is called. But it can be called from either sync or async handler so it's not strictly wrong to use async handlers, as long as there is code that catches those and responds to the request. For the purpose of simplifying that example, there is no such code here.

[lforst]

Yeah, the dilemma we face is that we need to attach handlers to onUnhandledPromiseRejection and onUncaughtException but we do not know if users will handle these cases themselves. We are aware of the fact that the current behavior isn't optimal (i.e. we are altering what node does by default).

Changing this behavior however, is problematic because users already depend on it so we're going to fix it in the next major version of the SDK. We already did something similar not long ago and opaquely broke a bunch of existing users which we would like to avoid. We're tracking this change for the next major here, here and here

I will close this issue, since we are aware of this and currently want to keep the issue backlog clean. I hope that's okay. Thanks again for reporting this!