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

Run specific code outside of Sentry domain/hub/scope #7031

Closed
nwalters512 opened this issue Feb 1, 2023 · 5 comments
Closed

Run specific code outside of Sentry domain/hub/scope #7031

nwalters512 opened this issue Feb 1, 2023 · 5 comments
Labels
Package: node Issues related to the Sentry Node SDK Type: Improvement

Comments

@nwalters512
Copy link

Problem Statement

My web server maintains a pool of long-lived resources. These resources are created on application startup and used in some of the requests. However, if we detect that a resource is unhealthy during a request, we'll remove it and create a new one.

These resources each maintain a long-lived socket. As best as I can tell, Sentry's hub/scope mechanisms maintain a reference to the incoming HTTP request and associate it with this socket. If the request object is very large (e.g. it contains a lot of data on res.locals), this data will be retained effectively forever, even once the request finishes. This manifests as a memory leak.

I'm not entirely sure of how Sentry's internals work, but this appears to be related to Sentry's usage of Node domains to track request context across async operations. If I take a heap snapshot at runtime, I can see that the response is ultimately retained in a domain's members, and if I disable Sentry completely, the response is no longer retained in any domain.

I've created a small repo to illustrate this issue: https://github.com/nwalters512/sentry-domain-memory-leak. Follow these steps to see the issue:

  • Clone the repo and run yarn install
  • Run docker pull ubuntu
  • Run node --inspect index.js
  • Attach a debugger and look at the memory consumption; it should be ~10MB
  • Make a request to http://localhost:80/ in your browser.
  • Look at the memory consumption in the debugger; it should be about 300MB and should stay that way even after manually triggering a garbage collection
  • Stop the server, remove all Sentry-related code from index.js, and repeat the above steps. Note that memory consumption should fall back to ~10MB either immediately or after triggering a garbage collection.

I'm writing this as a feature request because I have a relatively clear idea of what I want from Sentry: the ability to run a piece of code completely outside of Sentry's domains/hubs/scopes. AFAICT from the docs, there's not currently a way to do this.

Solution Brainstorm

From an API standpoint, something like this would be nice:

await Sentry.disableScope(async () => {
  await makeLongLivedResource();
});

Not knowing much about Sentry's internals, I can't really guess as to exactly how this could be implemented.

@AbhiPrasad
Copy link
Member

Hey @nwalters512, thanks for writing in! This is a tricky problem, and we're not sure how to exactly solve it at the moment in part because there is no way to write a disableScope as per what you put in the issue.

We can see here that the req and res event emitters are put on the domain:

const local = domain.create();
local.add(req);
local.add(res);

I think this is the source of your issue.

I do have a question though - why are you putting these objects on the event emitters? Could you take advantage of AsyncLocalStorage instead?

@nwalters512
Copy link
Author

nwalters512 commented Feb 2, 2023

It's a common and officially documented pattern to place request-specific context on res.locals in Express applications: https://expressjs.com/en/api.html#res.locals. I would expect Sentry to support the official features of the frameworks it instruments, as it would be a massive undertaking to rewrite our entire application to use AsyncLocalStorage instead.

@nwalters512
Copy link
Author

I suppose I can ask the same question though: is there a reason that Sentry uses the deprecated Domains API as opposed to AsyncLocalStorage? I'm not sure exactly what you use domains for, but if it's for tracking context across async operations, ALS seems like a perfect fit.

@lbogdan
Copy link

lbogdan commented Feb 2, 2023

@nwalters512 AFAIK ALS was not available when @sentry/node was created, and so its core is tightly coupled to the Domains API. That means moving to ALS would involve a complete rewrite (or at least a major refactor) of @sentry/node.

You can find a more detailed discussion here: #4071 .

Later edit: also this: #3660 .

@AbhiPrasad
Copy link
Member

@nwalters512 thanks for your patience. With the 7.48.0 release of the SDK, we now use AsyncLocalStorage instead of domains, so we shouldn't be retaining the res/req anymore - so you don't need your workaround anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK Type: Improvement
Projects
None yet
Development

No branches or pull requests

3 participants