fix(storage): Fix multiple instances of TransferDB leading to SQLiteD…

…atabaseLockedException (#2786) Co-authored-by: Tyler Roach <[email protected]>
aws-amplify · Apr 23, 2024 · c6e9e91 · c6e9e91
1 parent 916a3bb
commit c6e9e91
Show file tree

Hide file tree

Showing 6 changed files with 55 additions and 30 deletions.
diff --git a/aws-storage-s3/build.gradle.kts b/aws-storage-s3/build.gradle.kts
@@ -42,6 +42,7 @@ dependencies {
     testImplementation(libs.test.mockk)
     testImplementation(libs.test.androidx.workmanager)
     testImplementation(libs.test.kotlin.coroutines)
+    testImplementation(libs.test.kotest.assertions)
     testImplementation(project(":aws-storage-s3"))
 
     androidTestImplementation(project(":testutils"))

diff --git a/aws-storage-s3/src/main/java/com/amplifyframework/storage/s3/transfer/TransferDB.kt b/aws-storage-s3/src/main/java/com/amplifyframework/storage/s3/transfer/TransferDB.kt
@@ -15,11 +15,11 @@
 
 package com.amplifyframework.storage.s3.transfer
 
-import android.annotation.SuppressLint
 import android.content.ContentValues
 import android.content.Context
 import android.database.Cursor
 import android.net.Uri
+import androidx.annotation.VisibleForTesting
 import aws.sdk.kotlin.services.s3.model.CompletedPart
 import aws.sdk.kotlin.services.s3.model.ObjectCannedAcl
 import com.amplifyframework.core.Amplify
@@ -33,33 +33,33 @@ import java.io.File
 /**
  * SQlite database to store transfer records
  */
-@SuppressLint("VisibleForTests")
 internal class TransferDB private constructor(context: Context) {
 
-    private var transferDBHelper: TransferDBHelper = synchronized(this) {
-        TransferDBHelper(context)
-    }
+    private val transferDBHelper = TransferDBHelper(context)
 
-    private val logger =
-        Amplify.Logging.logger(
-            CategoryType.STORAGE,
-            AWSS3StoragePlugin.AWS_S3_STORAGE_LOG_NAMESPACE.format(this::class.java.simpleName)
-        )
+    private val logger = Amplify.Logging.logger(
+        CategoryType.STORAGE,
+        AWSS3StoragePlugin.AWS_S3_STORAGE_LOG_NAMESPACE.format(this::class.java.simpleName)
+    )
 
     companion object {
         private const val QUERY_PLACE_HOLDER_STRING = ",?"
-        private val instance: TransferDB? = null
 
-        @JvmStatic
+        @Volatile
+        @VisibleForTesting
+        var instance: TransferDB? = null
+
         fun getInstance(context: Context): TransferDB {
-            return instance ?: TransferDB(context)
+            // Use double check locking for thread safety. It is important that there is never more than one instance
+            // of this class since it holds an SQLiteOpenHelper.
+            return instance ?: synchronized(this) {
+                instance ?: TransferDB(context).also { instance = it }
+            }
         }
     }
 
     fun closeDB() {
-        synchronized(this) {
-            transferDBHelper.close()
-        }
+        transferDBHelper.close()
     }
 
     /**

diff --git a/aws-storage-s3/src/main/java/com/amplifyframework/storage/s3/transfer/TransferDBHelper.kt b/aws-storage-s3/src/main/java/com/amplifyframework/storage/s3/transfer/TransferDBHelper.kt
@@ -24,14 +24,16 @@ import android.database.sqlite.SQLiteOpenHelper
 import android.database.sqlite.SQLiteQueryBuilder
 import android.net.Uri
 import android.text.TextUtils
-import androidx.annotation.VisibleForTesting
 import com.amplifyframework.core.Amplify
 import com.amplifyframework.core.category.CategoryType
 import com.amplifyframework.storage.s3.AWSS3StoragePlugin
 
-@VisibleForTesting
-internal class TransferDBHelper(private val context: Context) :
-    SQLiteOpenHelper(context, DATABASE_NAME, null, DATABASE_VERSION) {
+internal class TransferDBHelper(private val context: Context) : SQLiteOpenHelper(
+    context,
+    DATABASE_NAME,
+    null,
+    DATABASE_VERSION
+) {
 
     internal val contentUri: Uri
     private val uriMatcher: UriMatcher

diff --git a/aws-storage-s3/src/main/java/com/amplifyframework/storage/s3/transfer/TransferManager.kt b/aws-storage-s3/src/main/java/com/amplifyframework/storage/s3/transfer/TransferManager.kt
@@ -43,15 +43,16 @@ import kotlin.math.min
  * download files. It inserts upload and download records into the database and
  * enqueue a WorkRequest for WorkManager to service the transfer
  */
-internal class TransferManager @JvmOverloads constructor(
+internal class TransferManager(
     context: Context,
     s3: S3Client,
     private val pluginKey: String,
     private val workManager: WorkManager = WorkManager.getInstance(context)
 ) {
 
     private val transferDB: TransferDB = TransferDB.getInstance(context)
-    val transferStatusUpdater: TransferStatusUpdater = TransferStatusUpdater.getInstance(context)
+    val transferStatusUpdater: TransferStatusUpdater = TransferStatusUpdater(transferDB)
+
     private val logger =
         Amplify.Logging.logger(
             CategoryType.STORAGE,

diff --git a/...torage-s3/src/main/java/com/amplifyframework/storage/s3/transfer/TransferStatusUpdater.kt b/...torage-s3/src/main/java/com/amplifyframework/storage/s3/transfer/TransferStatusUpdater.kt
@@ -15,7 +15,6 @@
 
 package com.amplifyframework.storage.s3.transfer
 
-import android.content.Context
 import android.os.Handler
 import android.os.Looper
 import com.amplifyframework.core.Amplify
@@ -28,7 +27,7 @@ import java.util.concurrent.ConcurrentHashMap
 /**
  * Updates transfer status to observers and to local DB
  **/
-internal class TransferStatusUpdater private constructor(
+internal class TransferStatusUpdater(
     private val transferDB: TransferDB
 ) {
     private val logger =
@@ -64,13 +63,7 @@ internal class TransferStatusUpdater private constructor(
     }
 
     companion object {
-
         internal const val TEMP_FILE_PREFIX = "aws-s3-d861b25a-1edf-11eb-adc1-0242ac120002"
-
-        @JvmStatic
-        fun getInstance(context: Context): TransferStatusUpdater {
-            return TransferStatusUpdater(TransferDB.getInstance(context))
-        }
     }
 
     @Synchronized

diff --git a/aws-storage-s3/src/test/java/com/amplifyframework/storage/s3/transfer/TransferDBTest.kt b/aws-storage-s3/src/test/java/com/amplifyframework/storage/s3/transfer/TransferDBTest.kt
@@ -0,0 +1,28 @@
+package com.amplifyframework.storage.s3.transfer
+
+import io.kotest.matchers.types.shouldBeSameInstanceAs
+import org.junit.After
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.RuntimeEnvironment
+
+@RunWith(RobolectricTestRunner::class)
+class TransferDBTest {
+
+    @After
+    fun teardown() {
+        // Clear instance
+        TransferDB.instance = null
+    }
+
+    @Test
+    fun `getInstance returns the same object`() {
+        val context = RuntimeEnvironment.getApplication()
+
+        val db1 = TransferDB.getInstance(context)
+        val db2 = TransferDB.getInstance(context)
+
+        db1 shouldBeSameInstanceAs db2
+    }
+}