sqlite: properly serialize operations on the databases

1. There is one database per folder and sqlite requires to serialize the
writings. Instead of locking at LocalStatusSQLiteFolder instance level,
introduce a new DatabaseFileLock object which is shared across threads. This
fixes the concurrent writes issues that some users might experience by
duplications or flags restored to the previous state.

2. Close the database only when we are sure no other threads will use the
connection on a *per-file* basis. Previous fix 677afb8d8f is wrong
because the same lock is shared for all the database files.

Github-fix: https://github.com/OfflineIMAP/offlineimap/issues/350
Signed-off-by: Nicolas Sebrecht <nicolas.s-dev@laposte.net>
This commit is contained in:
Nicolas Sebrecht 2016-07-25 14:39:11 +02:00
parent 5aa2a883f0
commit 3c42913120
3 changed files with 66 additions and 40 deletions

View File

@ -425,7 +425,8 @@ class SyncableAccount(Account):
def syncfolder(account, remotefolder, quick): def syncfolder(account, remotefolder, quick):
"""Synchronizes given remote folder for the specified account. """Synchronizes given remote folder for the specified account.
Filtered folders on the remote side will not invoke this function.""" Filtered folders on the remote side will not invoke this function. However,
this might be called in a concurrently."""
def check_uid_validity(localfolder, remotefolder, statusfolder): def check_uid_validity(localfolder, remotefolder, statusfolder):
# If either the local or the status folder has messages and # If either the local or the status folder has messages and
@ -489,8 +490,7 @@ def syncfolder(account, remotefolder, quick):
might not correspond. But, if we're cloning a folder into a new one, might not correspond. But, if we're cloning a folder into a new one,
[min_uid, ...] does correspond to [1, ...]. [min_uid, ...] does correspond to [1, ...].
This is just for IMAP-IMAP. For Maildir-IMAP, use maxage instead. This is just for IMAP-IMAP. For Maildir-IMAP, use maxage instead."""
"""
new.cachemessagelist() new.cachemessagelist()
min_uid = partial.retrieve_min_uid() min_uid = partial.retrieve_min_uid()

View File

