[PATCH] Fix memory leak caused by libvirt connection wrapper

[Mark Wu]

This patch changes to apply the wrapper on class method instead of
instance method to avoid the circular refrence caused by wrapping
instance method. Without this fix, it's observed the libvirt object
like 'virDomain' are leaked and the memory usage of kimchi grow rapidly.

Signed-off-by: Mark Wu <wu...@linux.vnet.ibm.com>
---
src/kimchi/model.py | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/kimchi/model.py b/src/kimchi/model.py
index 2ecbc9c..73c18ac 100644
--- a/src/kimchi/model.py
+++ b/src/kimchi/model.py
@@ -1529,11 +1529,6 @@ class LibvirtConnection(object):
def wrapper(*args, **kwargs):
try:
ret = f(*args, **kwargs)
- if isinstance(ret, self.wrappables):
- for name in dir(ret):
- method = getattr(ret, name)
- if callable(method) and not name.startswith('_'):
- setattr(ret, name, wrapMethod(method))
return ret
except libvirt.libvirtError as e:
edom = e.get_error_domain()
@@ -1577,6 +1572,12 @@ class LibvirtConnection(object):
if callable(method) and not name.startswith('_'):
setattr(conn, name, wrapMethod(method))

+ for cls in self.wrappables:
+ for name in dir(cls):
+ method = getattr(cls, name)
+ if callable(method) and not name.startswith('_'):
+ setattr(cls, name, wrapMethod(method))
+
self._connections[conn_id] = conn
# In case we're running into troubles with keeping the connections
# alive we should place here:
--
1.8.3.1

[Shu Ming]

Filed issue for this patch.

https://github.com/kimchi-project/kimchi/issues/283 

[Zhou Zheng Sheng]

on 2013/12/16 23:10, Mark Wu wrote:
> This patch changes to apply the wrapper on class method instead of
> instance method to avoid the circular refrence caused by wrapping

refrence -> reference

> instance method. Without this fix, it's observed the libvirt object
> like 'virDomain' are leaked and the memory usage of kimchi grow rapidly.
>

As Shao He suggests, this patch solves issues 283. Maybe you use add
"Issue 283, ..." as the commit message title.

As far as I know. Python can garbage collect circular referenced
objects. It only fails to do so when the object contains a __del__()
method. This is just the case for virDomain objects. Maybe you can add a
reference link in your commit message.
http://docs.python.org/2/library/gc.html#gc.garbage

If it is possible, it's also good to describe how the circular reference
is formed and how this patch breaks it. It's not obvious to me from the
first glance of the code. Is it because of this reference circle?

obj.f -> wrapper -> instance method (original f) in the wrapper's
closure -> implicit argument obj of an instance method f -> obj

After break the circle.

cls.f -> wrapper -> class method (original f) in the wrapper's closure
-> no more references

While I think this patch is good, there is another possibility to solve
the problem by using weakref. Which do you prefer?

--
Thanks and best regards!

Zhou Zheng Sheng / 周征晟
E-mail: zhsh...@linux.vnet.ibm.com
Telephone: 86-10-82454397

[Royce Lv]

Reviewed-by: Royce Lv<lvr...@linux.vnet.ibm.com>

This is really brilliant!

[Zhou Zheng Sheng]

It seems weakref can also solve the problem. I tried the following.

1 import weakref
2 from functools import wraps
3 import gc
4 import types
5
6
7 class Circular(object):
8 def foo(self, x):
9 print x
10
11 def __del__(self):
12 pass
13
14
15 def wrap_foo(f):
16 clsM = getattr(f.im_class, f.__name__)
17 f = types.MethodType(
18 clsM, weakref.proxy(f.__self__), f.im_class)
19 @wraps(f)
20 def wrapper(*args, **kwargs):
21 print 'wrapper'
22 return f(*args, **kwargs)
23 return wrapper
24
25
26 c = Circular()
27 c.foo = wrap_foo(c.foo)
28 c.foo(3)
29
30 print c
31 print gc.garbage
32 del c
33 gc.collect()
34 print gc.garbage

Without line 16-18, the second "print gc.garbage" prints c. With line
16-18, gc.garbage contains nothing that can not be collected
automatically. So adding line 16-18 to bellow the original "def
wrapMethod(f):" should work.

def wrapMethod(f):
clsM = getattr(f.im_class, f.__name__)
f = types.MethodType(
clsM, weakref.proxy(f.__self__), f.im_class)

I think your patch is good. I'm just interested in this issue and tried
another way. You can ignore my comments about weakref..

[Mark Wu]

Thanks a lot for explaining the bug and fix. I am going to make a
better commit message in patch v2.

I prefer my fix because your fix make it even difficult to understand.

Mark.

[Sheldon]

zhengsheng,  your example is interesting, and it is good to explain the circular refrence .
       

Sheldon Feng(冯少合)
IBM Linux Technology Center