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,57 @@ 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 issues" +
+            "for when using this method"
+    )
     override fun cleanup() {
         writeScope.coroutineContext.cancelChildren()
         readScope.coroutineContext.cancelChildren()
         places.close()
     }
 
+    override fun cancelWrites() {
+        interruptCurrentWrites()
+        writeScope.coroutineContext.cancelChildren()
+    }
+
+    override fun cancelReads() {
+        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)
+        } catch (e: PlacesException) {
+            crashReporter?.submitCaughtException(e)
+            logger.warn("Ignoring PlacesException while interrupting writes", 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)
+        } catch (e: PlacesException) {
+            crashReporter?.submitCaughtException(e)
+            logger.warn("Ignoring PlacesException while interrupting reads", 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

@@ -91,6 +91,14 @@ open class RemoteTabsStorage(
         scope.coroutineContext.cancelChildren()
     }
 
+    override fun cleanupWrites() {
+        // no-op
+    }
+
+    override fun cleanupReads() {
+        // no-op
+    }
+
     override fun registerWithSyncManager() {
         return api.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.cancelReads()
+
+        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.cancelWrites()
+
+        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/CancelableStorage.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 CancelableStorage {
+    /**
+     * Cleans up all background work and operations queue.
+     */
+    fun cleanup()
+
+    /**
+     * Cleans up all pending write operations.
+     */
+    fun cancelWrites()
+
+    /**
+     * Cleans up all pending read operations.
+     */
+    fun cancelReads()
+}

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 : CancelableStorage {
     /**
      * 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 : CancelableStorage {
     /**
      * 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.cancelReads()
+
         if (text.isEmpty()) {
             return emptyList()
         }

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

@@ -61,11 +61,13 @@ class CombinedHistorySuggestionProvider(
     }
 
     override suspend fun onInputChanged(text: String): List<AwesomeBar.Suggestion> = coroutineScope {
+        historyStorage.cancelReads()
+        historyMetadataStorage.cancelReads()
+
         if (text.isBlank()) {
             return@coroutineScope emptyList()
         }
 
-        // Do both queries in parallel to optimize for speed.
         val metadataSuggestionsAsync = async {
             historyMetadataStorage
                 .queryHistoryMetadata(text, maxNumberOfSuggestions)

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.cancelReads()
+
         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.cancelReads()
+
         if (text.isEmpty()) {
             return emptyList()
         }

components/feature/awesomebar/src/test/java/mozilla/components/feature/awesomebar/provider/BookmarksStorageSuggestionProviderTest.kt

@@ -20,8 +20,11 @@ import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.mockito.ArgumentMatchers.anyInt
 import org.mockito.ArgumentMatchers.anyString
+import org.mockito.Mockito.inOrder
 import org.mockito.Mockito.never
+import org.mockito.Mockito.spy
 import org.mockito.Mockito.times
 import org.mockito.Mockito.verify
 import java.util.UUID
@@ -45,6 +48,28 @@ class BookmarksStorageSuggestionProviderTest {
         assertTrue(suggestions.isEmpty())
     }
 
+    @Test
+    fun `Provider cleanups all previous read operations when text is empty`() = runTest {
+        val provider = BookmarksStorageSuggestionProvider(mock(), mock())
+
+        provider.onInputChanged("")
+
+        verify(provider.bookmarksStorage).cancelReads()
+    }
+
+    @Test
+    fun `Provider cleanups all previous read operations when text is not empty`() = runTest {
+        val storage = spy(bookmarks)
+        val provider = BookmarksStorageSuggestionProvider(storage, mock())
+        storage.addItem("Mobile", newItem.url!!, newItem.title!!, null)
+        val orderVerifier = inOrder(storage)
+
+        provider.onInputChanged("moz")
+
+        orderVerifier.verify(provider.bookmarksStorage).cancelReads()
+        orderVerifier.verify(provider.bookmarksStorage).searchBookmarks(eq("moz"), anyInt())
+    }
+
     @Test
     fun `Provider returns suggestions from configured bookmarks storage`() = runTest {
         val provider = BookmarksStorageSuggestionProvider(bookmarks, mock())
@@ -224,5 +249,14 @@ class BookmarksStorageSuggestionProviderTest {
             // "Not needed for the test"
             throw NotImplementedError()
         }
+
+        override fun cleanupWrites() {
+            // "Not needed for the test"
+            throw NotImplementedError()
+        }
+
+        override fun cleanupReads() {
+            // no-op
+        }
     }
 }