UnhandledRejection getting caught as UncaughtException

[Sammaye]

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

Framework Version

4.17.1

Link to Sentry event

https://delenta.sentry.io/issues/3902577770/?project=6739047&query=is%3Aunresolved&referrer=issue-stream

SDK Setup

        global.process.on('uncaughtException', (error) => {
            console.log('in uncaught exception', error);
        });

        const sentryOptions = {
            environment: config.APP_ENV === 'DEV'
                ? 'development'
                : (
                    config.APP_ENV.includes('ALPHA')
                        ? 'alpha'
                        : (config.APP_ENV === 'BETA' ? 'beta' : 'production')
                ),
            dsn: config.LOGS_SENTRY_DSN,
            // Set tracesSampleRate to 1.0 to capture 100%
            // of transactions for performance monitoring.
            // We recommend adjusting this value in production
            tracesSampleRate: 0.25,
            integrations: integrations => {
                const newIntegrations = integrations.filter(integration =>
                    integration.name !== 'OnUncaughtException' &&
                    integration.name !== 'Http' &&
                    integration.name !== 'LocalVariables'
                );

                newIntegrations.push(
                    new Sentry.Integrations.OnUncaughtException({exitEvenIfOtherHandlersAreRegistered: false}),
                    new Sentry.Integrations.Http({tracing: true}),
                    new Sentry.Integrations.LocalVariables({
                        captureAllExceptions: true,
                    })
                );

                if (app) {
                    newIntegrations.push(
                        // enable Express.js middleware tracing
                        new Tracing.Integrations.Express({app}),
                    );
                }

                return newIntegrations;
            },
        };

        Sentry.init(sentryOptions);

Steps to Reproduce

Make a new express action like:

export async function a(req, res, next) {
    try {
        const [
            ab,
        ] = await Promise.all([
            b(),
        ]);

        return res.status(200).json({
              ab
        });
    }catch(error){
        console.log('caught error');
        next(error);
    }
}

async b() {
    throw new Error('omg');
    return this.countsdtdfdst({});
}

And then simply attach this action to any route, like /blah and go to this route in something like postman

Expected Result

It handles this via the express middleware correctly and assigns all express information.

Actual Result

It instead strips all that and throws it as a uncaught exception:

[ERROR] 11:04:08 Error: omg
in uncaught exception Error: omg
    at Function.b
caught error
GET /blah 503 437.030 ms - 87
(node:3408) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)

If I remove the new Sentry.Integrations.OnUncaughtException({exitEvenIfOtherHandlersAreRegistered: false}), line it works

[Sammaye]

As a note: the example above is a bit contrived since I have found out that the unhandledRejection is being thrown by mongoose in a later part of the Promise.all(), however, the issue still stands, when I remove sentry unhandledRejections are correctly handled, but with Sentry unhandledReejections redirect to uncaughtExceptions.

[Sammaye]

As another note, this line:

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

seems to cause the redirect, if I remove it the error is treated as a unhandled rejection

[Lms24]

Hey @Sammaye thanks for writing in! This seems to be very much related to #6750.

If I remove the new Sentry.Integrations.OnUncaughtException({exitEvenIfOtherHandlersAreRegistered: false}), line it works.

That's interesting, cause afaik this option should exactly prevent exiting. So what you could do instead is just register an uncaught exception handler yourself to have this effect (or remove the default integration). I know that this doesn't really fix the error but it might be a worthwhile workaround.

Let me reproduce this error to see if we can actually do something against this without a breaking change. This seems to be quite a complicated topic as described in #6750 and #5627. I can't guarantee a proper fix here, sorry :(

[Lms24]

I've tried to reproduce this locally but didn't get the app to crash yet. Basically I took your example and just cut it down a little. Would you mind taking a look at this code and tell me if I missed something?

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

const express = require("express");

global.process.on("uncaughtException", (error) => {
  console.log("in uncaught exception", error);
});

const app = express();

Sentry.init({
  environment: "production",
  dsn: "__DSN__",
  integrations: (integrations) => {
    const newIntegrations = integrations.filter(
      (integration) =>
        integration.name !== "OnUncaughtException" &&
        integration.name !== "LocalVariables"
    );

    newIntegrations.push(
      new Sentry.Integrations.OnUncaughtException({
        exitEvenIfOtherHandlersAreRegistered: false,
      }),
      new Sentry.Integrations.LocalVariables({
        captureAllExceptions: true,
      })
    );

    return newIntegrations;
  },
});

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

async function a(req, res, next) {
  try {
    const [ab] = await Promise.all([b()]);

    return res.status(200).json({
      ab,
    });
  } catch (error) {
    console.log("caught error");
    next(error);
  }
}

async function b() {
  throw new Error("omg");
  return this.countsdtdfdst({});
}

app.get("/test", a);

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

app.listen(3000, () => {
  console.log("server started on port 3000");
});

Calling GET localhost:3000/test leads to the following console output:

server started on port 3000
caught error
Error: omg

Plus, the error is sent to my sentry project

[Sammaye]

The issue is that as I found out, that doesn't cause an unhandledrejection, I thought it did but as comment I discovered that it was actually the use of mongoose within the Promise.all() that caused the issue, very weird one, but anyway yeah would have to artificially make an unhadledrejection

[Sammaye]

Also this could be quite hard to replicate since I think it was an issue in nodejs as to why this was event happening, let me expand:

async function a(req, res, next) {
  try {
    const [ab, bc] = await Promise.all([b(), (await c()).length]);

    return res.status(200).json({
      ab,
      bc
    });
  } catch (error) {
    console.log("caught error");
    next(error);
  }
}

async c () {
    // this is a  mongoose reference
    this.mdel.find({o:1})
}

I am unsure if it ha-pens to unhandledrejections in general since it seems to be a express middleware causing the redirect and by then everything should be caught in theory. Still not entirely sure what nodejs was doing but when it rejected b() it also rejecxt c() async so it caused an unhandledrejction

[Lms24]

I'm sorry but I still can't replicate the problem In fact I'm not sure if this is a problem of the Sentry SDK at all or if it is rather a Node/express problem when using async handlers.

Please provide a full reproduction example (e.g. a github repo or a full code snippet) that shows us how to reliably reproduce this. Also, what is your exact expectation that the SDK should handle differently?

[Sammaye]

I'll try and get one.

I do believe this is sentry since if I use winston then winston will not treat an unhandledRejection as a uncaughtException, like I said, when I remove sentry the error is treated as it should be by my legacy system which is just a basic winston logger.

Though considering how rare it is that someone will encounter this, since I am not sure if this is a bug nodejs (due to that fact that it is throwing duplicate errors, one caught and one uncaught) it might not be worth the effort.

[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 🥀