unhandled promise crahing server with sentry middleware

[modestaspruckus]

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.30.0

Framework Version

No response

Link to Sentry event

No response

Steps to Reproduce

Sentry request handler middlware is changing behaviour of unhandledPromise. Without this middleware below

app.use(Sentry.Handlers.requestHandler());

Error with unhandled promise is thrown with unhandledRejection event which doesn't force node instance to restart.

With middleware above the error is thrown with uncaughtException event which crashing the node server.

Expected Result

Sentry.Handlers.requestHandler() middleware should not change behavior of promise errors.

Actual Result

Sentry.Handlers.requestHandler() middleware is changing promise error behavior

[Lms24]

Hi @modestaspruckus thanks for writing in. Can you post your Sentry.init config? Are you adding or removing any integrations? The reason I'm asking is because by default, our OnUnhandledRejection integration should capture unhandled promise rejections but only emit a warning. If you set it to strict mode, it will exit the process.

[modestaspruckus]

Hey @Lms24, here is the config.

Sentry.init({
    dsn: process.env.SENTRY_DSN,
    environment: process.env.ENV_NAME || 'local',
    release: `backend-${process.env.ENV_NAME}@${process.env.BUILD_NUMBER}`,
});

I'm not using any integrations. Additionally, there is sentry error middleware

app.use(Sentry.Handlers.errorHandler());

[modestaspruckus]

Hey, any news for this? Various API calls sometimes fails due network issues and this causes node instances to restart. This wasn't the case on 7.6.0 version

This can simply be reproduced by rejecting promise

app.get('/test', async (req, res, next) => {
	const promise = new Promise((resolve, reject) => {
		reject(new Error('Something bad hapened'));
	});

	res.json();
});

By commenting out sentry middleware it starts working as expected

// app.use(Sentry.Handlers.requestHandler());

[Lms24]

@modestaspruckus no news yet. I quickly tried reproducing this and so far I couldn't. Here's what I did:

1. yarn add express @sentry/node

2. index.js:

const express = require("express");
const Sentry = require("@sentry/node");

const app = express();

Sentry.init({
  dsn: "___DSN___",
  environment: "local",
  release: `backend-test@$1`,
});

app.get("/test", async (req, res, next) => {
  const promise = new Promise((resolve, reject) => {
    reject(new Error("Something bad hapened"));
  });

  res.json();
});

// app.use(Sentry.Handlers.errorHandler());
app.listen(3000);
console.log("Listening on port 3000");

3. curl localhost:3000/test

Regardless of using the errorHandler or not, the app doesn't crash for me.

Can you provide a full reproduction so that we can investigate this?

Oh, one more thing: The order in which you add middleware matters. According to our docs, you should add the errorhandler "before any other error middleware and after all controllers".

[modestaspruckus]

You need to add request middleware also

app.use(Sentry.Handlers.requestHandler());

[modestaspruckus]

I've created repository to debug - https://github.com/modestaspruckus/sentry-debug

1. run npm ci
2. run node index.js
3. Go to http://localhost:3000/test
4. Node crashed.

Comment out app.use(Sentry.Handlers.requestHandler()); and do same steps again.
You will notice that node will not crash and will show error in 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:

[Lms24]

Thanks for the repro (and my bad about the request handler, I misread your description, sorry). I can reproduce it now. Will try to find a fix.

[Lms24]

Update: It seems #5627 is the culprit here. Let's see how to fix this

[Lms24]

Hi @modestaspruckus, I thought a little about this and did some research: We can't revert #5627 as it would be a breaking change (I know ironic, as introducing it was also kind of an accidental breaking change - really sorry about that).

After some research, I think the problem is more related to the fact that your handler is async and express can't handle these errors. Hence, neither our OnUnhandledRejection integration nor our errorHandler catch this error at all and the process exits. The fact that this only happens when requestHandler is used is probably caused by us using domains in that handler.
So my advice here would be to try/catch async errors and use await as follows:

app.get("/test", async (req, res, next) => {
  try {
    const promise = new Promise((resolve, reject) => {
      reject(new Error("Something bad hapened"));
    });
    await promise;
  } catch (error) {
    next(error);
  }

  res.json();
});

More information about async handlers: https://zellwk.com/blog/async-await-express/
Also, this library helps abstracting the try/catch away: https://www.npmjs.com/package/express-async-handler

Hope this helps. Let me know if this issue is resolved or you have additional questions :)

[modestaspruckus]

Thanks @Lms24. Adding try/catch is a solution.

I think the problem is more related to the fact that your handler is async and express can't handle these errors - your state here is incorrect because node.js emits event unhandledRejection when error occurs on unhandled promise. This behavior is changed with sentry middleware, instead of emitting unhandledRejection event, it emits uncaughtException

https://github.com/modestaspruckus/sentry-debug/blob/main/index.js#L7

[KFoxder]

We are running into the same issue preventing us from being able to upgrade to the latest sentry. We will see if this works for us as well...

[jcasner]

We're running into this also. Unhandled async errors work fine when using Sentry 7.11.1, but start breaking in 7.12.0 when running express 4.18.2.

• Upgrading to Express 5 beta "fixes" this; we're not ready to be on the beta channel for express
• Using an error wrapper, as described in this SO post, works well https://stackoverflow.com/a/49417798

We're going to stick with Sentry 7.11.1 until we get things fixed up. Ultimately, this seems to come down to an anti-pattern of allowing unhandled async errors / promise rejections in Express. According to the express documentation:

For errors returned from asynchronous functions invoked by route handlers and middleware, you must pass them to the next() function, where Express will catch and process them.

It seems like something in older versions of Sentry actually captures these unhandled promise rejections and masks the true problem.

[Lms24]

It seems like something in older versions of Sentry actually captures these unhandled promise rejections and masks the true problem.

Yes, after researching and discussing this internally, we came to the same conclusion. I'm going to close this issue, as it seems most users here were able to resolve this with properly handling async errors. Everyone, feel free to leave are reply if things are still unclear.