Fix a memory leak with the new in-database SVN cert storage.
Review Request #3494 — Created Nov. 12, 2012 and submitted
Fix a memory leak with the new in-database SVN cert storage. The new SVN cert support introduced a memory leak due to a circular dependency. The client was holding on to a reference to the function on SVNTool, while SVNTool was holding on to the client, preventing either from being cleaned up. An attempt was made at inlining the function, but as that accessed data bound to the class, the circular dependency remained. This appears to be an issue specifically with PySVN, as an independent repro case could be devised for pysvn.Client's callbacks, but not those from a mock object. The callback is now defined as a classmethod and the only attribute it needs, a repository, is set up as a weakptr which is then accessed by a lambda calling the callback. This prevents the repository and SVNTool from remaining around indefinitely.
Created a repro case that demonstrated the behavior: #### import pysvn class A(object): pass class MyClient(object): def __init__(self): self.b = A() self.client = pysvn.Client() self.client.callback_ssl_server_trust_prompt = self.my_prompt def my_prompt(self, trust_dict): return self.b, trust_dict, False for i in xrange(1, 1000000): client = MyClient() #### And then verified the fix with: #### import pysvn import weakref class A(object): pass class MyClient(object): def __init__(self): self.b = A() ref = weakref.ref(self.b) self.client = pysvn.Client() self.client.callback_ssl_server_trust_prompt = \ lambda x: MyClient.my_prompt(x, ref()) @classmethod def my_prompt(cls, trust_dict, b): return b, trust_dict, False for i in xrange(1, 1000000): client = MyClient() #### Also tested variants with and without a lambda. Unit tests pass.
- Change Summary:
-
Discovered that this fix actually made things worse. Slept on it and came up with a new fix, which I can verify does solve the problem.
- Summary:
-
Fix a circular dependency for the SVN cert prompt callback.Fix a memory leak with the new in-database SVN cert storage.
- Description:
-
~ Fix a circular dependency for the SVN cert prompt callback.
~ Fix a memory leak with the new in-database SVN cert storage.
~ The SVN cert prompt callback assignment was causing a circular
~ dependency. The client was holding on to a reference to the function on ~ The new SVN cert support introduced a memory leak due to a circular
~ dependency. - SVNTool, while SVNTool was holding on to the client, preventing either - from being cleaned up. ~ The callback is now defined inline, breaking the dependency.
~ The client was holding on to a reference to the function on SVNTool,
+ while SVNTool was holding on to the client, preventing either from being + cleaned up. + + An attempt was made at inlining the function, but as that accessed data
+ bound to the class, the circular dependency remained. + + This appears to be an issue specifically with PySVN, as an independent
+ repro case could be devised for pysvn.Client's callbacks, but not those + from a mock object. + + The callback is now defined as a classmethod and the only attribute it
+ needs, a repository, is set up as a weakptr which is then accessed by a + lambda calling the callback. This prevents the repository and SVNTool + from remaining around indefinitely. - Testing Done:
-
~ Unit tests still pass. Deployed to r.rb.o and watching memory usage.
~ Created a repro case that demonstrated the behavior:
+ + + + import pysvn
+ + + + class A(object):
+ pass + + class MyClient(object):
+ def init(self): + self.b = A() + self.client = pysvn.Client() + self.client.callback_ssl_server_trust_prompt = self.my_prompt + + def my_prompt(self, trust_dict):
+ return self.b, trust_dict, False
+ + + for i in xrange(1, 1000000):
+ client = MyClient() + + + + + + And then verified the fix with:
+ + + + + + import pysvn
+ import weakref + + + + class A(object):
+ pass + + class MyClient(object):
+ def init(self): + self.b = A() + ref = weakref.ref(self.b) + + self.client = pysvn.Client()
+ self.client.callback_ssl_server_trust_prompt = \
+ lambda x: MyClient.my_prompt(x, ref())
+ + @classmethod
+ def my_prompt(cls, trust_dict, b):
+ return b, trust_dict, False
+ + + for i in xrange(1, 1000000):
+ client = MyClient() + + + + + + Also tested variants with and without a lambda.
+ + Unit tests pass.
- Diff:
-
Revision 2 (+15 -3)