Merge lp:~cjwatson/txpkgupload/fix-leak into lp:~lazr-developers/txpkgupload/trunk

[Colin Watson]

Fix memory leak when closing SFTP connections.

Yesterday evening there was an outage due to txpkgupload having leaked to the point where it was using rather more than 4GB of memory. I looked into this a bit today and found that I could make it leak just by repeatedly initiating SFTP connections. This is happening because it registers a global handler for SFTPClosed on every connection and never unregisters it. Rather than going through the component registry for this, it seems easier to just subclass FileTransferServer, and with this change memory use remains stable when hammering the server with consecutive no-op SFTP connections in a loop.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'src/txpkgupload/plugin.py'
--- src/txpkgupload/plugin.py	2015-01-21 12:50:43 +0000
+++ src/txpkgupload/plugin.py	2015-01-22 11:33:13 +0000
@@ -43,7 +43,10 @@
     ReadyService,
     )
 from txpkgupload.twistedftp import FTPServiceFactory
-from txpkgupload.twistedsftp import SFTPServer
+from txpkgupload.twistedsftp import (
+    PkgUploadFileTransferServer,
+    SFTPServer,
+    )
 
 
 # Ensure that formencode does not translate strings; there are encoding issues
@@ -155,6 +158,7 @@
         LaunchpadAvatar.__init__(self, user_dict)
         self.fs_root = fs_root
         self.temp_dir = temp_dir
+        self.subsystemLookup["sftp"] = PkgUploadFileTransferServer
 
 
 class Realm:

=== modified file 'src/txpkgupload/twistedsftp.py'
--- src/txpkgupload/twistedsftp.py	2015-01-21 12:59:40 +0000
+++ src/txpkgupload/twistedsftp.py	2015-01-22 11:33:13 +0000
@@ -13,16 +13,14 @@
 import os
 import tempfile
 
-from lazr.sshserver.events import SFTPClosed
-from lazr.sshserver.sftp import FileIsADirectory
+from lazr.sshserver.sftp import (
+    FileIsADirectory,
+    FileTransferServer,
+    )
 from twisted.conch.interfaces import (
     ISFTPFile,
     ISFTPServer,
     )
-from zope.component import (
-    adapter,
-    provideHandler,
-    )
 from zope.interface import implements
 
 from txpkgupload.filesystem import UploadFileSystem
@@ -35,15 +33,12 @@
     implements(ISFTPServer)
 
     def __init__(self, avatar):
-        provideHandler(self.connectionClosed)
         self._avatar = avatar
         self._fs_root = avatar.fs_root
         self.uploadfilesystem = UploadFileSystem(
             tempfile.mkdtemp(dir=avatar.temp_dir))
         self._current_upload = self.uploadfilesystem.rootpath
         os.chmod(self._current_upload, 0770)
-        self.hook = Hooks(self._fs_root, perms='g+rws', prefix='-sftp')
-        self.hook.new_client_hook(self._current_upload, 0, 0)
 
     def gotVersion(self, other_version, ext_data):
         return {}
@@ -101,12 +96,6 @@
         return self.uploadfilesystem._full(
             self.uploadfilesystem._sanitize(filename))
 
-    @adapter(SFTPClosed)
-    def connectionClosed(self, event):
-        if event.avatar is not self._avatar:
-            return
-        self.hook.client_done_hook(self._current_upload, 0, 0)
-
 
 class SFTPFile:
 
@@ -138,3 +127,16 @@
 
     def setAttrs(self, attr):
         pass
+
+
+class PkgUploadFileTransferServer(FileTransferServer):
+
+    def __init__(self, data=None, avatar=None):
+        FileTransferServer.__init__(self, data=data, avatar=avatar)
+        self.hook = Hooks(self.client._fs_root, perms='g+rws', prefix='-sftp')
+        self.hook.new_client_hook(self.client._current_upload, 0, 0)
+
+    def connectionLost(self, reason):
+        if self.hook is not None:
+            self.hook.client_done_hook(self.client._current_upload, 0, 0)
+            self.hook = None