Fix some bugs with async open task cancellation #8178
Conversation
tgoyne
force-pushed
the
tg/async-open-cancel
branch
2 times, most recently
from
7d58375 to
ac8effe
Compare
| if (_cancel) { | ||
| return [self reportError:s_canceledError]; | ||
| __weak auto weakSelf = self; | ||
| [self waitWithCompletion:^(NSError *error) { |
There was a problem hiding this comment.
Wait is the purpose of separating this into two steps?
There was a problem hiding this comment.
This is both to sidestep the sendable checking issues with cancellation handlers and to align the APIs for the two tasks so that the Swift code doesn't need to be duplicated. Passing a RLMRealm to the completion handler gives a spurious warning because when it's wrapped in a completion handler there's potentially a hop away from the correct executor and then back, and reading it from the token instead avoids that. These means that Swift wants a completion handler which just returns an error for both task types, but the obj-c (and Swift Realm.asyncOpen()) API needs a completion handler with a RLMRealm and error, so this wraps the applicable one to expose that.
| - (void)waitWithCompletion:(void (^)(NSError *))completion { | ||
| std::lock_guard lock(_mutex); | ||
| _completion = completion; | ||
| if (_cancel) { |
There was a problem hiding this comment.
can we do [self checkCancellation] here?
| } | ||
|
|
||
| - (bool)checkCancellation { | ||
| std::lock_guard lock(_mutex); | ||
| if (_cancel && _completion) { | ||
| [self reportError:s_canceledError]; |
There was a problem hiding this comment.
In case of cancel, shouldn't we unlock the lock as well?
There was a problem hiding this comment.
reportError: needs to be called with the lock held. The callback is invoked asynchronously on the scheduler so it's not invoked in a context where we're holding the lock.
|
|
||
| @available(macOS 10.15, tvOS 13.0, iOS 13.0, watchOS 6.0, *) | ||
| extension TaskWithCancellation { | ||
| func waitWithCancellationHandler() async throws { |
There was a problem hiding this comment.
Cool way to do the implementation for both RLMAsyncOpenTask and RLMAsyncDownloadTask
| } | ||
| } | ||
| } | ||
| extension RLMAsyncOpenTask: TaskWithCancellation {} |
There was a problem hiding this comment.
From the code, I understand that in a async await context we now use RLMAsyncDownloadTask if the realm is already open and RLMAsyncOpenTask if is the first time. Is that correct?
There was a problem hiding this comment.
Yes. The design of task cancellation in Swift means that any cancellable operation needs an object which represents that operation. RLMAsyncDownloadTask is mostly just an optimization - we could just go through the normal async open flow even if the Realm is already open - but I think the design of actor-confined Realms will lead to people hitting that code path fairly often.
| @@ -19,13 +19,15 @@ | |||
| #import "RLMAsyncTask_Private.h" | |||
|
|
|||
| #import "RLMError_Private.hpp" | |||
| #import "RLMRealm_Private.hpp" | |||
There was a problem hiding this comment.
It seems like opening asynchronously a realm is getting more complex, which is needed to avoid issues. I wish at least some small comments on each of the step for opening a realm for the first time asynchronously to understand why this step by step approach is needed.
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.
tgoyne
force-pushed
the
tg/async-open-cancel
branch
from
ac8effe to
aa4c3c1
Compare
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.
The tests which exposed these problems rely on actor-confined Realms and aren't easily ported to not depend on that, as they use a custom executor to test cancellation at every suspension point.