From 78d8ed2e05dd4e313211260274db933edb72c699 Mon Sep 17 00:00:00 2001 From: Mark Berger Date: Fri, 12 Jul 2013 11:09:04 -0400 Subject: [PATCH] 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. --- src/allmydata/storage/accountant.py | 1 + src/allmydata/storage/leasedb.py | 42 ++++++++++++++++++--------- src/allmydata/test/test_leasedb.py | 44 +++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 13 deletions(-) diff --git a/src/allmydata/storage/accountant.py b/src/allmydata/storage/accountant.py index e7c53ccf..c2aec865 100644 --- 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 02524e78..ad3bbfac 100644 --- 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 + # . + (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 ac09b98e..d95524dc 100644 --- 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) -- 2.45.2