Add synchronization when locking database connection #1200
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1200 +/- ##
==========================================
+ Coverage 75.26% 75.44% +0.17%
==========================================
Files 135 135
Lines 9082 9093 +11
Branches 358 363 +5
==========================================
+ Hits 6836 6860 +24
+ Misses 2246 2233 -13
|
| @@ -99,7 +99,7 @@ object SqliteUtils { | |||
| * | |||
| * The lock will be kept until the database is closed, or if the locking mode is explicitly reset. | |||
| */ | |||
| def obtainExclusiveLock(sqlite: Connection) { | |||
| def obtainExclusiveLock(sqlite: Connection) = synchronized { | |||
There was a problem hiding this comment.
This only fixes the issue if two calls to obtainExclusiveLock happen in the same process, but in the case of a background job running concurrently with the wallet app, this won't fix anything, right?
It's very surprising that sqlite doesn't have a way to atomically get a real exclusive lock or fail...
There was a problem hiding this comment.
This scenario is taken care of by the front application and should not happen. There's a logic that prevents background jobs to run concurrently with the front.
There was a problem hiding this comment.
Ok in that case the synchronized statement should work (and it doesn't hurt to have it anyway).
I still think we can probably do better on the sqlite side with a better understanding of their locking mechanisms, but it can be done later.
| } | ||
|
|
||
| object Databases { | ||
| object Databases extends Logging { |
There was a problem hiding this comment.
It seems like we should try to avoid extending this Logging trait and instead implicitly pass a logger to the functions that need one. Can you try doing that?
There was a problem hiding this comment.
Using grizzled.slf4j.Logging is indeed discouraged, but for code completely unrelated to actors I think it makes sense to keep using it. What we don't want is lose the context when we call code in static methods from within an actor.
This pull request fixes an issue where creating concurrent database connection could deadlock.
Context
An exclusive lock was added to
eclair.sqliteto prevent running several instances of eclair using the same sqlite database. The code obtaining the exclusive lock is a succession of three SQL queries, and is not thread-safe. If two threads try to obtain the lock at the same time, none of these locks will succeed, and subsequent locks will fail because the database is deadlocked.This can happen on the eclair mobile application where several background jobs (that need exclusive lock on the
eclair.sqlitedb) may be launched at the exact same time by the OS due to battery optimization.See:
Proposed change
This PR changes the
SqliteUtils.obtainExclusiveLockmethod into a synchronized statement. Also, if the SQL connections creation fails, db connections will be closed if needed.