Fixes fd leak in leasedb (ticket #2015)

Brian correctly diagnozed this issue and suggested the fix in Weekly Dev
Chat of 2013-07-09. This patch has been manually tested by Zooko using his
fdleakfinder and a unit test has been added to ensure that database connections
are properly closed. Without this patch, the 1819-cloud-merge branch uses
more than 1020 fds and will start failing on operating systems with a low fds
limit.

The portions affecting leasedb.py were written by Zooko and Daira.

diff --git a/src/allmydata/storage/accountant.py b/src/allmydata/storage/accountant.py
index e7c53ccfe6af8d8451c80204512c80340bc10dc3..c2aec865e14f7003a7abf82186131f384abb6af6 100644 (file)
--- a/src/allmydata/storage/accountant.py
+++ b/src/allmydata/storage/accountant.py
@@ -23,6 +23,7 @@ class Accountant(service.MultiService):
         service.MultiService.__init__(self)
         self._storage_server = storage_server
         self._leasedb = LeaseDB(dbfile)
+        self._leasedb.setServiceParent(self)
         self._active_accounts = weakref.WeakValueDictionary()
         self._anonymous_account = Account(LeaseDB.ANONYMOUS_ACCOUNTID, None,
                                           self._storage_server, self._leasedb)

diff --git a/src/allmydata/storage/leasedb.py b/src/allmydata/storage/leasedb.py
index 02524e783a101db66ae40a2b5b403f5d039ee03e..ad3bbfac9df0cdc834ab423750788c914ecbbcfe 100644 (file)
--- a/src/allmydata/storage/leasedb.py
+++ b/src/allmydata/storage/leasedb.py
@@ -5,6 +5,8 @@ from allmydata.util.assertutil import _assert
 from allmydata.util import dbutil
 from allmydata.storage.common import si_b2a
 
+from twisted.application import service
+
 
 class NonExistentShareError(Exception):
     def __init__(self, si_s, shnum):
@@ -113,27 +115,41 @@ CREATE UNIQUE INDEX `cycle` ON `crawler_history` (`cycle`);
 DAY = 24*60*60
 MONTH = 30*DAY
 
-class LeaseDB:
+class LeaseDB(service.Service):
     ANONYMOUS_ACCOUNTID = 0
     STARTER_LEASE_ACCOUNTID = 1
     STARTER_LEASE_DURATION = 2*MONTH
 
     def __init__(self, dbfile):
-        # synchronous = OFF is necessary for leasedb to pass tests for the time being,
-        # since using synchronous = NORMAL causes failures that are apparently due to
-        # a file descriptor leak, and the default synchronous = FULL causes the tests
-        # to time out. For discussion see
-        # https://tahoe-lafs.org/pipermail/tahoe-dev/2012-December/007877.html
-
-        (self._sqlite,
-         self._db) = dbutil.get_db(dbfile, create_version=(LEASE_SCHEMA_V1, 1),
-                                   # journal_mode="WAL",
-                                   synchronous="OFF")
-        self._cursor = self._db.cursor()
         self.debug = False
         self.retained_history_entries = 10
+        self._dbfile = dbfile
+        self._db = None
+        self._open_db()
+
+    def _open_db(self):
+        if self._db is None:
+            # For the reasoning behind WAL and NORMAL, refer to
+            # <https://tahoe-lafs.org/pipermail/tahoe-dev/2012-December/007877.html>.
+            (self._sqlite,
+             self._db) = dbutil.get_db(self._dbfile, create_version=(LEASE_SCHEMA_V1, 1),
+                                       journal_mode="WAL",
+                                       synchronous="NORMAL")
+            self._cursor = self._db.cursor()
+
+    def _close_db(self):
+        try:
+            self._cursor.close()
+        finally:
+            self._cursor = None
+        self._db.close()
+        self._db = None
+
+    def startService(self):
+        self._open_db()
 
-    # share management
+    def stopService(self):
+        self._close_db()
 
     def get_shares_for_prefix(self, prefix):
         """

diff --git a/src/allmydata/test/test_leasedb.py b/src/allmydata/test/test_leasedb.py
index ac09b98e81be35abadf8ee26f400e64c5da2b45e..d95524dc8d3e7aa8632f95dc9f458f961d41f196 100644 (file)
--- a/src/allmydata/test/test_leasedb.py
+++ b/src/allmydata/test/test_leasedb.py
@@ -4,6 +4,7 @@ import os
 from twisted.trial import unittest
 
 from allmydata.util import fileutil
+from allmydata.util import dbutil
 from allmydata.util.dbutil import IntegrityError
 from allmydata.storage.leasedb import LeaseDB, LeaseInfo, NonExistentShareError, \
      SHARETYPE_IMMUTABLE
@@ -21,15 +22,18 @@ class DB(unittest.TestCase):
     def test_create(self):
         dbfilename = self.make("create")
         l = LeaseDB(dbfilename)
+        l.startService()
         self.failUnlessEqual(set(l.get_all_accounts()), BASE_ACCOUNTS)
 
         # should be able to open an existing one too
         l2 = LeaseDB(dbfilename)
+        l2.startService()
         self.failUnlessEqual(set(l2.get_all_accounts()), BASE_ACCOUNTS)
 
     def test_basic(self):
         dbfilename = self.make("create")
         l = LeaseDB(dbfilename)
+        l.startService()
 
         l.add_new_share('si1', 0, 12345, SHARETYPE_IMMUTABLE)
 
@@ -81,3 +85,43 @@ class DB(unittest.TestCase):
         self.failUnlessRaises(IntegrityError, l._cursor.execute,
                               "INSERT INTO `leases` VALUES(?,?,?,?,?)",
                               ('si1', 0,  LeaseDB.ANONYMOUS_ACCOUNTID, 0, 0))
+
+class MockCursor:
+    def __init__(self):
+        self.closed = False
+
+    def close(self):
+        self.closed = True
+
+class MockDB:
+    def __init__(self):
+        self.closed = False
+
+    def cursor(self):
+        return MockCursor()
+
+    def close(self):
+        self.closed = True
+
+class FD_Leak(unittest.TestCase):
+
+    def create_leasedb(self, testname):
+        basedir = os.path.join("leasedb", "FD_Leak", testname)
+        fileutil.make_dirs(basedir)
+        dbfilename = os.path.join(basedir, "leasedb.sqlite")
+        return dbfilename
+
+    def test_basic(self):
+        # This test ensures that the db connection is closed by leasedb after
+        # the service stops.
+        def _call_get_db(*args, **kwargs):
+            return None, MockDB()
+        self.patch(dbutil, 'get_db', _call_get_db)
+        dbfilename = self.create_leasedb("test_basic")
+        l = LeaseDB(dbfilename)
+        l.startService()
+        db = l._db
+        cursor = l._cursor
+        l.stopService()
+        self.failUnless(db.closed)
+        self.failUnless(cursor.closed)