@ -16,13 +16,39 @@
# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
import os import os
import six
import sqlite3 as sqlite import sqlite3 as sqlite
from sys import exc_info from sys import exc_info
from threading import Lock from threading import Lock
import six
from .Base import BaseFolder from .Base import BaseFolder
class DatabaseFileLock(object):
"""Lock at database file level."""
def __init__(self):
self._lock = Lock()
self._counter = 0
def __enter__(self):
self._lock.acquire()
def __exit__(self, typ, value, tb):
self._lock.release()
def registerNewUser(self):
self._counter += 1
def removeOneUser(self):
self._counter -= 1
def getLock(self):
return self._lock
def shouldClose(self):
return self._counter < 1
class LocalStatusSQLiteFolder(BaseFolder): class LocalStatusSQLiteFolder(BaseFolder):
"""LocalStatus backend implemented with an SQLite database """LocalStatus backend implemented with an SQLite database
@ -42,11 +68,10 @@ class LocalStatusSQLiteFolder(BaseFolder):
# Current version of our db format. # Current version of our db format.
cur_version = 2 cur_version = 2
# Keep track on how many threads need access to the database. # Keep track on how many threads need access to the database.
threads_open_count = 0 locks = {} # Key: filename, value: DatabaseFileLock instance.
threads_open_lock = Lock()
def __init__(self, name, repository): def __init__(self, name, repository):
self.sep = '.' # Needs to be set before super.__init__() self.sep = '.' # Needs to be set before super().__init__().
super(LocalStatusSQLiteFolder, self).__init__(name, repository) super(LocalStatusSQLiteFolder, self).__init__(name, repository)
self.root = repository.root self.root = repository.root
self.filename = os.path.join(self.getroot(), self.getfolderbasename()) self.filename = os.path.join(self.getroot(), self.getfolderbasename())
@ -60,19 +85,24 @@ class LocalStatusSQLiteFolder(BaseFolder):
raise UserWarning("SQLite database path '%s' is not a directory."% raise UserWarning("SQLite database path '%s' is not a directory."%
dirname) dirname)
self.connection = None
# This lock protects against concurrent writes in same connection. # This lock protects against concurrent writes in same connection.
self._dblock = Lock() self._dblock = Lock()
self.connection = None
def openfiles(self): def openfiles(self):
# Protect the creation/upgrade of database accross threads. # Make sure sqlite is in multithreading SERIALIZE mode.
with LocalStatusSQLiteFolder.threads_open_lock: assert sqlite.threadsafety == 1, 'Your sqlite is not multithreading safe.'
# Try to establish connection, no need for threadsafety in __init__.
# Protect the creation/upgrade of database accross threads.
if self.filename not in LocalStatusSQLiteFolder.locks:
LocalStatusSQLiteFolder.locks[self.filename] = DatabaseFileLock()
databaseFileLock = LocalStatusSQLiteFolder.locks[self.filename]
with databaseFileLock.getLock():
# Try to establish connection, no need for threadsafety in __init__.
try: try:
self.connection = sqlite.connect(self.filename, self.connection = sqlite.connect(self.filename,
check_same_thread=False) check_same_thread=False)
LocalStatusSQLiteFolder.threads_open_count += 1 databaseFileLock.registerNewUser()
except sqlite.OperationalError as e: except sqlite.OperationalError as e:
# Operation had failed. # Operation had failed.
six.reraise(UserWarning, six.reraise(UserWarning,
@ -83,9 +113,6 @@ class LocalStatusSQLiteFolder(BaseFolder):
(self.filename, e)), (self.filename, e)),
exc_info()[2]) exc_info()[2])
# Make sure sqlite is in multithreading SERIALIZE mode.
assert sqlite.threadsafety == 1, 'Your sqlite is not multithreading safe.'
# Test if db version is current enough and if db is readable. # Test if db version is current enough and if db is readable.
try: try:
cursor = self.connection.execute( cursor = self.connection.execute(
@ -118,30 +145,30 @@ class LocalStatusSQLiteFolder(BaseFolder):
def isnewfolder(self): def isnewfolder(self):
return self._newfolder return self._newfolder
def __sql_write(self, sql, vars=None, executemany=False): def __sql_write(self, sql, args=None, executemany=False):
"""Execute some SQL, retrying if the db was locked. """Execute some SQL, retrying if the db was locked.
:param sql: the SQL string passed to execute() :param sql: the SQL string passed to execute()
:param vars: the variable values to `sql`. E.g. (1,2) or {uid:1, :param args: the variable values to `sql`. E.g. (1,2) or {uid:1,
flags:'T'}. See sqlite docs for possibilities. flags:'T'}. See sqlite docs for possibilities.
:param executemany: bool indicating whether we want to :param executemany: bool indicating whether we want to
perform conn.executemany() or conn.execute(). perform conn.executemany() or conn.execute().
:returns: the Cursor() or raises an Exception""" :returns: None or raises an Exception."""
success = False success = False
while not success: while not success:
self._dblock.acquire()
try: try:
if vars is None: with LocalStatusSQLiteFolder.locks[self.filename].getLock():
if args is None:
if executemany: if executemany:
cursor = self.connection.executemany(sql) self.connection.executemany(sql)
else: else:
cursor = self.connection.execute(sql) self.connection.execute(sql)
else: else:
if executemany: if executemany:
cursor = self.connection.executemany(sql, vars) self.connection.executemany(sql, args)
else: else:
cursor = self.connection.execute(sql, vars) self.connection.execute(sql, args)
success = True success = True
self.connection.commit() self.connection.commit()
except sqlite.OperationalError as e: except sqlite.OperationalError as e:
@ -152,9 +179,6 @@ class LocalStatusSQLiteFolder(BaseFolder):
success = False success = False
else: else:
raise raise
finally:
self._dblock.release()
return cursor
def __upgrade_db(self, from_ver): def __upgrade_db(self, from_ver):
"""Upgrade the sqlite format from version 'from_ver' to current""" """Upgrade the sqlite format from version 'from_ver' to current"""
@ -229,9 +253,10 @@ class LocalStatusSQLiteFolder(BaseFolder):
self.messagelist[uid]['mtime'] = row[2] self.messagelist[uid]['mtime'] = row[2]
def closefiles(self): def closefiles(self):
with LocalStatusSQLiteFolder.threads_open_lock: databaseFileLock = LocalStatusSQLiteFolder.locks[self.filename]
LocalStatusSQLiteFolder.threads_open_count -= 1 with databaseFileLock.getLock():
if self.threads_open_count < 1: databaseFileLock.removeOneUser()
if databaseFileLock.shouldClose():
try: try:
self.connection.close() self.connection.close()
except: except:

View File

@ -95,6 +95,7 @@ class LocalStatusRepository(BaseRepository):
# data. This might happen if the user removed the folder (maildir) and # data. This might happen if the user removed the folder (maildir) and
# it is re-created afterwards. # it is re-created afterwards.
folder.purge() folder.purge()
folder.openfiles()
folder.save() folder.save()
folder.closefiles() folder.closefiles()