Db conf on pr 1877 (#1879)

* Extend StartStopAsync with BitcoinSAppConfig, create 'CachedAppConfig' test trait, clean up P2PClientTest * Start cleaning up after ourselves in the chainTest test suite * Call .stop() for appConfig's spun up in ChainAppConfigTest * Database configuration defaults * increase number of Postgres connections * add more logging * close connections pools in tests * update afterAll() * Fix conflict Co-authored-by: rorp <rorp@users.noreply.github.com>
bitcoin-s · Aug 24, 2020 · f10b0e1 · f10b0e1
1 parent 4b675db
commit f10b0e1
Show file tree

Hide file tree

Showing 11 changed files with 174 additions and 41 deletions.
diff --git a/db-commons-test/src/test/resources/db.conf b/db-commons-test/src/test/resources/db.conf
@@ -1,4 +1,4 @@
-common = {
+sqlite = {
   dataSourceClass = slick.jdbc.DatabaseUrlDataSource
   profile = "slick.jdbc.SQLiteProfile$"
 
@@ -10,15 +10,15 @@ common = {
     username = ""
     password = ""
 
-    numThreads = 5 # default num threads is 20, which is way too much
+    numThreads = 3 # default num threads is 20, which is way too much
     # as long as we're on SQLite there's no point
     # in doing connection pooling
     connectionPool = disabled
   }
 }
 
 bitcoin-s {
-  wallet = ${common}
+  wallet = ${sqlite}
   wallet {
     # this config key is read by Slick
     db {
@@ -34,7 +34,7 @@ bitcoin-s {
     # }
   }
 
-  node = ${common}
+  node = ${sqlite}
   node {
     # this config key is read by Slick
     db {
@@ -43,7 +43,7 @@ bitcoin-s {
     }
   }
 
-  chain = ${common}
+  chain = ${sqlite}
   chain {
     # this config key is read by Slick
     db {
@@ -52,7 +52,7 @@ bitcoin-s {
     }
   }
 
-  test = ${common}
+  test = ${sqlite}
   test {
     # this config key is read by Slick
     db {

diff --git a/db-commons-test/src/test/scala/org/bitcoins/db/DBConfigTest.scala b/db-commons-test/src/test/scala/org/bitcoins/db/DBConfigTest.scala
@@ -0,0 +1,106 @@
+package org.bitcoins.db
+
+import java.io.{File, IOException}
+import java.nio.file.attribute.BasicFileAttributes
+import java.nio.file.{
+  FileVisitResult,
+  Files,
+  Path,
+  SimpleFileVisitor,
+  StandardOpenOption
+}
+
+import org.bitcoins.chain.config.ChainAppConfig
+import org.bitcoins.node.config.NodeAppConfig
+import org.bitcoins.wallet.config.WalletAppConfig
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+class DBConfigTest extends AnyFlatSpec with Matchers {
+
+  import scala.concurrent.ExecutionContext.Implicits.global
+
+  it should "use sqlite as default database and set its connection pool size to 1" in {
+    withTempDir { dataDir =>
+      val bytes = Files.readAllBytes(
+        new File("db-commons/src/main/resources/db.conf").toPath)
+      Files.write(dataDir.resolve("bitcoin-s.conf"),
+                  bytes,
+                  StandardOpenOption.CREATE_NEW,
+                  StandardOpenOption.WRITE)
+
+      val chainConfig = ChainAppConfig(dataDir)
+      val slickChainConfig = chainConfig.slickDbConfig
+      assert(slickChainConfig.profileName == "slick.jdbc.SQLiteProfile")
+      assert(slickChainConfig.config.hasPath("db.numThreads"))
+      assert(slickChainConfig.config.getInt("db.numThreads") == 1)
+
+      val nodeConfig = NodeAppConfig(dataDir)
+      val slickNodeConfig = nodeConfig.slickDbConfig
+      assert(slickNodeConfig.profileName == "slick.jdbc.SQLiteProfile")
+      assert(slickNodeConfig.config.hasPath("db.numThreads"))
+      assert(slickNodeConfig.config.getInt("db.numThreads") == 1)
+
+      val walletConfig = WalletAppConfig(dataDir)
+      val slickWalletConfig = walletConfig.slickDbConfig
+      assert(slickWalletConfig.profileName == "slick.jdbc.SQLiteProfile")
+      assert(slickWalletConfig.config.hasPath("db.numThreads"))
+      assert(slickWalletConfig.config.getInt("db.numThreads") == 1)
+    }
+  }
+
+  it should "use sqlite as default database and disable connection pool for tests" in {
+    withTempDir { dataDir =>
+      val chainConfig = ChainAppConfig(dataDir)
+      val slickChainConfig = chainConfig.slickDbConfig
+      assert(slickChainConfig.profileName == "slick.jdbc.SQLiteProfile")
+      assert(slickChainConfig.config.hasPath("db.numThreads"))
+      assert(slickChainConfig.config.getInt("db.numThreads") == 3)
+      assert(
+        slickChainConfig.config.getString("db.connectionPool") == "disabled")
+
+      val nodeConfig = NodeAppConfig(dataDir)
+      val slickNodeConfig = nodeConfig.slickDbConfig
+      assert(slickNodeConfig.profileName == "slick.jdbc.SQLiteProfile")
+      assert(slickNodeConfig.config.hasPath("db.numThreads"))
+      assert(slickNodeConfig.config.getInt("db.numThreads") == 3)
+      assert(
+        slickNodeConfig.config.getString("db.connectionPool") == "disabled")
+
+      val walletConfig = WalletAppConfig(dataDir)
+      val slickWalletConfig = walletConfig.slickDbConfig
+      assert(slickWalletConfig.profileName == "slick.jdbc.SQLiteProfile")
+      assert(slickWalletConfig.config.hasPath("db.numThreads"))
+      assert(slickWalletConfig.config.getInt("db.numThreads") == 3)
+      assert(
+        slickWalletConfig.config.getString("db.connectionPool") == "disabled")
+    }
+  }
+
+  def withTempDir[T](f: Path => T): T = {
+    val dir = Files.createTempDirectory(getClass.getName)
+    try {
+      f(dir)
+    } finally {
+      Files.walkFileTree(
+        dir,
+        new SimpleFileVisitor[Path] {
+          override def visitFile(
+              file: Path,
+              attrs: BasicFileAttributes): FileVisitResult = {
+            Files.delete(file);
+            FileVisitResult.CONTINUE
+          }
+
+          override def postVisitDirectory(
+              dir: Path,
+              exc: IOException): FileVisitResult = {
+            Files.delete(dir);
+            FileVisitResult.CONTINUE
+          }
+        }
+      )
+      ()
+    }
+  }
+}
diff --git a/db-commons/src/main/resources/db.conf b/db-commons/src/main/resources/db.conf
@@ -1,4 +1,4 @@
-common = {
+sqlite = {
   dataSourceClass = slick.jdbc.DatabaseUrlDataSource
   profile = "slick.jdbc.SQLiteProfile$"
 
@@ -10,16 +10,14 @@ common = {
     username = ""
     password = ""
 
-    numThreads = 5 # default num threads is 20, which is way too much
-    #maxConnections = 1
-    # as long as we're on SQLite there's no point
-    # in doing connection pooling
-    connectionPool = disabled
+    # this needs to be set to 1 for SQLITE as it does not support concurrent database operations
+    # see: https://github.com/bitcoin-s/bitcoin-s/pull/1840
+    numThreads = 1
   }
 }
 
 bitcoin-s {
-  wallet = ${common}
+  wallet = ${sqlite}
   wallet {
     # this config key is read by Slick
     db {
@@ -28,28 +26,42 @@ bitcoin-s {
     }
     # PostgreSQL example:
     # db {
-    #   url = "jdbc:postgresql://localhost:5432/"${bitcoin-s.wallet.db.name}
-    #   driver = "org.postgresql.Driver"
+    #   url = "jdbc:postgresql://localhost:5432/wallet"
+    #   driver = org.postgresql.Driver
     #   username = postgres
     #   password = ""
     # }
   }
 
-  node = ${common}
+  node = ${sqlite}
   node {
     # this config key is read by Slick
     db {
       name = nodedb.sqlite
       url = "jdbc:sqlite:"${bitcoin-s.node.db.path}${bitcoin-s.node.db.name}
     }
+    # PostgreSQL example:
+    # db {
+    #   url = "jdbc:postgresql://localhost:5432/node"
+    #   driver = org.postgresql.Driver
+    #   username = postgres
+    #   password = ""
+    # }
   }
 
-  chain = ${common}
+  chain = ${sqlite}
   chain {
     # this config key is read by Slick
     db {
       name = chaindb.sqlite
       url = "jdbc:sqlite:"${bitcoin-s.chain.db.path}${bitcoin-s.chain.db.name}
     }
+    # PostgreSQL example:
+    # db {
+    #   url = "jdbc:postgresql://localhost:5432/chain"
+    #   driver = org.postgresql.Driver
+    #   username = postgres
+    #   password = ""
+    # }
   }
 }
diff --git a/node-test/src/test/scala/org/bitcoins/node/networking/P2PClientTest.scala b/node-test/src/test/scala/org/bitcoins/node/networking/P2PClientTest.scala
@@ -19,6 +19,7 @@ import org.bitcoins.testkit.node.{
   CachedBitcoinSAppConfig,
   NodeTestUtil
 }
+import org.bitcoins.testkit.node.{CachedAppConfig, NodeTestUtil}
 import org.bitcoins.testkit.rpc.BitcoindRpcTestUtil
 import org.bitcoins.testkit.util.BitcoindRpcTest
 import org.scalatest._

diff --git a/project/CommonSettings.scala b/project/CommonSettings.scala
@@ -114,9 +114,9 @@ object CommonSettings {
 
   lazy val testWithDbSettings: Seq[Setting[_]] = Seq(
     // To make in-memory DBs work properly
-    Test / fork := true,
+    Test / fork := false,
     // To avoid deadlock issues with SQLite
-    Test / parallelExecution := false
+    Test / parallelExecution := true
   ) ++ testSettings
 
   lazy val prodSettings: Seq[Setting[_]] = settings

diff --git a/testkit/src/main/scala/org/bitcoins/testkit/BitcoinSTestAppConfig.scala b/testkit/src/main/scala/org/bitcoins/testkit/BitcoinSTestAppConfig.scala
@@ -113,7 +113,7 @@ object BitcoinSTestAppConfig {
          |   driver = "org.postgresql.Driver"
          |   username = "postgres"
          |   password = ""
-         |   connectionPool = disabled
+         |   numThreads = 10
          |   keepAliveConnection = true
          | }""".stripMargin
     }

diff --git a/testkit/src/main/scala/org/bitcoins/testkit/EmbeddedPg.scala b/testkit/src/main/scala/org/bitcoins/testkit/EmbeddedPg.scala
@@ -44,14 +44,19 @@ trait EmbeddedPg extends BeforeAndAfterAll { this: Suite =>
 
   def executePgSql(sql: String): Unit =
     pg.foreach { pg =>
-      val conn = pg.getPostgresDatabase.getConnection
       try {
-        val st = conn.createStatement()
+        val conn = pg.getPostgresDatabase.getConnection
         try {
-          st.execute(sql)
-        } finally st.close()
-
-      } finally conn.close()
+          val st = conn.createStatement()
+          try {
+            st.execute(sql)
+          } finally st.close()
+        } finally conn.close()
+      } catch {
+        case ex: Throwable =>
+          println(sql)
+          ex.printStackTrace()
+      }
     }
 
 }
diff --git a/testkit/src/main/scala/org/bitcoins/testkit/db/TestDAOFixture.scala b/testkit/src/main/scala/org/bitcoins/testkit/db/TestDAOFixture.scala
@@ -29,9 +29,8 @@ sealed trait TestDAOFixture
   }
 
   override def afterAll(): Unit = {
-    super.afterAll()
     testConfig.stop()
-    ()
+    super.afterAll()
   }
 
   def withFixture(test: OneArgAsyncTest): FutureOutcome = {

diff --git a/testkit/src/main/scala/org/bitcoins/testkit/fixtures/WalletDAOFixture.scala b/testkit/src/main/scala/org/bitcoins/testkit/fixtures/WalletDAOFixture.scala
@@ -44,17 +44,21 @@ trait WalletDAOFixture extends BitcoinSWalletTest {
 
   override def afterAll(): Unit = {
     super.afterAll()
-    walletConfig.stop()
-    ()
   }
 
   def withFixture(test: OneArgAsyncTest): FutureOutcome =
     makeFixture(build = () => Future(walletConfig.migrate()).map(_ => daos),
                 destroy = () => dropAll())(test)
 
-  def dropAll(): Future[Unit] =
-    for {
+  def dropAll(): Future[Unit] = {
+    val res = for {
       _ <- walletConfig.dropTable("flyway_schema_history")
       _ <- walletConfig.dropAll()
     } yield ()
+    res.failed.foreach { ex =>
+      ex.printStackTrace()
+    }
+    res
+  }
+
 }
diff --git a/testkit/src/main/scala/org/bitcoins/testkit/node/NodeUnitTest.scala b/testkit/src/main/scala/org/bitcoins/testkit/node/NodeUnitTest.scala
@@ -48,7 +48,7 @@ import org.bitcoins.wallet.WalletCallbacks
 import org.scalatest.FutureOutcome
 
 import scala.concurrent.duration._
-import scala.concurrent.{ExecutionContext, Future}
+import scala.concurrent.{Await, ExecutionContext, Future}
 
 trait NodeUnitTest extends BitcoinSFixture with EmbeddedPg {
 
@@ -58,6 +58,9 @@ trait NodeUnitTest extends BitcoinSFixture with EmbeddedPg {
   }
 
   override def afterAll(): Unit = {
+    Await.result(config.chainConf.stop(), 1.minute)
+    Await.result(config.nodeConf.stop(), 1.minute)
+    Await.result(config.walletConf.stop(), 1.minute)
     super[EmbeddedPg].afterAll()
   }
 

diff --git a/testkit/src/main/scala/org/bitcoins/testkit/wallet/BitcoinSWalletTest.scala b/testkit/src/main/scala/org/bitcoins/testkit/wallet/BitcoinSWalletTest.scala
@@ -2,10 +2,10 @@ package org.bitcoins.testkit.wallet
 
 import akka.actor.ActorSystem
 import com.typesafe.config.{Config, ConfigFactory}
-import org.bitcoins.core.api.chain.ChainQueryApi.FilterResponse
-import org.bitcoins.core.api.node.NodeApi
 import org.bitcoins.core.api.chain.ChainQueryApi
+import org.bitcoins.core.api.chain.ChainQueryApi.FilterResponse
 import org.bitcoins.core.api.feeprovider.FeeRateApi
+import org.bitcoins.core.api.node.NodeApi
 import org.bitcoins.core.currency._
 import org.bitcoins.core.gcs.BlockFilter
 import org.bitcoins.core.protocol.BlockStamp
@@ -29,12 +29,8 @@ import org.bitcoins.wallet.config.WalletAppConfig
 import org.bitcoins.wallet.{Wallet, WalletCallbacks, WalletLogger}
 import org.scalatest._
 
-import scala.concurrent.{
-  ExecutionContext,
-  ExecutionContextExecutor,
-  Future,
-  Promise
-}
+import scala.concurrent.duration._
+import scala.concurrent._
 
 trait BitcoinSWalletTest
     extends BitcoinSFixture
@@ -55,6 +51,13 @@ trait BitcoinSWalletTest
     super[EmbeddedPg].beforeAll()
   }
 
+  override def afterAll(): Unit = {
+    Await.result(config.chainConf.stop(), 1.minute)
+    Await.result(config.nodeConf.stop(), 1.minute)
+    Await.result(config.walletConf.stop(), 1.minute)
+    super[EmbeddedPg].afterAll()
+  }
+
   def nodeApi: NodeApi = MockNodeApi
 
   val legacyWalletConf: Config =