Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify Sentry request handling to eliminate memory leak #434

Open
MaximusHaximus opened this issue Jun 4, 2021 · 0 comments
Open

Modify Sentry request handling to eliminate memory leak #434

MaximusHaximus opened this issue Jun 4, 2021 · 0 comments

Comments

@MaximusHaximus
Copy link
Contributor

even though there's a comment on our sentry middleware saying it came from the Sentry example, it's actually different -- it looks like the example code for KOA used a domain object to wrap the logic, but the domain logic didn't get copied in to our implementation -- even though the comment from the example code referencing domains is still there. Did you delete the domain object for a compatibility or due to an encountered bug?

Calling addEventProcessor() on the same global hub object's scope for every request is causing us to add a new event processor handler for every request we process, and they never get removed, thus the memory leak per every request. As a visible side-effect (and the reason why we are seeing stack overflow errors), the execution of event processors happens synchronously in one long recursive call chain, so once we get a few thousand of these event processors registered, we're overflowing the stack. Also, I think we're mass parsing the same event + request over and over with the same parseRequest() logic from Sentry since we've added thousands of event processors to the queue (1 per request).

Here's the troublesome synchronous recursive call pattern:
https://github.com/getsentry/sentry-javascript/blob/e59bb030198f20c5208ece66c1f46ed6b6410fd4/packages/hub/src/scope.ts#L440

It wasn't obvious that using domains insulates against this, but I found the code that makes copies of the global hub if you call getCurrentHub() from inside of a domain that doesn't have its own hub attached:
https://github.com/getsentry/sentry-javascript/blob/172e478084ec0d61b8f30efdda476e7e33e5237f/packages/hub/src/hub.ts#L530

The example code that Sentry provides uses domains to avoids this problem, but in a bit of a misleading way indicates the 'domains are optional', when in reality you need code that looks fairly different from their example if you don't use domains, and they are actually relying on domain behavior involving the cloning of the global hub to ensure that there's not a memory leak.
https://docs.sentry.io/platforms/node/guides/koa/

Our similar code, without the use of domains, thus introducing the bug:
https://github.com/near/near-contract-helper/blob/master/middleware/sentry.js#L30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant