sentry-javascript: @sentry/node memory leaks on node 10.13.0

This isn’t an issue in node but in Sentry as disabling the Sentry handlers fixes the issue;

@michalkvasnicak I investigated this and it’s not directly caused by Sentry.

Our @sentry/node transport has a dependency on https-proxy-agent, which has a dependency on agent-base which requires it’s patch-core.js file and this is what creates a leak 🤷

https://github.com/TooTallNate/node-agent-base/issues/22

You can easily verify this by copying its content into your test and running it with a heap stats:

const url = require("url");
const https = require("https");

https.request = (function(request) {
  return function(_options, cb) {
    let options;
    if (typeof _options === "string") {
      options = url.parse(_options);
    } else {
      options = Object.assign({}, _options);
    }
    if (null == options.port) {
      options.port = 443;
    }
    options.secureEndpoint = true;
    return request.call(https, options, cb);
  };
})(https.request);

https.get = function(options, cb) {
  const req = https.request(options, cb);
  req.end();
  return req;
};

it("works", () => {
  expect(true).toBe(true);
});

We’ll have to probably fork it or write a workaround.

I manually patched the Hub module directly in my node_modules folder. I removed the line using the dynamicRequire and just added import domain from 'domain'; at the top of the file and… it now works perfectly! No more leaks! 🎉

Maybe the dynamicRequire hack was needed before, but is no longer needed with newer versions of webpack? 🤔

I also tried to replace :

const domain = dynamicRequire(module, 'domain');

with:

const domain = require('domain');

and it also works fine. I don’t know which of those two solutions you would prefer.

Would you like me to open a PR with this fix?

@kamilogorek do you have any ideas for a workaround or temporary fix for this? Progress on the node issue looks quite slow

https://nodejs.org/en/blog/release/v10.16.0/

fixed some memory leaks, can someone test it?

We’re currently using PM2 to restart Node.js processes when memory reaches a certain threshold. https://pm2.io/doc/en/runtime/features/memory-limit/#max-memory-threshold-auto-reload

@abraxxas @tpbowden there’s an issue with leaking domain module in Node’s core itself. We’ll keep monitoring it and try to come up with a temporary solution before it’s fixed in the core. Related issue: https://github.com/nodejs/node/issues/23862

Looks like it’s related to bundling Sentry using webpack on the server - only happens when the app has been built by Webpack. Adding @sentry/node to externals has fixed our memory problem. Any idea why this happens?

@tpbowden You are right about this, I have the same issue. I was running the SDK v5.15.0 and node v12.3.1, both supposed to include all the required fixes mentioned here.

I am bundling all dependencies within my server bundle with webpack. This way I can ship a docker image without node_modules, but something is messing up the sentry SDK and it leaks memory when bundled this way.

It might be a bug made by some optimization process. My wild guess is it’s probably terser. Some optimization is probably messing up the usage of the domain module, and the closure of the callback passed to scope.addEventProcessor is no longer garbage collected, so every request made is leaking a good amount of memory.

I’m also using razzle.js which is a bit behind on the webpack/terser versions, maybe it’s already fixed.

This doesn’t seem to be a bug on sentry’s side anymore. I will continue to investigate this and open an issue where appropriate and keep this thread updated.

@HazAT Thanks, I know that, but is a small price to pay. to solve that huge memory leak, but to minimize this lost I did a few things.

1. I try handle all exceptions I preffer to not rely on the unCoughException/promise events.
2. For breadcrumbs I manually add them as long I have access to the req object…
3. Upload the sourcemaps to sentry with @sentry/webpack-plugin
4. Add Tags/Extra on every request that will help to identify the issue

One more thing, I forgot to paste one more tag I’m adding that is the transaction (to simulate the default Handler)

req.sentry.scope.setTag('transaction', `${req.method}|${req.route ? req.route.path : req.path}`);

I hope this help someone, Sentry its a really great tool and I did what I could to avoid removing it from my stack.

