[7.48.0][Node.js] Memory leak

[xr0master]

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 SDK are you using?

@sentry/node

SDK Version

7.48.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

No response

Steps to Reproduce

Updated from version 7.44.2 to 7.48.0 and started getting a report on the server about the fully used memory. Tried twice. Then I returned to version 7.44.2 and the problem disappeared.

This is a production server, there is always a chance that the memory has been used by something else.

The node version is 16.20

Expected Result

no leak

Actual Result

leak

[AbhiPrasad]

Hey @xr0master, without a reproduction this is really hard for us to debug. The notable change we made was to starting using async hooks in https://github.com/getsentry/sentry-javascript/releases/tag/7.48.0.

Could you try updating to 7.47.0 and seeing if that makes a difference?

[AbhiPrasad]

Also, are you using express, or another web framework? Was the memory leak just from the server running, or when it was receiving requests?

[xr0master]

@AbhiPrasad thank you for your quick response and your interest.
The framework is Fastify with custom Sentry implementation: #4784 (comment)

Was the memory leak just from the server running, or when it was receiving requests?

This server receives many requests, so it's difficult for me to answer that question. Most likely because of receiving requests.

Could you try updating to 7.47.0 and seeing if that makes a difference?

Yes, I'll try a little later. It will take me a few days to do this.
I can also try updating Sentry on a different server that Express is using.

[AbhiPrasad]

@xr0master you'll have to remove your domains implementation, you can just use runWithAsyncContext instead with 7.48.0. See our docs: https://docs.sentry.io/platforms/node/configuration/async-context/

Also the leak may be because of:

domain.add(req as never);
domain.add(res as never);

which actually you should remove regardless, since this does nothing for you even on 7.44.2 because you aren't also registering an error handler.

[xr0master]

The error handler is located in a different place since it is a built-in global handler in Fastify. But I may not fully understand it.

In any case, I will take your advice and switch to a new implementation. Let's see.
Thank you.

[bagr001]

Hi, I can confirm 7.47.0 is fine (using Fastify) but 7.48.0 has a memory leak.

[AbhiPrasad]

Hey @bagr001 could you share the Node version you are using, as well as the way you are initializing sentry? Are you using the implementation here: #4784 (comment)?

[bagr001]

@AbhiPrasad My Node version is 17.9.0 and using this plugin https://github.com/immobiliare/fastify-sentry

[AbhiPrasad]

Seems like this will be fixed with immobiliare/fastify-sentry#533!

[xr0master]

So, I will also confirm 7.47.0 is fine.

I changed the implementation from the domain to the async context. The code runs for a few hours on the production server, and no problems have been noticed.
It is difficult to say if the issue was with Fastify or with the new Sentry SDK and the domain. However, the issue has been resolved.

P.S. It's up to you guys, but it may be better to increase the major version of the SDK next time. IMO

[AbhiPrasad]

Hey @xr0master apologies on that. We decided to ship this with a minor because we found no regressions when testing, but I guess we didn't look at the case where both domains and async local storage were used at the same time.

We wanted to get this out of the door asap because of the massive performance benefits you get by using AsyncLocalStorage instead of domains - up to 30% is a huge deal for our users.

[xr0master]

@AbhiPrasad You don't need to apologize. I definitely understand your motives. And I think not only me.
I'm pretty sure 99% of your users won't notice this problem as they are using the official SDK. Most likely they won't notice any improvements either :)

[AbhiPrasad]

Closing this for now since it seems it was just fastify having the issue, and domains were dropped with https://github.com/immobiliare/fastify-sentry/releases/tag/v6.0.0.

Please re-open if anything else comes up. Thanks!