From 0a84b6ac61ef444e4f80ef6f838b097365e0c441 Mon Sep 17 00:00:00 2001 From: Lina Butler Date: Fri, 24 May 2024 15:30:52 -0700 Subject: [PATCH] Bugfix FXIOS-9159 - Fix Firefox Suggest operations to avoid blocking. (#20401) * Fix Firefox Suggest operations to avoid blocking. This commit: * Refactors `RustFirefoxSuggest` to use a pair of serial dispatch queues to execute blocking read and write operations on the underlying `SuggestStore`. Previously, these operations would block, because calling a blocking function from an actor method (whether isolated or not) does _not_ automatically execute that function on a "global concurrent executor"; it can starve the calling thread. * Adopts the new `SuggestStore.interrupt(kind:)` API to implement `RustFirefoxSuggest.interruptEverything()`, which interrupts all ongoing operations. * Adds an expiration handler for the background ingestion task that calls `interruptEverything()`. * Make `RustFirefoxSuggest` a class, not an actor. Isolated methods on an actor are serialized, to protect the actor's mutable state. This is undesired for `RustFirefoxSuggest`, because it doesn't have any mutable state, and uses GCD queues to run read and write operations concurrently and off-main-thread. Using an actor here would serialize these operations, because isolated methods can't run concurrently. Instead, we can make `RustFirefoxSuggest` a regular class, and use `withCheckedThrowingContinuation` to expose an `await`-compatible API. --- ...ackgroundFirefoxSuggestIngestUtility.swift | 10 +++- firefox-ios/Providers/Profile.swift | 4 +- .../Storage/Rust/RustFirefoxSuggest.swift | 54 ++++++++++++++----- .../Tests/ClientTests/Mocks/MockProfile.swift | 4 +- .../SearchViewControllerTests.swift | 6 ++- 5 files changed, 58 insertions(+), 20 deletions(-) diff --git a/firefox-ios/Client/Application/BackgroundFirefoxSuggestIngestUtility.swift b/firefox-ios/Client/Application/BackgroundFirefoxSuggestIngestUtility.swift index facdd551b844..36fb95ae3962 100644 --- a/firefox-ios/Client/Application/BackgroundFirefoxSuggestIngestUtility.swift +++ b/firefox-ios/Client/Application/BackgroundFirefoxSuggestIngestUtility.swift @@ -13,11 +13,11 @@ import Storage class BackgroundFirefoxSuggestIngestUtility: BackgroundUtilityProtocol, FeatureFlaggable { static let taskIdentifier = "org.mozilla.ios.firefox.suggest.ingest" - let firefoxSuggest: RustFirefoxSuggestActor + let firefoxSuggest: RustFirefoxSuggestProtocol let logger: Logger private var didRegisterTaskHandler = false - init(firefoxSuggest: RustFirefoxSuggestActor, logger: Logger = DefaultLogger.shared) { + init(firefoxSuggest: RustFirefoxSuggestProtocol, logger: Logger = DefaultLogger.shared) { self.firefoxSuggest = firefoxSuggest self.logger = logger @@ -72,6 +72,12 @@ class BackgroundFirefoxSuggestIngestUtility: BackgroundUtilityProtocol, FeatureF private func setUp() { guard featureFlags.isFeatureEnabled(.firefoxSuggestFeature, checking: .buildAndUser) else { return } BGTaskScheduler.shared.register(forTaskWithIdentifier: Self.taskIdentifier, using: nil) { task in + task.expirationHandler = { + // Interrupt all ongoing storage operations if our + // background time is about to expire. + self.firefoxSuggest.interruptEverything() + task.setTaskCompleted(success: false) + } Task { let success = await self.ingest() if !success { diff --git a/firefox-ios/Providers/Profile.swift b/firefox-ios/Providers/Profile.swift index 4313c26db97c..d8752a9d59d0 100644 --- a/firefox-ios/Providers/Profile.swift +++ b/firefox-ios/Providers/Profile.swift @@ -82,7 +82,7 @@ protocol Profile: AnyObject { var files: FileAccessor { get } var pinnedSites: PinnedSites { get } var logins: RustLogins { get } - var firefoxSuggest: RustFirefoxSuggestActor? { get } + var firefoxSuggest: RustFirefoxSuggestProtocol? { get } var certStore: CertStore { get } var recentlyClosedTabs: ClosedTabsStore { get } @@ -615,7 +615,7 @@ open class BrowserProfile: Profile { return RustLogins(databasePath: databasePath) }() - lazy var firefoxSuggest: RustFirefoxSuggestActor? = { + lazy var firefoxSuggest: RustFirefoxSuggestProtocol? = { do { let cacheFileURL = try FileManager.default.url( for: .cachesDirectory, diff --git a/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift b/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift index 244835518c6f..8a84c9139196 100644 --- a/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift +++ b/firefox-ios/Storage/Rust/RustFirefoxSuggest.swift @@ -5,7 +5,7 @@ import Foundation import MozillaAppServices -public protocol RustFirefoxSuggestActor: Actor { +public protocol RustFirefoxSuggestProtocol { /// Downloads and stores new Firefox Suggest suggestions. func ingest() async throws @@ -17,14 +17,22 @@ public protocol RustFirefoxSuggestActor: Actor { ) async throws -> [RustFirefoxSuggestion] /// Interrupts any ongoing queries for suggestions. - nonisolated func interruptReader() + func interruptReader() + + /// Interrupts all ongoing operations. + func interruptEverything() } -/// An actor that wraps the synchronous Rust `SuggestStore` binding to execute -/// blocking operations on the default global concurrent executor. -public actor RustFirefoxSuggest: RustFirefoxSuggestActor { +/// Wraps the synchronous Rust `SuggestStore` binding to execute +/// blocking operations on a dispatch queue. +public class RustFirefoxSuggest: RustFirefoxSuggestProtocol { private let store: SuggestStore + // Using a pair of serial queues lets read and write operations run + // without blocking one another. + private let writerQueue = DispatchQueue(label: "RustFirefoxSuggest.writer") + private let readerQueue = DispatchQueue(label: "RustFirefoxSuggest.reader") + public init(dataPath: String, cachePath: String, remoteSettingsConfig: RemoteSettingsConfig? = nil) throws { var builder = SuggestStoreBuilder() .dataPath(path: dataPath) @@ -42,7 +50,16 @@ public actor RustFirefoxSuggest: RustFirefoxSuggestActor { // downloading new suggestions. This is safe to call multiple times. Viaduct.shared.useReqwestBackend() - try store.ingest(constraints: SuggestIngestionConstraints()) + try await withCheckedThrowingContinuation { continuation in + writerQueue.async(qos: .utility) { + do { + try self.store.ingest(constraints: SuggestIngestionConstraints()) + continuation.resume() + } catch { + continuation.resume(throwing: error) + } + } + } } public func query( @@ -50,14 +67,27 @@ public actor RustFirefoxSuggest: RustFirefoxSuggestActor { providers: [SuggestionProvider], limit: Int32 ) async throws -> [RustFirefoxSuggestion] { - return try store.query(query: SuggestionQuery( - keyword: keyword, - providers: providers, - limit: limit - )).compactMap(RustFirefoxSuggestion.init) + return try await withCheckedThrowingContinuation { continuation in + readerQueue.async(qos: .userInitiated) { + do { + let suggestions = try self.store.query(query: SuggestionQuery( + keyword: keyword, + providers: providers, + limit: limit + )).compactMap(RustFirefoxSuggestion.init) ?? [] + continuation.resume(returning: suggestions) + } catch { + continuation.resume(throwing: error) + } + } + } } - public nonisolated func interruptReader() { + public func interruptReader() { store.interrupt() } + + public func interruptEverything() { + store.interrupt(kind: .readWrite) + } } diff --git a/firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift b/firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift index 8b0aa9f5ad39..017c3f4b6325 100644 --- a/firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift +++ b/firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift @@ -108,14 +108,14 @@ open class MockProfile: Client.Profile { public var files: FileAccessor public var syncManager: ClientSyncManager! - public var firefoxSuggest: RustFirefoxSuggestActor? + public var firefoxSuggest: RustFirefoxSuggestProtocol? fileprivate let name: String = "mockaccount" private let directory: String private let databasePrefix: String - init(databasePrefix: String = "mock", firefoxSuggest: RustFirefoxSuggestActor? = nil) { + init(databasePrefix: String = "mock", firefoxSuggest: RustFirefoxSuggestProtocol? = nil) { files = MockFiles() syncManager = ClientSyncManagerSpy() self.databasePrefix = databasePrefix diff --git a/firefox-ios/firefox-ios-tests/Tests/ClientTests/SearchViewControllerTests.swift b/firefox-ios/firefox-ios-tests/Tests/ClientTests/SearchViewControllerTests.swift index 3c71da9b530b..4c31a759031d 100644 --- a/firefox-ios/firefox-ios-tests/Tests/ClientTests/SearchViewControllerTests.swift +++ b/firefox-ios/firefox-ios-tests/Tests/ClientTests/SearchViewControllerTests.swift @@ -9,7 +9,7 @@ import Shared @testable import Client -actor MockRustFirefoxSuggest: RustFirefoxSuggestActor { +class MockRustFirefoxSuggest: RustFirefoxSuggestProtocol { func ingest() async throws { } func query( @@ -36,7 +36,9 @@ actor MockRustFirefoxSuggest: RustFirefoxSuggestActor { } return suggestions } - nonisolated func interruptReader() { + func interruptReader() { + } + func interruptEverything() { } }