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.
realm · Mar 27, 2023 · aa4c3c1 · aa4c3c1
1 parent ba45055
commit aa4c3c1
Show file tree

Hide file tree

Showing 4 changed files with 180 additions and 59 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,9 @@ x.y.z Release notes (yyyy-MM-dd)
 * <How to hit and notice issue? what was the impact?> ([#????](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).
 
 <!-- ### Breaking Changes - ONLY INCLUDE FOR NEW MAJOR version -->
 

diff --git a/Realm/RLMAsyncTask.mm b/Realm/RLMAsyncTask.mm
@@ -19,13 +19,15 @@
 #import "RLMAsyncTask_Private.h"
 
 #import "RLMError_Private.hpp"
+#import "RLMRealm_Private.hpp"
 #import "RLMRealmConfiguration_Private.hpp"
 #import "RLMScheduler.h"
 #import "RLMSyncSubscription_Private.h"
 #import "RLMUtil.hpp"
 
 #import <realm/exceptions.hpp>
 #import <realm/object-store/sync/async_open_task.hpp>
+#import <realm/object-store/sync/sync_session.hpp>
 #import <realm/object-store/thread_safe_reference.hpp>
 
 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<realm::AsyncOpenTask>)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<Realm>
             auto realm = ref.resolve<std::shared_ptr<realm::Realm>>(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<realm::SyncSession> _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
@@ -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
@@ -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