Updating to node 11.12 seems to have stabilized the memory and CPU usage. It seems there isn’t any discernible difference now in resource usage when comparing it to having the handlers disabled, maybe even slightly better. It seems to also catch errors fine with all the info I need (it might have more console “breadcrumbs” which is nice). Not sure what else I can check for 5.0.0. I’ll let you know if I run into any issues, but probably not.

LGTM. Thanks!

@MartijnHols We are currently working on a major release that should significantly reduce the memory footprint our SDK. If you feel adventurous you could try it out https://github.com/getsentry/sentry-javascript/pull/1919

@sunknudsen The issue is actually fixed in NodeJS, see https://github.com/nodejs/node/issues/23862 and https://github.com/nodejs/node/pull/25993. We probably need to wait for a release.

@abraxxas confirmed. Node 10 + ~300req for each resource does the job. Will investigate further. Thanks!

@tstirrat15 5.4.2 with the fix included has been released, give it a try 😃

Might be bit offtopic, but could also cause some leaks that there is no domain cleanup in Handlers.ts, only domain.create? Medium article or SO suggests there should be removeListeners and domain.exit. But did not find some definitive answer to that.

UPDATE: Now I see that handlers are using domain.run which internally calls domain.exit. Still removing the listeners/emitters might make difference, but honestly have no idea.

I have the same problem on node 11.10, but it seems to be fixed on Node 12.3.1

Using lab for unit testing. The leaks are still present. I know leaks can be painful to debug. Is there an ETA for a fix?

1 tests complete Test duration: 1832 ms The following leaks were detected:__extends, __assign, __rest, __decorate, __param, __metadata, __awaiter, __generator, __exportStar, __values, __read, __spread, __await, __asyncGenerator, __asyncDelegator, __asyncValues, __makeTemplateObject, __importStar, __importDefault

npm ERR! code ELIFECYCLE npm ERR! errno 1 npm ERR! theregulars-reviews-backend@0.0.1 test: lab build/test npm ERR! Exit status 1 npm ERR! npm ERR! Failed at the theregulars-reviews-backend@0.0.1 test script. npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in: npm ERR! /Users/sunknudsen/.npm/_logs/2019-02-13T14_41_28_595Z-debug.log

@abraxxas ignore my previous comment (the one I removed), it’s totally on our side 😃

It appears to be the issue in Node’s core itself. Please refer to this comment https://github.com/getsentry/sentry-javascript/issues/1762#issuecomment-444126990

@kamilogorek Thanks for your response. What i did was the following. I checked out this example https://github.com/sheerun/next.js/tree/with-sentry-fix/examples/with-sentry and added memwatch to the server.js

if (!dev) {
  memwatch.on("leak", info => {
    console.log("memwatch::leak");
    console.error(info);
  });

  memwatch.on("stats", stats => {
    console.log("memwatch::stats");
    console.error(Util.inspect(stats, true, null));
  });
}

then i ran the example using node 10.x (with 8.x i observed no memory issues) and requested the following ressources using our gatling testsuite:

    "/",
    "/_next/static/r1zovjaZ1TujaA0hJEp91/pages/_app.js",
    "/_next/static/r1zovjaZ1TujaA0hJEp91/pages/_error.js",
    "/_next/static/r1zovjaZ1TujaA0hJEp91/pages/index.js",
    "/_next/static/runtime/main-1eaa6d1d0c8e7d048efd.js",
    "/_next/static/chunks/commons.b34d260fee0c4a698139.js",
    "/_next/static/runtime/webpack-42652fa8b82c329c0559.js"

(note the hashes might change for you)

but you should be able to achieve the same results with this approach very simple https://www.simonholywell.com/post/2015/06/parallel-benchmark-many-urls-with-apachebench/

after a few thousand requests our memory usage was at almost 1GB and never reduced even when idling. As soon as i remove the request and errorHandler from server.js the memory-leak stops. So it seems to be connected to those 2. Maybe you either had too few requests or used node 8.x?

@abraxxas can you help me reproduce this issue? How exactly are you triggering this memleak? I tried your description with no luck, it just produces stats events, never leak one.