GNOME Bugzilla – Bug 693779
EBookBackendSqliteDB: Fix concurrency protection
Last modified: 2013-09-14 16:55:54 UTC
Created attachment 236030 [details] [review] Patch to fix concurrent access to EBookBackendSqliteDB This file has seen many revisions, and as the sqlitedb is usually never used concurrently, it seems these problems have not shown up (until direct read access was tried). An outline of the problems: o Judging from the usage of GRWLock; this code assumes that multiple concurrent reads can be performed on the same SQLite connection (not exactly true, sqlite3_mutex_* functions are provided to help this), however SQLite does support multiple connections concurrently reading the same database. o In the start/commit/rollback transaction code, the critical sections are not protected from concurrent access at all. The attached patch protects critical sections in the SQLite DB in a more traditional way, API entry points hold a single mutex (protecting both reads and writes). Any internal calls to public apis call a _locked() version of the API. NOTE: Ideally we should get rid of the db_connections hash table and lock, but because it's possible to store multiple addressbooks in the same SQLite, we need to avoid this until we can remove the "folderid" argument on all APIs. ... This is because we dont want to have multiple SQLite connections /writing/ to the same DB concurrently.
Review of attachment 236030 [details] [review]: ::: addressbook/libedata-book/e-book-backend-sqlitedb.c @@ +405,1 @@ if (ebsdb->priv->in_transaction == 1) { it would be good to write here that the in_transaction is guarded by the priv->lock, which is already held, or make the priv->lock a recursive mutex, which will also guard from deadlocks, once the functions will be called recursively. @@ +1048,3 @@ ebsdb->priv->hash_key = g_strdup (hash_key); +exit: write space in front of the label, the diff then generates proper function name @@ +1344,3 @@ + LOCK_MUTEX (&ebsdb->priv->updates_lock); + + success = book_backend_sqlitedb_start_transaction (ebsdb, error); Who does held the priv->lock here? It can be called from anywhere, right? And beware of locking order. @@ +1361,3 @@ book_backend_sqlitedb_commit_transaction (ebsdb, error) : book_backend_sqlitedb_rollback_transaction (ebsdb, error); + Who does held the priv->lock here? It can be called from anywhere, right? And beware of locking order. @@ +3294,3 @@ + else + /* The GError is already set. */ + book_backend_sqlitedb_rollback_transaction (ebsdb, NULL); This may always fail, if the book_backend_sqlitedb_start_transaction() failed @@ +3386,3 @@ + else + /* The GError is already set. */ + book_backend_sqlitedb_rollback_transaction (ebsdb, NULL); This may always fail, if the book_backend_sqlitedb_start_transaction() failed @@ +3466,3 @@ + else + /* The GError is already set. */ + book_backend_sqlitedb_rollback_transaction (ebsdb, NULL); This may always fail, if the book_backend_sqlitedb_start_transaction() failed @@ +3550,3 @@ + else + /* The GError is already set. */ + book_backend_sqlitedb_rollback_transaction (ebsdb, NULL); This may always fail, if the book_backend_sqlitedb_start_transaction() failed @@ +3625,3 @@ + else + /* The GError is already set. */ + book_backend_sqlitedb_rollback_transaction (ebsdb, NULL); This may always fail, if the book_backend_sqlitedb_start_transaction() failed @@ +3704,3 @@ + else + /* The GError is already set. */ + book_backend_sqlitedb_rollback_transaction (ebsdb, NULL); This may always fail, if the book_backend_sqlitedb_start_transaction() failed
Created attachment 236034 [details] [review] Patch to fix concurrent access to EBookBackendSqliteDB (rev 2) New patch to address comments in review
Review of attachment 236034 [details] [review]: Thanks. Not tested, but looks fine on the first look. Please commit to master.
Comment on attachment 236034 [details] [review] Patch to fix concurrent access to EBookBackendSqliteDB (rev 2) Thanks, committed and pushed.