From 0878b25fbd213607c91d62146d83c15f9c51997c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebastian=20K=C3=BCrten?= <sebastian@mobanisto.de>
Date: Mon, 7 Feb 2022 17:29:46 +0100
Subject: [PATCH] Gracefully handle background thread exceptions
---
.../ContactsManagerIntegrationTest.kt | 4 +-
...ctsManagerMalformedInputIntegrationTest.kt | 4 +-
.../core/files/FileManagerIntegrationTest.kt | 4 +-
.../mailbox/core/server/IntegrationTest.kt | 40 ++++++++++++++++---
.../core/settings/MetadataRouteManagerTest.kt | 4 +-
.../mailbox/core/setup/WipeTest1.kt | 14 ++++---
.../mailbox/core/setup/WipeTest2.kt | 16 +++++---
7 files changed, 62 insertions(+), 24 deletions(-)
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerIntegrationTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerIntegrationTest.kt
index ffa34af3..aa9f5454 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerIntegrationTest.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerIntegrationTest.kt
@@ -27,8 +27,8 @@ import kotlin.test.assertEquals
class ContactsManagerIntegrationTest : IntegrationTest() {
@BeforeEach
- override fun initDb() {
- super.initDb()
+ override fun beforeEach() {
+ super.beforeEach()
addOwnerToken()
}
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerMalformedInputIntegrationTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerMalformedInputIntegrationTest.kt
index 0f745679..6acd4fa5 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerMalformedInputIntegrationTest.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerMalformedInputIntegrationTest.kt
@@ -19,8 +19,8 @@ import kotlin.test.assertEquals
class ContactsManagerMalformedInputIntegrationTest : IntegrationTest(false) {
@BeforeEach
- override fun initDb() {
- super.initDb()
+ override fun beforeEach() {
+ super.beforeEach()
addOwnerToken()
}
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt
index fd0d67b9..cd92398a 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt
@@ -23,8 +23,8 @@ class FileManagerIntegrationTest : IntegrationTest() {
private val bytes = Random.nextBytes(2048)
@BeforeEach
- override fun initDb() {
- super.initDb()
+ override fun beforeEach() {
+ super.beforeEach()
addOwnerToken()
addContact(contact1)
addContact(contact2)
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/server/IntegrationTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/server/IntegrationTest.kt
index bbb9f151..a628a963 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/server/IntegrationTest.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/server/IntegrationTest.kt
@@ -22,7 +22,10 @@ import org.junit.jupiter.api.BeforeAll
import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.TestInstance
import org.junit.jupiter.api.TestInstance.Lifecycle
+import org.junit.jupiter.api.fail
import org.junit.jupiter.api.io.TempDir
+import org.slf4j.Logger
+import org.slf4j.LoggerFactory
import java.io.File
import kotlin.test.assertFalse
import kotlin.test.assertTrue
@@ -30,12 +33,16 @@ import kotlin.test.assertTrue
@TestInstance(Lifecycle.PER_CLASS)
abstract class IntegrationTest(private val installJsonFeature: Boolean = true) {
+ companion object {
+ private val LOG: Logger = LoggerFactory.getLogger(IntegrationTest::class.java)
+ }
+
protected lateinit var testComponent: TestComponent
protected val db by lazy { testComponent.getDatabase() }
private val lifecycleManager by lazy { testComponent.getLifecycleManager() }
protected val setupManager by lazy { testComponent.getSetupManager() }
protected val metadataManager by lazy { testComponent.getMetadataManager() }
- private val wipeManager by lazy { testComponent.getWipeManager() }
+ protected val wipeManager by lazy { testComponent.getWipeManager() }
protected val httpClient = HttpClient(CIO) {
expectSuccess = false // prevents exceptions on non-success responses
if (installJsonFeature) {
@@ -55,6 +62,18 @@ abstract class IntegrationTest(private val installJsonFeature: Boolean = true) {
protected val contact1 = getNewRandomContact()
protected val contact2 = getNewRandomContact()
+ @Volatile
+ protected var exceptionInBackgroundThread = false
+
+ init {
+ // Ensure exceptions thrown on worker threads cause tests to fail
+ val fail = Thread.UncaughtExceptionHandler { _: Thread?, throwable: Throwable ->
+ LOG.warn("Caught unhandled exception", throwable)
+ exceptionInBackgroundThread = true
+ }
+ Thread.setDefaultUncaughtExceptionHandler(fail)
+ }
+
@BeforeAll
fun setUp(@TempDir tempDir: File) {
testComponent = DaggerTestComponent.builder().testModule(TestModule(tempDir)).build()
@@ -71,7 +90,8 @@ abstract class IntegrationTest(private val installJsonFeature: Boolean = true) {
}
@BeforeEach
- open fun initDb() {
+ open fun beforeEach() {
+ exceptionInBackgroundThread = false
// need to reopen database here because we're closing it after each test
db.open(null)
db.read { txn ->
@@ -82,9 +102,19 @@ abstract class IntegrationTest(private val installJsonFeature: Boolean = true) {
}
@AfterEach
- open fun wipe() {
- wipeManager.wipeDatabaseAndFiles()
- assertFalse(setupManager.hasDb)
+ open fun afterEach() {
+ afterEach(true)
+ }
+
+ fun afterEach(wipe: Boolean) {
+ if (wipe) {
+ wipeManager.wipeDatabaseAndFiles()
+ assertFalse(setupManager.hasDb)
+ }
+
+ if (exceptionInBackgroundThread) {
+ fail("background thread has thrown an exception unexpectedly")
+ }
}
protected fun addOwnerToken() {
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/settings/MetadataRouteManagerTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/settings/MetadataRouteManagerTest.kt
index 726a6f8c..71fa450b 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/settings/MetadataRouteManagerTest.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/settings/MetadataRouteManagerTest.kt
@@ -13,8 +13,8 @@ import kotlin.test.assertEquals
class MetadataRouteManagerTest : IntegrationTest() {
@BeforeEach
- override fun initDb() {
- super.initDb()
+ override fun beforeEach() {
+ super.beforeEach()
addOwnerToken()
addContact(contact1)
addContact(contact2)
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest1.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest1.kt
index e140ae19..11b79b0f 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest1.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest1.kt
@@ -8,9 +8,9 @@ import kotlin.concurrent.thread
class WipeTest1 : IntegrationTest() {
@Test
- fun test() {
+ fun `wiping and concurrent database access doesn't cause deadlocks`() {
val t1 = thread(name = "dropper") {
- db.dropAllTablesAndClose()
+ wipeManager.wipeDatabaseAndFiles()
}
// This most likely throws a DbException within the thread, but if it does is doesn't make
// the test fail.
@@ -19,13 +19,17 @@ class WipeTest1 : IntegrationTest() {
}
t1.join()
t2.join()
+
+ // reset flag for exceptions thrown on background threads as this can indeed happen here
+ // and is OK
+ exceptionInBackgroundThread = false
}
@AfterEach
- override fun clearDb() {
- // This is not expected to work because calling dropAllTablesAndClose() on a closed Database
+ override fun afterEach() {
+ // We need to pass false here because calling wipeDatabaseAndFiles() with a closed Database
// throws a DbClosedException.
- // super.clearDb()
+ afterEach(false)
}
}
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest2.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest2.kt
index d7e0ae45..14de328d 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest2.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest2.kt
@@ -22,11 +22,11 @@ class WipeTest2 : IntegrationTest() {
}
@Test
- fun test() {
+ fun `concurrent wipe requests don't interfere and database access doesn't cause deadlocks`() {
// One of the two dropper threads will succeed, the other will fail.
val t1 = thread(name = "dropper 1") {
try {
- db.dropAllTablesAndClose()
+ wipeManager.wipeDatabaseAndFiles()
incrementSuccess()
} catch (t: Throwable) {
incrementFailure()
@@ -34,7 +34,7 @@ class WipeTest2 : IntegrationTest() {
}
val t2 = thread(name = "dropper 2") {
try {
- db.dropAllTablesAndClose()
+ wipeManager.wipeDatabaseAndFiles()
incrementSuccess()
} catch (t: Throwable) {
incrementFailure()
@@ -50,13 +50,17 @@ class WipeTest2 : IntegrationTest() {
t3.join()
assertEquals(1, succeeded)
assertEquals(1, failed)
+
+ // reset flag for exceptions thrown on background threads as this can indeed happen here
+ // and is OK
+ exceptionInBackgroundThread = false
}
@AfterEach
- override fun clearDb() {
- // This is not expected to work because calling dropAllTablesAndClose() on a closed Database
+ override fun afterEach() {
+ // We need to pass false here because calling wipeDatabaseAndFiles() with a closed Database
// throws a DbClosedException.
- // super.clearDb()
+ afterEach(false)
}
}
--
GitLab