For mozilla-mobile#12469 - Cancel in progress storage requests before…

… new awesomebar suggestions

components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesStorage.kt

@@ -30,14 +30,16 @@ abstract class PlacesStorage(
     context: Context,
     val crashReporter: CrashReporting? = null
 ) : Storage, SyncableStore {
-    internal val writeScope by lazy {
+    internal var writeScope =
         CoroutineScope(
             Executors.newSingleThreadExecutor(
                 NamedThreadFactory("PlacesStorageWriteScope")
             ).asCoroutineDispatcher()
         )
-    }
-    internal val readScope by lazy { CoroutineScope(Dispatchers.IO) }
+        @VisibleForTesting internal set
+
+    internal var readScope = CoroutineScope(Dispatchers.IO)
+        @VisibleForTesting internal set
     private val storageDir by lazy { context.filesDir }
 
     abstract val logger: Logger
@@ -48,8 +50,8 @@ abstract class PlacesStorage(
         RustPlacesConnection
     }
 
-    internal val writer: PlacesWriterConnection by lazy { places.writer() }
-    internal val reader: PlacesReaderConnection by lazy { places.reader() }
+    internal open val writer: PlacesWriterConnection by lazy { places.writer() }
+    internal open val reader: PlacesReaderConnection by lazy { places.reader() }
 
     override suspend fun warmUp() {
         logElapsedTime(logger, "Warming up places storage") {
@@ -67,15 +69,50 @@ abstract class PlacesStorage(
         }
     }
 
-    /**
-     * Cleans up background work and database connections
-     */
+    @Deprecated(
+        "Use `cleanupWrites` and `cleanupReads` to get a similar functionality. " +
+            "See https://github.com/mozilla-mobile/android-components/issues/7348 for a description of the issue."
+    )
     override fun cleanup() {
         writeScope.coroutineContext.cancelChildren()
         readScope.coroutineContext.cancelChildren()
         places.close()
     }
 
+    override fun cleanupWrites() {
+        interruptCurrentWrites()
+        writeScope.coroutineContext.cancelChildren()
+    }
+
+    override fun cleanupReads() {
+        interruptCurrentReads()
+        readScope.coroutineContext.cancelChildren()
+    }
+
+    /**
+     * Stop all current write operations.
+     * Allows immediately dismissing all write operations and clearing the write queue.
+     */
+    internal fun interruptCurrentWrites() {
+        try {
+            writer.interrupt()
+        } catch (e: PlacesException.OperationInterrupted) {
+            logger.debug("Ignoring expected OperationInterrupted exception for explicit writer interrupt call", e)
+        }
+    }
+
+    /**
+     * Stop all current read queries.
+     * Allows avoiding having to wait for stale queries responses and clears the queries queue.
+     */
+    internal fun interruptCurrentReads() {
+        try {
+            reader.interrupt()
+        } catch (e: PlacesException.OperationInterrupted) {
+            logger.debug("Ignoring expected OperationInterrupted exception for explicit reader interrupt call", e)
+        }
+    }
+
     /**
      * Runs [block] described by [operation], ignoring and logging non-fatal exceptions.
      *

components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/RemoteTabsStorage.kt

@@ -30,7 +30,8 @@ open class RemoteTabsStorage(
     private val crashReporter: CrashReporting? = null
 ) : Storage, SyncableStore {
     internal val api by lazy { RemoteTabsProvider(File(context.filesDir, TABS_DB_NAME).canonicalPath) }
-    private val scope by lazy { CoroutineScope(Dispatchers.IO) }
+    private val readScope by lazy { CoroutineScope(Dispatchers.IO) }
+    private val writeScope by lazy { CoroutineScope(Dispatchers.IO) }
     internal val logger = Logger("RemoteTabsStorage")
 
     override suspend fun warmUp() {
@@ -42,7 +43,7 @@ open class RemoteTabsStorage(
      * @param tabs The list of opened tabs, for all opened non-private windows, on this device.
      */
     suspend fun store(tabs: List<Tab>) {
-        return withContext(scope.coroutineContext) {
+        return withContext(writeScope.coroutineContext) {
             try {
                 api.setLocalTabs(
                     tabs.map {
@@ -62,7 +63,7 @@ open class RemoteTabsStorage(
      * @return A mapping of opened tabs per device.
      */
     suspend fun getAll(): Map<SyncClient, List<Tab>> {
-        return withContext(scope.coroutineContext) {
+        return withContext(readScope.coroutineContext) {
             try {
                 api.getAll().map { device ->
                     val tabs = device.remoteTabs.map { tab ->
@@ -88,7 +89,16 @@ open class RemoteTabsStorage(
     }
 
     override fun cleanup() {
-        scope.coroutineContext.cancelChildren()
+        cleanupWrites()
+        cleanupReads()
+    }
+
+    override fun cleanupWrites() {
+        writeScope.coroutineContext.cancelChildren()
+    }
+
+    override fun cleanupReads() {
+        readScope.coroutineContext.cancelChildren()
     }
 
     override fun registerWithSyncManager() {

components/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt

@@ -42,6 +42,7 @@ class PlacesBookmarksStorageTest {
     }
 
     @After
+    @Suppress("DEPRECATION")
     fun cleanup() = runTestOnMain {
         bookmarks.cleanup()
     }

components/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesHistoryStorageTest.kt

@@ -56,6 +56,7 @@ class PlacesHistoryStorageTest {
     }
 
     @After
+    @Suppress("DEPRECATION")
     fun cleanup() = runTestOnMain {
         history.cleanup()
     }

components/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesStorageTest.kt

@@ -0,0 +1,78 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.browser.storage.sync
+
+import android.content.Context
+import kotlinx.coroutines.cancelChildren
+import mozilla.appservices.places.PlacesReaderConnection
+import mozilla.appservices.places.PlacesWriterConnection
+import mozilla.appservices.places.uniffi.PlacesException
+import mozilla.components.support.base.log.logger.Logger
+import mozilla.components.support.test.mock
+import org.junit.Test
+import org.mockito.Mockito.doAnswer
+import org.mockito.Mockito.doReturn
+import org.mockito.Mockito.verify
+import kotlin.coroutines.CoroutineContext
+
+class PlacesStorageTest {
+    private val storage = FakePlacesStorage()
+
+    @Test
+    fun `WHEN all reads are interrupted THEN no exception is thrown`() {
+        doAnswer {
+            throw PlacesException.OperationInterrupted("This should be caught")
+        }.`when`(storage.reader).interrupt()
+
+        storage.interruptCurrentReads()
+
+        verify(storage.reader).interrupt()
+    }
+
+    @Test
+    fun `WHEN all writes are interrupted THEN no exception is thrown`() {
+        doAnswer {
+            throw PlacesException.OperationInterrupted("This should be caught")
+        }.`when`(storage.writer).interrupt()
+
+        storage.interruptCurrentWrites()
+
+        verify(storage.writer).interrupt()
+    }
+
+    @Test
+    fun `WHEN a call is made to clean all reads THEN they are cancelled`() {
+        storage.readScope = mock {
+            doReturn(mock<CoroutineContext>()).`when`(this).coroutineContext
+        }
+
+        storage.cleanupReads()
+
+        verify(storage.reader).interrupt()
+        verify(storage.readScope.coroutineContext).cancelChildren()
+    }
+
+    @Test
+    fun `WHEN a call is made to clean all writes THEN they are cancelled`() {
+        storage.writeScope = mock {
+            doReturn(mock<CoroutineContext>()).`when`(this).coroutineContext
+        }
+
+        storage.cleanupWrites()
+
+        verify(storage.writer).interrupt()
+        verify(storage.writeScope.coroutineContext).cancelChildren()
+    }
+}
+
+class FakePlacesStorage(
+    context: Context = mock()
+) : PlacesStorage(context) {
+    override val logger = Logger("FakePlacesStorage")
+    override fun registerWithSyncManager() {}
+
+    override val writer: PlacesWriterConnection = mock()
+    override val reader: PlacesReaderConnection = mock()
+}

components/concept/storage/src/main/java/mozilla/components/concept/storage/CleanableStorage.kt

@@ -0,0 +1,25 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.concept.storage
+
+/**
+ * Storage that allows to stop and clean in progress operations.
+ */
+interface CleanableStorage {
+    /**
+     * Cleans up all background work and operations queue.
+     */
+    fun cleanup()
+
+    /**
+     * Cleans up all pending write operations.
+     */
+    fun cleanupWrites()
+
+    /**
+     * Cleans up all pending read operations.
+     */
+    fun cleanupReads()
+}

components/concept/storage/src/main/java/mozilla/components/concept/storage/HistoryMetadataStorage.kt

@@ -110,7 +110,7 @@ data class HistoryHighlightWeights(
 /**
  * An interface for interacting with a storage that manages [HistoryMetadata].
  */
-interface HistoryMetadataStorage {
+interface HistoryMetadataStorage : CleanableStorage {
     /**
      * Returns the most recent [HistoryMetadata] for the provided [url].
      *

components/concept/storage/src/main/java/mozilla/components/concept/storage/Storage.kt

@@ -7,7 +7,7 @@ package mozilla.components.concept.storage
 /**
  * An interface which provides generic operations for storing browser data like history and bookmarks.
  */
-interface Storage {
+interface Storage : CleanableStorage {
     /**
      * Make sure underlying database connections are established.
      */
@@ -17,9 +17,4 @@ interface Storage {
      * Runs internal database maintenance tasks
      */
     suspend fun runMaintenance()
-
-    /**
-     * Cleans up background work and database connections
-     */
-    fun cleanup()
 }

components/feature/awesomebar/src/main/java/mozilla/components/feature/awesomebar/provider/BookmarksStorageSuggestionProvider.kt

@@ -5,6 +5,7 @@
 package mozilla.components.feature.awesomebar.provider
 
 import android.graphics.drawable.Drawable
+import androidx.annotation.VisibleForTesting
 import mozilla.components.browser.icons.BrowserIcons
 import mozilla.components.browser.icons.IconRequest
 import mozilla.components.concept.awesomebar.AwesomeBar
@@ -34,7 +35,7 @@ private const val BOOKMARKS_SUGGESTION_LIMIT = 20
  * @param suggestionsHeader optional parameter to specify if the suggestion should have a header
  */
 class BookmarksStorageSuggestionProvider(
-    private val bookmarksStorage: BookmarksStorage,
+    @get:VisibleForTesting internal val bookmarksStorage: BookmarksStorage,
     private val loadUrlUseCase: SessionUseCases.LoadUrlUseCase,
     private val icons: BrowserIcons? = null,
     private val indicatorIcon: Drawable? = null,
@@ -49,6 +50,8 @@ class BookmarksStorageSuggestionProvider(
     }
 
     override suspend fun onInputChanged(text: String): List<AwesomeBar.Suggestion> {
+        bookmarksStorage.cleanupReads()
+
         if (text.isEmpty()) {
             return emptyList()
         }

components/feature/awesomebar/src/main/java/mozilla/components/feature/awesomebar/provider/CombinedHistorySuggestionProvider.kt

@@ -61,6 +61,9 @@ class CombinedHistorySuggestionProvider(
     }
 
     override suspend fun onInputChanged(text: String): List<AwesomeBar.Suggestion> = coroutineScope {
+        historyStorage.cleanupReads()
+        historyMetadataStorage.cleanupReads()
+
         if (text.isBlank()) {
             return@coroutineScope emptyList()
         }

components/feature/awesomebar/src/main/java/mozilla/components/feature/awesomebar/provider/HistoryMetadataSuggestionProvider.kt

@@ -38,11 +38,11 @@ const val DEFAULT_METADATA_SUGGESTION_LIMIT = 5
  * @param suggestionsHeader optional parameter to specify if the suggestion should have a header
  */
 class HistoryMetadataSuggestionProvider(
-    private val historyStorage: HistoryMetadataStorage,
+    @get:VisibleForTesting internal val historyStorage: HistoryMetadataStorage,
     private val loadUrlUseCase: SessionUseCases.LoadUrlUseCase,
     private val icons: BrowserIcons? = null,
     internal val engine: Engine? = null,
-    @VisibleForTesting internal val maxNumberOfSuggestions: Int = DEFAULT_METADATA_SUGGESTION_LIMIT,
+    @get:VisibleForTesting internal val maxNumberOfSuggestions: Int = DEFAULT_METADATA_SUGGESTION_LIMIT,
     private val showEditSuggestion: Boolean = true,
     private val suggestionsHeader: String? = null,
 ) : AwesomeBar.SuggestionProvider {
@@ -53,6 +53,8 @@ class HistoryMetadataSuggestionProvider(
     }
 
     override suspend fun onInputChanged(text: String): List<AwesomeBar.Suggestion> {
+        historyStorage.cleanupReads()
+
         if (text.isNullOrBlank()) {
             return emptyList()
         }

components/feature/awesomebar/src/main/java/mozilla/components/feature/awesomebar/provider/HistoryStorageSuggestionProvider.kt

@@ -38,11 +38,11 @@ const val DEFAULT_HISTORY_SUGGESTION_LIMIT = 20
  * @param suggestionsHeader optional parameter to specify if the suggestion should have a header
  */
 class HistoryStorageSuggestionProvider(
-    private val historyStorage: HistoryStorage,
+    @get:VisibleForTesting internal val historyStorage: HistoryStorage,
     private val loadUrlUseCase: SessionUseCases.LoadUrlUseCase,
     private val icons: BrowserIcons? = null,
     internal val engine: Engine? = null,
-    @VisibleForTesting internal var maxNumberOfSuggestions: Int = DEFAULT_HISTORY_SUGGESTION_LIMIT,
+    @get:VisibleForTesting internal var maxNumberOfSuggestions: Int = DEFAULT_HISTORY_SUGGESTION_LIMIT,
     private val showEditSuggestion: Boolean = true,
     private val suggestionsHeader: String? = null,
 ) : AwesomeBar.SuggestionProvider {
@@ -54,6 +54,8 @@ class HistoryStorageSuggestionProvider(
     }
 
     override suspend fun onInputChanged(text: String): List<AwesomeBar.Suggestion> {
+        historyStorage.cleanupReads()
+
         if (text.isEmpty()) {
             return emptyList()
         }