Skip to content
This repository was archived by the owner on Oct 23, 2023. It is now read-only.

Fix autoBreadcrumb http instrumentation memory leak #296

Merged
merged 2 commits into from
Mar 16, 2017

Conversation

LewisJEllis
Copy link
Contributor

@LewisJEllis LewisJEllis commented Mar 16, 2017

PR #276/version 1.1.3 introduced a memory leak in our http instrumentation as reported in #295. The core issue was that we were monkeypatching http.ClientRequest.prototype.emit during every ClientRequest instantiation, instead of just during initial instrumentation, which meant that for each outbound http request made, we kept a bunch of stuff around in a closure.

Users on raven-node 1.1.3 or 1.1.4 with long-running processes that made outbound http requests found their memory usage growing steadily over time.

This PR refactors the ClientRequest instrumentation to only monkeypatch http.ClientRequest.prototype.emit once during initial Raven setup when we also monkeypatch the constructor. The monkeypatched emit method previously found the request url by closure reference, but now the constructor attaches a __ravenBreadcrumbUrl property to the request object and emit grabs that instead.

I confirmed the issue and fix using memwatch-next (plus express, some outbound requesting, and some apachebenching) and have ideas for how to use that to automate some "did we intro a memory leak" sanity testing, but that can come later. That effort will have to test multiple different possible causes; the manual memory load test in https://github.com/getsentry/raven-node/blob/master/test/manual/context-memory.js would not have caught this because it only exercises contexts, not each of the different autoBreadcrumb instrumentation options.

This should fix #295. As soon as this merges I'll publish a new version.

/cc @benvinegar @MaxBittker @maxcountryman

@LewisJEllis LewisJEllis requested a review from benvinegar March 16, 2017 03:10
@maxcountryman
Copy link

Nice catch! 👍

Copy link
Contributor

@benvinegar benvinegar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's cut a new patch release.

@idris
Copy link

idris commented Mar 16, 2017

Would love to see automated tests around memory leaks, as you mentioned in the description.

This is the second time Raven introduced a memory leak into my app, with the first being #231

@maxcountryman
Copy link

I would like to second @idris comment with a big plus one: Raven took down my production nodes due to this problem and it would be nice to see provisions against this happening going forward.

@LewisJEllis LewisJEllis merged commit 483b70e into master Mar 16, 2017
@LewisJEllis LewisJEllis deleted the http-instrumentation-memleak branch March 16, 2017 19:32
@oliviertassinari
Copy link
Contributor

@LewisJEllis Thanks for fixing it 🆗

@emmenko
Copy link

emmenko commented Mar 18, 2017

Thanks for the fix, really a nasty one!!! 🎉

FYI: this is how it looked in our grafana dashboards:
image

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

Successfully merging this pull request may close these issues.

Massive memory leak when auto breadcrumbs is enabled
6 participants