Skip to content

Commit

Permalink
monkeypatch to fix memory leak in memcache library
Browse files Browse the repository at this point in the history 
Instantiating a memcache.Client object will create a memcache._Host
object that stores the debuglog method of the Client object. That
_Host object gets stored in the original Client object, presumably
causing that memory to get lost in time and space forever. The
workaround is to set Client.debuglog to None so that _Host won't
try to store anything, avoiding the cyclic reference. (#278)
  • Loading branch information
Carlo Cabanilla committed Dec 27, 2012
1 parent 8afe3a6 commit d3bbd1f
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions checks/db/mcache.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,13 @@ def check(self, agentConfig):
self.logger.exception("Cannot import python-based memcache driver")
return False

# Hacky monkeypatch to fix a memory leak in the memcache library.
# See https://github.com/DataDog/dd-agent/issues/278 for details.
try:
memcache.Client.debuglog = None
except:
pass

for i in range(len(memcache_urls)):
server = memcache_urls[i]
if server is None:
Expand Down

3 comments on commit d3bbd1f

@dcrosta
Copy link
Contributor

Choose a reason for hiding this comment

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

As of at least python-memcached version 1.31 and 1.48 (can't find the repository for that, though it is on PyPI) there doesn't appear to be a cycle -- and moreover, since debuglog is meant to be a function, this might actually be introducing a bug. I guess datadog agent never sets debug=True?

Note that this might cause problems if custom checks using memcached with debug=True run in-process (I can't recall whether they are run in subprocs or not), or if someone's application code imports this module and runs check() (a little hard to imagine why anyone would do that though).

@clofresh
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I checked where Client.debuglog might get called, it's only on pickling errors or unknown flag errors, so most likely a small chance of that happening, since we're only using the get stats method. If someone has a custom check that uses the memcache library, they might get it, but it would just mean they'd get stack trace pointing to a weird place instead of silently logging those errors.

The bug must have been fixed in 1.48. I tested on Ubuntu 10.04 and CentOS 6's python-memcache packages, which provide 1.44 and 1.43 respectively, using this test: https://gist.github.com/4407618

In the future we may just drop the memcache client dependency and write a simple client to just make the stats call. Less dependencies == a good thing.

@dcrosta
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird -- so they added and then later removed (by 1.48) the cycle? 🤷

Please sign in to comment.