realm · tgoyne · Jan 17, 2023 · Jan 19, 2023
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,16 +2,20 @@
 
 ### Enhancements
 * `SyncSession::pause()` and `SyncSession::resume()` allow users to suspend a Realm's sync session until it is explicitly resumed in ([#6183](https://github.com/realm/realm-core/pull/6183)). Previously `SyncSession::log_out()` and `SyncSession::close()` could be resumed under a number of circumstances where `SyncSession::revive_if_needed()` were called (like when freezing a realm) - fixes ([#6085](https://github.com/realm/realm-core/issues/6085))
+* Improve the performance of `Realm::freeze()` by eliminating some redudant work around schema initialization and validation. These optimizations do not apply to Realm::get_frozen_realm() ([PR #6211](https://github.com/realm/realm-core/pull/6211)).
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
 * "find first" on Decimal128 field with value NaN does not find objects ([6182](https://github.com/realm/realm-core/issues/6182), since v6.0.0)
 * Value in List of Mixed would not be updated if new value is Binary and old value is StringData and the values otherwise matches ([#6201](https://github.com/realm/realm-core/issues/6201), since v6.0.0)
 * When client reset with recovery is used and the recovery does not actually result in any new local commits, the sync client may have gotten stuck in a cycle with a `A fatal error occured during client reset: 'A previous 'Recovery' mode reset from <timestamp> did not succeed, giving up on 'Recovery' mode to prevent a cycle'` error message. ([#6195](https://github.com/realm/realm-core/issues/6195), since v11.16.0)
+* Fix several data races when opening cached frozen Realms. New frozen Realms were added to the cache and the lock released before they were fully initialized, resulting in races if they were immediately read from the cache on another thread ([PR #6211](https://github.com/realm/realm-core/pull/6211), since v6.0.0).
+* Properties and types not present in the requested schema would be missing from the reported schema in several scenarios, such as if the Realm was being opened with a different schema version than the persisted one, and if the new tables or columns were added while the Realm instance did not have an active read transaction ([PR #6211](https://github.com/realm/realm-core/pull/6211), since v13.2.0).
 
 ### Breaking changes
 * `SyncSession::log_out()` has been renamed to `SyncSession::force_close()` to reflect what it actually does ([#6183](https://github.com/realm/realm-core/pull/6183))
 * Passing an empty `key_path_array` to `add_notification_callback now` now ignores nested property changes. Pass `std::nullopt` to achieve the old meaning. ([#6122](https://github.com/realm/realm-core/pull/6122))
 * Whether to report the file's complete schema or only the requested schema is now an option on RealmConfig (schema_subset_mode) rather than always being enabled for Additive schema modes. All schema modes which this applies to are now supported ([PR #6211](https://github.com/realm/realm-core/pull/6211)).
 
 ### Compatibility
 * Fileformat: Generates files with format v23. Reads and automatically upgrade from fileformat v5.
@@ -77,7 +81,7 @@
 
 ### Breaking changes
 * FLX Subscription API reworked to better match SDK consumption patterns ([#6065](https://github.com/realm/realm-core/pull/6065)). Not all changes are breaking, but listing them all here together.
-  * `Subscription` is now a plain struct with public fields rather than getter functions    * `has_name()` and `name()` were merged into a single `optional<string> name` field
+  * `Subscription` is now a plain struct with public fields rather than getter functions
   * `has_name()` and `name()` were merged into a single `optional<string> name` field
   * `SubscriptionSet` now uses the same types for `iterator` and `const_iterator` since neither was intended to support direct mutability
   * `SubscriptionSet::get_state_change_notification()` now offers a callback-taking overload

diff --git a/src/realm/object-store/dictionary.cpp b/src/realm/object-store/dictionary.cpp
@@ -335,9 +335,10 @@ class NotificationHandler {
     Dictionary::CBFunc m_cb;
 };
 
-NotificationToken Dictionary::add_key_based_notification_callback(CBFunc cb, KeyPathArray key_path_array) &
+NotificationToken Dictionary::add_key_based_notification_callback(CBFunc cb,
+                                                                  std::optional<KeyPathArray> key_path_array) &
 {
-    return add_notification_callback(NotificationHandler(dict(), std::move(cb)), key_path_array);
+    return add_notification_callback(NotificationHandler(dict(), std::move(cb)), std::move(key_path_array));
 }
 
 Dictionary Dictionary::freeze(const std::shared_ptr<Realm>& frozen_realm) const

diff --git a/src/realm/object-store/dictionary.hpp b/src/realm/object-store/dictionary.hpp
@@ -118,7 +118,8 @@ class Dictionary : public object_store::Collection {
     Results get_values() const;
 
     using CBFunc = util::UniqueFunction<void(DictionaryChangeSet)>;
-    NotificationToken add_key_based_notification_callback(CBFunc cb, KeyPathArray key_path_array = {}) &;
+    NotificationToken
+    add_key_based_notification_callback(CBFunc cb, std::optional<KeyPathArray> key_path_array = std::nullopt) &;
 
     Iterator begin() const;
     Iterator end() const;

diff --git a/src/realm/object-store/impl/realm_coordinator.cpp b/src/realm/object-store/impl/realm_coordinator.cpp
@@ -247,9 +247,7 @@ std::shared_ptr<Realm> RealmCoordinator::get_realm(Realm::Config config, util::O
     util::CheckedUniqueLock lock(m_realm_mutex);
     set_config(config);
     if ((realm = do_get_cached_realm(config))) {
-        if (version) {
-            REALM_ASSERT(realm->read_transaction_version() == *version);
-        }
+        REALM_ASSERT(!version || realm->read_transaction_version() == *version);
         return realm;
     }
     do_get_realm(std::move(config), realm, version, lock);
@@ -269,15 +267,34 @@ std::shared_ptr<Realm> RealmCoordinator::get_realm(std::shared_ptr<util::Schedul
     return realm;
 }
 
+std::shared_ptr<Realm> RealmCoordinator::freeze_realm(const Realm& source_realm)
+{
+    std::shared_ptr<Realm> realm;
+    util::CheckedUniqueLock lock(m_realm_mutex);
+
+    auto version = source_realm.read_transaction_version();
+    auto scheduler = util::Scheduler::make_frozen(version);
+    if ((realm = do_get_cached_realm(source_realm.config(), scheduler))) {
+        return realm;
+    }
+
+    auto config = source_realm.config();
+    config.scheduler = scheduler;
+    realm = Realm::make_shared_realm(std::move(config), version, shared_from_this());
+    Realm::Internal::copy_schema(*realm, source_realm);
+    m_weak_realm_notifiers.emplace_back(realm, config.cache);
+    return realm;
+}
+
 ThreadSafeReference RealmCoordinator::get_unbound_realm()
 {
     std::shared_ptr<Realm> realm;
     util::CheckedUniqueLock lock(m_realm_mutex);
-    do_get_realm(m_config, realm, none, lock);
+    do_get_realm(RealmConfig(m_config), realm, none, lock);
     return ThreadSafeReference(realm);
 }
 
-void RealmCoordinator::do_get_realm(Realm::Config config, std::shared_ptr<Realm>& realm,
+void RealmCoordinator::do_get_realm(RealmConfig&& config, std::shared_ptr<Realm>& realm,
                                     util::Optional<VersionID> version, util::CheckedUniqueLock& realm_lock)
 {
     open_db();
@@ -305,6 +322,15 @@ void RealmCoordinator::do_get_realm(Realm::Config config, std::shared_ptr<Realm>
         REALM_TERMINATE("Cannot use Audit interface if Realm Core is built without Sync");
 #endif
 
+    // Cached frozen Realms need to initialize their schema before releasing
+    // the lock as otherwise they could be read from the cache on another thread
+    // before the schema initialization happens. They'll never perform a write
+    // transaction, so unlike with live Realms this is safe to do.
+    if (config.cache && version && schema) {
+        realm->update_schema(std::move(*schema));
+        schema.reset();
+    }
+
     realm_lock.unlock_unchecked();
     if (schema) {
         realm->update_schema(std::move(*schema), config.schema_version, std::move(migration_function),

diff --git a/src/realm/object-store/impl/realm_coordinator.hpp b/src/realm/object-store/impl/realm_coordinator.hpp
@@ -60,6 +60,11 @@ class RealmCoordinator : public std::enable_shared_from_this<RealmCoordinator> {
         REQUIRES(!m_realm_mutex, !m_schema_cache_mutex);
     std::shared_ptr<Realm> get_realm(std::shared_ptr<util::Scheduler> = nullptr)
         REQUIRES(!m_realm_mutex, !m_schema_cache_mutex);
+
+    // Return a frozen copy of the source Realm. May return a cached instance
+    // if the source Realm has caching enabled.
+    std::shared_ptr<Realm> freeze_realm(const Realm& source_realm) REQUIRES(!m_realm_mutex);
+
 #if REALM_ENABLE_SYNC
     // Get a thread-local shared Realm with the given configuration
     // If the Realm is not already present, it will be fully downloaded before being returned.
@@ -262,7 +267,7 @@ class RealmCoordinator : public std::enable_shared_from_this<RealmCoordinator> {
     std::shared_ptr<Realm> do_get_cached_realm(Realm::Config const& config,
                                                std::shared_ptr<util::Scheduler> scheduler = nullptr)
         REQUIRES(m_realm_mutex);
-    void do_get_realm(Realm::Config config, std::shared_ptr<Realm>& realm, util::Optional<VersionID> version,
+    void do_get_realm(Realm::Config&& config, std::shared_ptr<Realm>& realm, util::Optional<VersionID> version,
                       util::CheckedUniqueLock& realm_lock) REQUIRES(m_realm_mutex);
     void run_async_notifiers() REQUIRES(!m_notifier_mutex, m_running_notifiers_mutex);
     void clean_up_dead_notifiers() REQUIRES(m_notifier_mutex);

diff --git a/src/realm/object-store/object_accessor.hpp b/src/realm/object-store/object_accessor.hpp
@@ -264,7 +264,7 @@ Object Object::create(ContextType& ctx, std::shared_ptr<Realm> const& realm, Obj
     // considered a primary key by core, and so will need to be set.
     bool skip_primary = true;
     // If the input value is missing values for any of the properties we want to
-    // set the propery to the default value for new objects, but leave it
+    // set the property to the default value for new objects, but leave it
     // untouched for existing objects.
     bool created = false;
 

diff --git a/src/realm/object-store/object_store.cpp b/src/realm/object-store/object_store.cpp
@@ -875,7 +875,6 @@ void ObjectStore::apply_schema_changes(Transaction& group, uint64_t schema_versi
     }
 
     if (mode == SchemaMode::Manual) {
-        set_schema_keys(group, target_schema);
         if (migration_function) {
             migration_function();
         }

diff --git a/src/realm/object-store/schema.cpp b/src/realm/object-store/schema.cpp
@@ -313,12 +313,10 @@ void Schema::zip_matching(T&& a, U&& b, Func&& func)
             ++j;
         }
     }
-    for (; i < a.size(); ++i) {
+    for (; i < a.size(); ++i)
         func(&a[i], nullptr);
-    }
-    for (; j < b.size(); ++j) {
+    for (; j < b.size(); ++j)
         func(nullptr, &b[j]);
-    }
 }
 
 std::vector<SchemaChange> Schema::compare(Schema const& target_schema, SchemaMode mode,
@@ -343,10 +341,8 @@ std::vector<SchemaChange> Schema::compare(Schema const& target_schema, SchemaMod
 
     // Modify columns
     zip_matching(target_schema, *this, [&](const ObjectSchema* target, const ObjectSchema* existing) {
-        if (target && existing) {
+        if (target && existing)
             ::compare(*existing, *target, changes);
-        }
-
         else if (target && !orphans.count(target->name)) {
             // Target is a new table -- add all properties
             changes.emplace_back(schema_change::AddInitialProperties{target});
@@ -364,38 +360,34 @@ std::vector<SchemaChange> Schema::compare(Schema const& target_schema, SchemaMod
     return changes;
 }
 
-void Schema::copy_keys_from(Schema const& other, bool is_schema_additive)
+void Schema::copy_keys_from(realm::Schema const& other, SchemaSubsetMode subset_mode)
 {
-    std::vector<ObjectSchema> other_classes;
-    zip_matching(*this, other, [&](ObjectSchema const* existing, ObjectSchema const* other) {
-        if (is_schema_additive && !existing && other) {
-            other_classes.push_back(*other);
-        }
+    std::vector<const ObjectSchema*> other_classes;
+    zip_matching(*this, other, [&](ObjectSchema* existing, const ObjectSchema* other) {
+        if (subset_mode.include_types && !existing && other)
+            other_classes.push_back(other);
         if (!existing || !other)
             return;
-        update_or_append_properties(const_cast<ObjectSchema*>(existing), other, is_schema_additive);
+
+        existing->table_key = other->table_key;
+        for (auto& current_prop : other->persisted_properties) {
+            if (auto target_prop = existing->property_for_name(current_prop.name)) {
+                target_prop->column_key = current_prop.column_key;
+            }
+            else if (subset_mode.include_properties) {
+                existing->persisted_properties.push_back(current_prop);
+            }
+        }
     });
 
     if (!other_classes.empty()) {
-        insert(end(), other_classes.begin(), other_classes.end());
+        reserve(size() + other_classes.size());
+        for (auto other : other_classes)
+            push_back(*other);
         sort_schema();
     }
 }
 
-void Schema::update_or_append_properties(ObjectSchema* existing, const ObjectSchema* other, bool is_schema_additive)
-{
-    existing->table_key = other->table_key;
-    for (auto& current_prop : other->persisted_properties) {
-        auto target_prop = existing->property_for_name(current_prop.name);
-        if (target_prop) {
-            target_prop->column_key = current_prop.column_key;
-        }
-        else if (is_schema_additive) {
-            existing->persisted_properties.push_back(current_prop);
-        }
-    }
-}
-
 namespace realm {
 bool operator==(SchemaChange const& lft, SchemaChange const& rgt) noexcept
 {

diff --git a/src/realm/object-store/schema.hpp b/src/realm/object-store/schema.hpp
@@ -114,6 +114,43 @@ enum class SchemaMode : uint8_t {
     Manual
 };
 
+// Options for how to handle the schema when the file has classes and/or
+// properties not in the schema.
+//
+// Most schema modes allow the requested schema to be a subset of the actual
+// schema of the Realm file. By default, any properties or object types not in
+// the requested schema are simply ignored entirely and the Realm's in-memory
+// schema will always exactly match the requested one.
+struct SchemaSubsetMode {
+    // Add additional tables present in the Realm file to the schema. This is
+    // applicable to all schema modes except for Manual and ResetFile.
+    bool include_types : 1;
+
+    // Add additional columns in the tables present in the Realm file to the
+    // object schema for those types. The additional properties are always
+    // added to the end of persisted_properties. This is only applicable to
+    // Additive and ReadOnly schema modes.
+    bool include_properties : 1;
+
+    // The reported schema will always exactly match the requested one.
+    static const SchemaSubsetMode Strict;
+    // Additional object classes present in the Realm file are added to the
+    // requested schema, but all object types present in the requested schema
+    // will always exactly match even if there are additional columns in the
+    // tables.
+    static const SchemaSubsetMode AllClasses;
+    // Additional properties present in the Realm file are added to the
+    // requested schema, but tables not present in the schema are ignored.
+    static const SchemaSubsetMode AllProperties;
+    // Always report the complete schema.
+    static const SchemaSubsetMode Complete;
+};
+
+inline constexpr SchemaSubsetMode SchemaSubsetMode::Strict = {false, false};
+inline constexpr SchemaSubsetMode SchemaSubsetMode::AllClasses = {true, false};
+inline constexpr SchemaSubsetMode SchemaSubsetMode::AllProperties = {false, true};
+inline constexpr SchemaSubsetMode SchemaSubsetMode::Complete = {true, true};
+
 
 class Schema : private std::vector<ObjectSchema> {
 private:
@@ -151,7 +188,7 @@ class Schema : private std::vector<ObjectSchema> {
     std::vector<SchemaChange> compare(Schema const&, SchemaMode = SchemaMode::Automatic,
                                       bool include_removals = false) const;
 
-    void copy_keys_from(Schema const&, bool is_schema_additive = false);
+    void copy_keys_from(Schema const&, SchemaSubsetMode subset_mode);
 
     friend bool operator==(Schema const&, Schema const&) noexcept;
     friend bool operator!=(Schema const& a, Schema const& b) noexcept
@@ -171,8 +208,6 @@ class Schema : private std::vector<ObjectSchema> {
     static void zip_matching(T&& a, U&& b, Func&& func);
     // sort all the classes by name in order to speed up find(StringData name)
     void sort_schema();
-    // append missing properties and update matching properties for schema
-    void update_or_append_properties(ObjectSchema*, const ObjectSchema*, bool);
 };
 
 namespace schema_change {