[node][7.113.0] tracingHandler of memory leak

[ProsperBao]

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?

Self-hosted/on-premise

Which SDK are you using?

@sentry/node

SDK Version

7.113.0

Framework Version

nitro latest

Link to Sentry event

No response

SDK Setup

Sentry.init({
    dsn: '',
    environment: 'debug',
    sampleRate: 1.0,
    integrations: [
      ...Sentry.autoDiscoverNodePerformanceMonitoringIntegrations(),
    ],
    tracesSampler: (samplingContext) => {
      if (samplingContext.parentSampled !== undefined) {
        return samplingContext.parentSampled
      }
      else {
        // return 0.0005
        return 1
      }
    },
  })

Steps to Reproduce

reproducible example

1. pnpm build:preview
2. pnpm test
3. open chrome://inspect/#devices

This is the memory footprint at the beginning and code

execute pnpm test, then wait for the number to reach 5000.

you can see that the memory footprint has not decreased

many instances created by sentry are not released

Then I looked at the source code, Modified my code

It's very clean,

I think here in the code but less clear action, I don't know if changing here will cause other effects

sentry-javascript/packages/node/src/handlers.ts

Line 106 in f185332

setImmediate(() => {

Expected Result

There should be no risk of memory leaks

Actual Result

Many internal instances of sentry are not released

[Lms24]

Hi @FuBaooo thanks for writing in! I took a look at your reproduction example (admittedly didn't run it). I have a feeling that the leak is happening because the Sentry.Handlers.requestHandler() is missing in your setup. I'd highly recommend adding the requesthandler because it takes care of isolating scopes for each request. This is crucial to avoid leaking data between concurrent requests. Most importantly, I think there's a good chance that the leak is cleaned up.

Since we don't have a guide (or official support) for Nitro, I'm gonna link you to our express documentation which shows how to use the handler in express. You probably need to register the handler similarly to the tracingHandler in your setup. Also please note, the order between tracingHandler and requestHandler is important, as pointed out here.

Can you give this a try and report back if it fixes your mem leak?

Also this issue might be related to #10790, where users also reported a memory leak. Did you use an older version of the SDK before encountering the leak? If yes, which exact version was that? We're still trying to narrow down the responsible change that caused the leaks reported in this issue.

Finally, we're currently in the final stretch of releasing a new major version of the SDK which will use OpenTelemetry for instrumentation of Node frameworks. So chances are, you won't encounter this leak at all anymore once our new major is stable and you upgraded.

[ProsperBao]

@Lms24 Sorry, I do use requestHandler in my actual project, I've added it to the repository, and I don't think requestHandler has to do with memory leaks.

I think it's the lack of getCurrentScope()?.clear() in res.once('finish').
I added getCurrentScope()?.clear() directly to 'node_modules/@sentry/node/esm/handers', the memory leak was solved.

To fix the memory leak, I executed this action in my code:

[ProsperBao]

@Lms24 I made sure I registered the requestHandler first and then the tracingHandler in the right order.

Extra note: The numbers attached to the front of the file are the nitro registration order.

[ProsperBao]

@Lms24 I haven't used the old sdk, this is the latest version

[Lms24]

Thanks for checking! Now at least we know that the request handler doesn't seem to play a part in this. I need to backlog this for the moment due to v8 being the top priority. Which is unfortunate for everyone still on v7, so sorry for that.

Also thanks for the reproduction, it'll be for sure helpful once we take another look at this.

[mydea]

Hey, we've released v8.0.0 of the SDK, which has a new instrumentation API. Could you give that a try, and see if the memory leak is gone? You can find info on how to update here: https://docs.sentry.io/platforms/javascript/guides/node/migration/v7-to-v8/

[getsantry]

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 remove the label Waiting for: Community, I will leave it alone ... forever!

---

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