Refactor async open to add support for Task cancellation #8148

tgoyne · 2023-02-23T17:40:32Z

This has most of the supporting refactoring to enable actor-confined Realms, and some of it won't make a ton of sense without that part.

This adjusts the behavior of async open cancellation a bit. We used to sometimes report a cancellation error and sometimes just never invoke the callback at all. This was weird and inconsistent and doesn't work for async continuations (which need to always be completed), so it now always reports an error.

withTaskCancellationHandler() has a really awkward API which drives the design of this. It needs an object which is allocated before calling withTaskCancellationHandler() which is then shared by the two callbacks passed to withTaskCancellationHandler(), which means that creating the RLMAsyncOpenTask and starting it need to be separate steps.

RLMAsyncTask currently only has RLMAsyncOpenTask, but with actor-confined Realms there's an assortment of async tasks defined there (async write and async refresh).

RLMConfinement stores all of the various things that a Realm can be confined to, which soon will be either a runloop, queue, or actor. It has an awkward definition to make it constructible in Swift but still retain things when used in C++. This is a maybe-not-actually-legal pattern used extensively by Foundation and libdispatch.

Setting SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL=2 makes the process abort when actor data races are detected rather than just logging a warning.

jsflax · 2023-03-01T12:47:09Z

Realm/RLMRealm.mm

@@ -484,11 +371,11 @@ + (instancetype)realmWithConfiguration:(RLMRealmConfiguration *)configuration
 realm->_dynamic = dynamic;

 // protects the realm cache and accessors cache
- static std::mutex& initLock = *new std::mutex();
- std::lock_guard<std::mutex> lock(initLock);
+ static auto& initLock = *new RLMUnfairMutex;


Why do we want this to be static? I.e., isn't this locking in the case of multiple realms?

The scope of this lock can probably be reduced, but all of this code has always been written under the assumption that it's called with a global lock held and changing that is an unrelated project.

jsflax · 2023-03-01T12:47:51Z

Realm/RLMRealmUtil.mm

+ }
+ else if (id actor = options.actor) {
+ // check for main actor?
+ return (__bridge void *)actor;


How are we checking actor equality?

Actors are reference types, so pointer equality works fine.

tgoyne · 2023-03-01T19:52:20Z

RLMConfinement has turned out to be miserable to work with so I'm reworking things to replace it with a RLMScheduler type that's an ordinary obj-c class and works similarly to the object store Scheduler type.

dianaafanador3

First overview, I have to finish reviewing the recent changes

dianaafanador3 · 2023-03-01T17:39:49Z

Realm.xcodeproj/xcshareddata/xcschemes/RealmSwift.xcscheme

@@ -86,6 +86,13 @@
 ReferencedContainer = "container:Realm.xcodeproj">
 </BuildableReference>
 </MacroExpansion>
+ <EnvironmentVariables>
+ <EnvironmentVariable
+ key = "SWIFT_UNEXPECTED_EXECUTOR_LOG_LEVEL"


Will this make the app crash in cases where the executor fails?

By default the actor data race detection merely logs a warning if it detects a problem, and we want our tests to visibly fail if they have a bug.

Realm/ObjectServerTests/RLMFlexibleSyncServerTests.mm

RealmSwift/Tests/RealmTests.swift

RealmSwift/Tests/TestUtils.swift

dianaafanador3 · 2023-03-01T19:57:26Z

Realm/RLMUtil.hpp

+// checking eliminates a lot of the benefit of using os_unfair_lock (better perf
+// and less storage required), so only use os_unfair_lock when our minimum
+// deployment target is high enough
+#if __clang_major__ >= 14


Do you think this change may solve some issues we have like this one RLMUnfairMutex?, I know this was introduced a few releases ago but I'm curious if this change may solve this issues'

Using os_unfair_mutex is purely just a minor performance optimization.

dianaafanador3 · 2023-03-01T20:06:30Z

RealmSwift/Realm.swift

@@ -1192,35 +1204,54 @@ extension Realm {
 @MainActor


Does this mean that we are still constrained to the the main actor to call try await Realm() ?

Yes, this does not add support for confining Realms to arbitrary actors.

dianaafanador3

LGTM! Just some minor comments and questions.

Realm/RLMRealm_Private.h

dianaafanador3 · 2023-03-01T21:21:57Z

Realm/RLMScheduler.h

+
+#import <Realm/RLMConstants.h>
+
+#ifdef __cplusplus


Are we exposing this to the Objective-C API but this can't be used, should we expose this in our private API so swift can use it.

Or maybe this is intended for future PRs

RLMRealm.mm gets osScheduler from RLMScheduler, but that's a c++ type so it needs to not be exposed when Swift is importing this header (which it does for async Realm.init()).

dianaafanador3 · 2023-03-01T21:57:42Z

Realm/RLMAsyncTask.mm

+ error:&error];
+ auto completion = _completion;
+ [self releaseResources];
+ completion(localRealm, error);


Are there some cases where we have an error and we return a Realm, or this never happens?

No, we always either have a Realm or an error.

Realm/ObjectServerTests/SwiftObjectServerTests.swift

tgoyne self-assigned this Feb 23, 2023

cla-bot bot added the cla: yes label Feb 23, 2023

tgoyne force-pushed the tg/actor-refactoring branch 7 times, most recently from 87dc526 to 55fc1d0 Compare February 28, 2023 18:56

tgoyne marked this pull request as ready for review February 28, 2023 20:11

tgoyne requested review from leemaguire and dianaafanador3 February 28, 2023 20:11

jsflax reviewed Mar 1, 2023

View reviewed changes

dianaafanador3 reviewed Mar 1, 2023

View reviewed changes

tgoyne force-pushed the tg/actor-refactoring branch from 516d6bf to 25eedd3 Compare March 1, 2023 20:50

dianaafanador3 approved these changes Mar 1, 2023

View reviewed changes

tgoyne added 3 commits March 1, 2023 17:43

Refactor async open to add support for Task cancellation

ed133a7

Disable exponential backoff on the test server

81caf78

Replace RLMConfinement with RLMScheduler

2a307d1

tgoyne force-pushed the tg/actor-refactoring branch from 25eedd3 to 2a307d1 Compare March 2, 2023 01:44

tgoyne added 3 commits March 2, 2023 20:07

Fix occasional crashes in testStreamingUploadNotifier

326b073

Make the timeout for creating a baas app longer

dc6b925

Maybe fix intermittent failures in offline autoopen tests

e9a2daa

tgoyne force-pushed the tg/actor-refactoring branch from 82c1ed9 to e9a2daa Compare March 3, 2023 05:04

tgoyne merged commit 8524c4e into master Mar 3, 2023

tgoyne deleted the tg/actor-refactoring branch March 3, 2023 21:14

github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor async open to add support for Task cancellation #8148

Refactor async open to add support for Task cancellation #8148

tgoyne commented Feb 23, 2023

jsflax Mar 1, 2023

tgoyne Mar 1, 2023

jsflax Mar 1, 2023

tgoyne Mar 1, 2023

tgoyne commented Mar 1, 2023

dianaafanador3 left a comment

dianaafanador3 Mar 1, 2023

tgoyne Mar 1, 2023

dianaafanador3 Mar 1, 2023 •

edited

Loading

tgoyne Mar 1, 2023

dianaafanador3 Mar 1, 2023

tgoyne Mar 1, 2023

dianaafanador3 left a comment

dianaafanador3 Mar 1, 2023

dianaafanador3 Mar 1, 2023

tgoyne Mar 2, 2023

dianaafanador3 Mar 1, 2023

tgoyne Mar 2, 2023


		#import <Realm/RLMConstants.h>

		#ifdef __cplusplus

Refactor async open to add support for Task cancellation #8148

Refactor async open to add support for Task cancellation #8148

Conversation

tgoyne commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgoyne commented Mar 1, 2023

dianaafanador3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianaafanador3 Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianaafanador3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianaafanador3 Mar 1, 2023 •

edited

Loading