search

getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.87k stars 1.55k forks source link

[7.48.0][Node.js] Memory leak #7896

Closed xr0master closed 1 year ago

xr0master commented 1 year ago

Is there an existing issue for this?

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@AbhiPrasad thank you for your quick response and your interest. The framework is Fastify with custom Sentry implementation: https://github.com/getsentry/sentry-javascript/issues/4784#issuecomment-1221362875

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

AbhiPrasad commented 1 year ago

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: https://github.com/getsentry/sentry-javascript/issues/4784#issuecomment-1221362875?

bagr001 commented 1 year ago

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

AbhiPrasad commented 1 year ago

Seems like this will be fixed with https://github.com/immobiliare/fastify-sentry/pull/533!

xr0master commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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!