Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add synchronization when locking database connection #1200

Merged
merged 2 commits into from
Dec 12, 2019
Merged

Add synchronization when locking database connection #1200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
951a2aa
Exclusive lock on eclair.sqlite method is now synchronized
dpad85 Nov 4, 2019
27f94cc
Close SQL connections if connection creation fails
dpad85 Nov 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions eclair-core/src/main/scala/fr/acinq/eclair/db/Databases.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import java.io.File
import java.sql.{Connection, DriverManager}

import fr.acinq.eclair.db.sqlite._
import grizzled.slf4j.Logging
import org.sqlite.SQLiteException

trait Databases {

Expand All @@ -35,24 +37,39 @@ trait Databases {

val pendingRelay: PendingRelayDb

def backup(file: File) : Unit
def backup(file: File): Unit
}

object Databases {
object Databases extends Logging {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Given a parent folder it creates or loads all the databases from a JDBC connection
*
* @param dbdir
* @return
*/
def sqliteJDBC(dbdir: File): Databases = {
dbdir.mkdir()
val sqliteEclair = DriverManager.getConnection(s"jdbc:sqlite:${new File(dbdir, "eclair.sqlite")}")
val sqliteNetwork = DriverManager.getConnection(s"jdbc:sqlite:${new File(dbdir, "network.sqlite")}")
val sqliteAudit = DriverManager.getConnection(s"jdbc:sqlite:${new File(dbdir, "audit.sqlite")}")
SqliteUtils.obtainExclusiveLock(sqliteEclair) // there should only be one process writing to this file
var sqliteEclair: Connection = null
var sqliteNetwork: Connection = null
var sqliteAudit: Connection = null
try {
sqliteEclair = DriverManager.getConnection(s"jdbc:sqlite:${new File(dbdir, "eclair.sqlite")}")
sqliteNetwork = DriverManager.getConnection(s"jdbc:sqlite:${new File(dbdir, "network.sqlite")}")
sqliteAudit = DriverManager.getConnection(s"jdbc:sqlite:${new File(dbdir, "audit.sqlite")}")
SqliteUtils.obtainExclusiveLock(sqliteEclair) // there should only be one process writing to this file
logger.info("successful lock on eclair.sqlite")
databaseByConnections(sqliteAudit, sqliteNetwork, sqliteEclair)
} catch {
case t: Throwable => {
logger.error("could not create connection to sqlite databases: ", t)
if (sqliteEclair != null) sqliteEclair.close()
if (sqliteNetwork != null) sqliteNetwork.close()
if (sqliteAudit != null) sqliteAudit.close()
throw t
}
}

databaseByConnections(sqliteAudit, sqliteNetwork, sqliteEclair)
}

def databaseByConnections(auditJdbc: Connection, networkJdbc: Connection, eclairJdbc: Connection) = new Databases {
Expand All @@ -62,9 +79,10 @@ object Databases {
override val peers = new SqlitePeersDb(eclairJdbc)
override val payments = new SqlitePaymentsDb(eclairJdbc)
override val pendingRelay = new SqlitePendingRelayDb(eclairJdbc)

override def backup(file: File): Unit = {
SqliteUtils.using(eclairJdbc.createStatement()) {
statement => {
statement => {
statement.executeUpdate(s"backup to ${file.getAbsolutePath}")
}
}
Expand Down
2 changes: 1 addition & 1 deletion eclair-core/src/main/scala/fr/acinq/eclair/db/sqlite/SqliteUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val statement = sqlite.createStatement()
statement.execute("PRAGMA locking_mode = EXCLUSIVE")
// we have to make a write to actually obtain the lock
Expand Down