From 582dc673dd4be5a737bc6944a7981dfba0eea9fc Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Wed, 15 Mar 2023 11:53:08 -0700 Subject: [PATCH] Fix some bugs with async open task cancellation There were some very specific timings where cancelling the Task for an async open could either crash or fail to call the completion handler. Waiting for downloads when the Realm was already open would not cancel the download. --- CHANGELOG.md | 3 + Realm/RLMAsyncTask.mm | 176 ++++++++++++++++++++++++++--------- Realm/RLMAsyncTask_Private.h | 12 ++- RealmSwift/Realm.swift | 48 +++++++--- 4 files changed, 180 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a2a7f678d..07ec878504 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ x.y.z Release notes (yyyy-MM-dd) * ([#????](https://github.com/realm/realm-swift/issues/????), since v?.?.?) * Add missing `@Sendable` annotations to several sync and app services related callbacks ([PR #8169](https://github.com/realm/realm-swift/pull/8169), since v10.34.0). +* Fix some bugs in handling task cancellation for async Realm init. Some very + specific timing windows could cause crashes, and the download would not be + cancelled if the Realm was already open ([PR #8178](https://github.com/realm/realm-swift/pull/8178), since v10.37.0). diff --git a/Realm/RLMAsyncTask.mm b/Realm/RLMAsyncTask.mm index fbc0dfbf17..9f0a3004e0 100644 --- a/Realm/RLMAsyncTask.mm +++ b/Realm/RLMAsyncTask.mm @@ -19,6 +19,7 @@ #import "RLMAsyncTask_Private.h" #import "RLMError_Private.hpp" +#import "RLMRealm_Private.hpp" #import "RLMRealmConfiguration_Private.hpp" #import "RLMScheduler.h" #import "RLMSyncSubscription_Private.h" @@ -26,6 +27,7 @@ #import #import +#import #import static dispatch_queue_t s_async_open_queue = dispatch_queue_create("io.realm.asyncOpenDispatchQueue", @@ -49,7 +51,7 @@ @implementation RLMAsyncOpenTask { RLMRealmConfiguration *_configuration; RLMScheduler *_scheduler; bool _waitForDownloadCompletion; - RLMAsyncOpenRealmCallback _completion; + void (^_completion)(NSError *); RLMRealm *_backgroundRealm; } @@ -82,22 +84,13 @@ - (void)cancel { _progressBlocks.clear(); if (_task) { _task->cancel(); + // Cancelling realm::AsyncOpenTask results in it never calling our callback, + // so if we're currently in that we have to just send the cancellation + // error immediately. In all other cases, though, we want to wait until + // we've actually cancelled and will send the error the next time we + // check for cancellation + [self reportError:s_canceledError]; } - [self reportError:s_canceledError]; -} - -- (void)setTask:(std::shared_ptr)task __attribute__((objc_direct)) { - std::lock_guard lock(_mutex); - if (_cancel) { - task->cancel(); - return; - } - - _task = task; - for (auto& block : _progressBlocks) { - _task->register_download_progress_notifier(block); - } - _progressBlocks.clear(); } - (instancetype)initWithConfiguration:(RLMRealmConfiguration *)configuration @@ -127,12 +120,22 @@ - (instancetype)initWithConfiguration:(RLMRealmConfiguration *)configuration } - (void)waitForOpen:(RLMAsyncOpenRealmCallback)completion { - { - std::lock_guard lock(_mutex); - _completion = completion; - if (_cancel) { - return [self reportError:s_canceledError]; + __weak auto weakSelf = self; + [self waitWithCompletion:^(NSError *error) { + RLMRealm *realm; + if (auto self = weakSelf) { + realm = self->_localRealm; + self->_localRealm = nil; } + completion(realm, error); + }]; +} + +- (void)waitWithCompletion:(void (^)(NSError *))completion { + std::lock_guard lock(_mutex); + _completion = completion; + if (_cancel) { + return [self reportError:s_canceledError]; } // get_synchronized_realm() synchronously opens the DB and performs file-format @@ -144,23 +147,52 @@ - (void)waitForOpen:(RLMAsyncOpenRealmCallback)completion { }); } +// The full async open flow is: +// 1. Dispatch to a background queue +// 2. Use Realm::get_synchronized_realm() to create the Realm file, run +// migrations and compactions, and download the latest data from the server. +// 3. Dispatch back to queue +// 4. Initialize a RLMRealm in the background queue to perform the SDK +// initialization (e.g. creating managed accessor classes). +// 5. Wait for initial flexible sync subscriptions to complete +// 6. Dispatch to the final scheduler +// 7. Open the final RLMRealm, release the previously opened background one, +// and then invoke the completion callback. +// +// Steps 2 and 5 are skipped for non-sync or non-flexible sync Realms, in which +// case step 4 will handle doing migrations and compactions etc. in the background. +// +// At any point `cancel` can be called from another thread. Cancellation is mostly +// cooperative rather than preemptive: we check at each step if we've been cancelled, +// and if so call the completion with the cancellation error rather than +// proceeding. Downloading the data from the server is the one exception to this. +// Ideally waiting for flexible sync subscriptions would also be preempted. - (void)startAsyncOpen { + std::unique_lock lock(_mutex); if ([self checkCancellation]) { return; } if (_waitForDownloadCompletion && _configuration.configRef.sync_config) { #if REALM_ENABLE_SYNC - auto task = realm::Realm::get_synchronized_realm(_configuration.config); - self.task = task; - task->start([=](realm::ThreadSafeReference ref, std::exception_ptr err) { + _task = realm::Realm::get_synchronized_realm(_configuration.config); + for (auto& block : _progressBlocks) { + _task->register_download_progress_notifier(block); + } + _progressBlocks.clear(); + _task->start([=](realm::ThreadSafeReference ref, std::exception_ptr err) { + std::lock_guard lock(_mutex); if ([self checkCancellation]) { return; } + // Note that cancellation intentionally trumps reporting other kinds + // of errors if (err) { return [self reportException:err]; } + // Dispatch blocks can only capture copyable types, so we need to + // resolve the TSR to a shared_ptr auto realm = ref.resolve>(nullptr); // We're now running on the sync worker thread, so hop back // to a more appropriate queue for the next stage of init. @@ -178,12 +210,15 @@ - (void)startAsyncOpen { #endif } else { - // We're not downloading first, so just pretend it completed successfully + // We're not downloading first, so just proceed directly to the next step. + lock.unlock(); [self downloadCompleted]; } } - (void)downloadCompleted { + std::unique_lock lock(_mutex); + _task.reset(); if ([self checkCancellation]) { return; } @@ -212,36 +247,53 @@ - (void)downloadCompleted { return [subscriptions waitForSynchronizationOnQueue:nil completionBlock:^(NSError *error) { if (error) { + std::lock_guard lock(_mutex); return [self reportError:error]; } - [self completeAsyncOpen]; + [self asyncOpenCompleted]; }]; } } #endif - [self completeAsyncOpen]; + lock.unlock(); + [self asyncOpenCompleted]; } -- (void)completeAsyncOpen { - if ([self checkCancellation]) { - return; +- (void)asyncOpenCompleted { + std::lock_guard lock(_mutex); + if (![self checkCancellation]) { + [_scheduler invoke:^{ + [self openFinalRealmAndCallCompletion]; + }]; } +} - [_scheduler invoke:^{ - @autoreleasepool { - NSError *error; - RLMRealm *localRealm = [RLMRealm realmWithConfiguration:_configuration - confinedTo:_scheduler - error:&error]; - auto completion = _completion; - [self releaseResources]; - completion(localRealm, error); +- (void)openFinalRealmAndCallCompletion { + std::unique_lock lock(_mutex); + @autoreleasepool { + if ([self checkCancellation]) { + return; } - }]; + if (!_completion) { + return; + } + NSError *error; + auto completion = _completion; + // It should not actually be possible for this to fail + _localRealm = [RLMRealm realmWithConfiguration:_configuration + confinedTo:_scheduler + error:&error]; + [self releaseResources]; + + lock.unlock(); + completion(error); + } } - (bool)checkCancellation { - std::lock_guard lock(_mutex); + if (_cancel && _completion) { + [self reportError:s_canceledError]; + } return _cancel; } @@ -263,11 +315,15 @@ - (void)reportException:(std::exception_ptr const&)err { } - (void)reportError:(NSError *)error { + if (!_completion || !_scheduler) { + return; + } + auto completion = _completion; auto scheduler = _scheduler; [self releaseResources]; [scheduler invoke:^{ - completion(nil, error); + completion(error); }]; } @@ -278,3 +334,39 @@ - (void)releaseResources { _completion = nil; } @end + +@implementation RLMAsyncDownloadTask { + RLMUnfairMutex _mutex; + std::shared_ptr _session; + bool _started; +} + +- (instancetype)initWithRealm:(RLMRealm *)realm { + if (self = [super init]) { + _session = realm->_realm->sync_session(); + } + return self; +} + +- (void)waitWithCompletion:(void (^)(NSError *_Nullable))completion { + std::unique_lock lock(_mutex); + if (!_session) { + lock.unlock(); + return completion(nil); + } + + _started = true; + _session->revive_if_needed(); + _session->wait_for_download_completion([=](realm::Status status) { + completion(makeError(status)); + }); +} + +- (void)cancel { + std::unique_lock lock(_mutex); + if (_started) { + _session->force_close(); + } + _session = nullptr; +} +@end diff --git a/Realm/RLMAsyncTask_Private.h b/Realm/RLMAsyncTask_Private.h index c7c47bb405..98b1db7c4d 100644 --- a/Realm/RLMAsyncTask_Private.h +++ b/Realm/RLMAsyncTask_Private.h @@ -34,8 +34,16 @@ __attribute__((objc_direct)); - (instancetype)initWithConfiguration:(RLMRealmConfiguration *)configuration confinedTo:(RLMScheduler *)confinement download:(bool)waitForDownloadCompletion; -- (void)waitForOpen:(RLMAsyncOpenRealmCallback)completion - __attribute__((swift_attr("@_unsafeInheritExecutor"))); + +- (void)waitWithCompletion:(void (^)(NSError *_Nullable))completion; +- (void)waitForOpen:(RLMAsyncOpenRealmCallback)completion __attribute__((objc_direct)); +@end + +RLM_SWIFT_SENDABLE +@interface RLMAsyncDownloadTask : NSObject +- (instancetype)initWithRealm:(RLMRealm *)realm; +- (void)cancel; +- (void)waitWithCompletion:(void (^)(NSError *_Nullable))completion; @end RLM_HEADER_AUDIT_END(nullability) diff --git a/RealmSwift/Realm.swift b/RealmSwift/Realm.swift index 50dd40b57b..51e603d12e 100644 --- a/RealmSwift/Realm.swift +++ b/RealmSwift/Realm.swift @@ -1220,9 +1220,10 @@ extension Realm { if let realm = realm { // This can't be hit on the first open so .once == .never if downloadBeforeOpen == .always { - try await realm.waitForDownloadCompletion() + let task = RLMAsyncDownloadTask(realm: realm) + try await task.waitWithCancellationHandler() } - self = Realm(realm) + rlmRealm = realm return } @@ -1231,18 +1232,8 @@ extension Realm { let task = RLMAsyncOpenTask(configuration: rlmConfiguration, confinedTo: scheduler, download: shouldAsyncOpen(configuration, downloadBeforeOpen)) do { - // Work around https://github.com/apple/swift/issues/61119, which - // makes it impossible to use withTaskCancellationHandler() from - // within an isolated function without getting warnings - nonisolated func workaround() async throws { - try await withTaskCancellationHandler { @Sendable in - task.localRealm = try await task.waitForOpen() - } onCancel: { @Sendable in - task.cancel() - } - } - try await workaround() - self = Realm(task.localRealm!) + try await task.waitWithCancellationHandler() + rlmRealm = task.localRealm! task.localRealm = nil } catch { // Check if the task was cancelled and if so replace the error @@ -1252,7 +1243,34 @@ extension Realm { } } } -#endif // swift(>=5.5) + +@available(macOS 10.15, tvOS 13.0, iOS 13.0, watchOS 6.0, *) +private protocol TaskWithCancellation: Sendable { + func waitWithCancellationHandler() async throws + func wait() async throws + func cancel() +} + +@available(macOS 10.15, tvOS 13.0, iOS 13.0, watchOS 6.0, *) +extension TaskWithCancellation { + func waitWithCancellationHandler() async throws { + do { + try await withTaskCancellationHandler { + try await wait() + } onCancel: { + cancel() + } + } catch { + // Check if the task was cancelled and if so replace the error + // with reporting cancellation + try Task.checkCancellation() + throw error + } + } +} +extension RLMAsyncOpenTask: TaskWithCancellation {} +extension RLMAsyncDownloadTask: TaskWithCancellation {} +#endif // canImport(_Concurrency) /** Objects which can be fetched from the Realm - Object or Projection