Merge pull request #17 from realm/tg-multiprocess-schema-init

Fix race condition in multiprocess schema init

src/object_store.cpp

@@ -52,13 +52,11 @@ bool ObjectStore::has_metadata_tables(const Group *group) {
     return group->get_table(c_primaryKeyTableName) && group->get_table(c_metadataTableName);
 }
 
-bool ObjectStore::create_metadata_tables(Group *group) {
+void ObjectStore::create_metadata_tables(Group *group) {
-    bool changed = false;
     TableRef table = group->get_or_add_table(c_primaryKeyTableName);
     if (table->get_column_count() == 0) {
         table->add_column(type_String, c_primaryKeyObjectClassColumnName);
         table->add_column(type_String, c_primaryKeyPropertyNameColumnName);
-        changed = true;
     }
 
     table = group->get_or_add_table(c_metadataTableName);
@@ -68,10 +66,7 @@ bool ObjectStore::create_metadata_tables(Group *group) {
         // set initial version
         table->add_empty_row();
         table->set_int(c_versionColumnIndex, c_zeroRowIndex, ObjectStore::NotVersioned);
-        changed = true;
     }
-
-    return changed;
 }
 
 uint64_t ObjectStore::get_schema_version(const Group *group) {
@@ -255,9 +250,7 @@ static void copy_property_values(const Property& source, const Property& destina
 // set references to tables on targetSchema and create/update any missing or out-of-date tables
 // if update existing is true, updates existing tables, otherwise validates existing tables
 // NOTE: must be called from within write transaction
-bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update_existing) {
+void ObjectStore::create_tables(Group *group, Schema &target_schema, bool update_existing) {
-    bool changed = false;
-
     // first pass to create missing tables
     std::vector<ObjectSchema *> to_update;
     for (auto& object_schema : target_schema) {
@@ -267,7 +260,6 @@ bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update
         // we will modify tables for any new objectSchema (table was created) or for all if update_existing is true
         if (update_existing || created) {
             to_update.push_back(&object_schema);
-            changed = true;
         }
     }
 
@@ -291,7 +283,6 @@ bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update
             table->remove_column(current_prop.table_column);
 
             current_prop.table_column = target_prop->table_column;
-            changed = true;
         }
 
         bool inserted_placeholder_column = false;
@@ -314,7 +305,6 @@ bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update
                 table->remove_column(current_prop.table_column);
                 ++deleted;
                 current_prop.table_column = npos;
-                changed = true;
             }
         }
 
@@ -339,8 +329,6 @@ bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update
                                                                      target_prop.is_nullable);
                         break;
                 }
-
-                changed = true;
             }
             else {
                 target_prop.table_column = current_prop->table_column;
@@ -361,16 +349,13 @@ bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update
             // if there is a primary key set, check if it is the same as the old key
             if (current_schema.primary_key != target_object_schema->primary_key) {
                 set_primary_key_for_object(group, target_object_schema->name, target_object_schema->primary_key);
-                changed = true;
             }
         }
         else if (current_schema.primary_key.length()) {
             // there is no primary key, so if there was one nil out
             set_primary_key_for_object(group, target_object_schema->name, "");
-            changed = true;
         }
     }
-    return changed;
 }
 
 bool ObjectStore::is_schema_at_version(const Group *group, uint64_t version) {
@@ -405,7 +390,7 @@ bool ObjectStore::needs_update(Schema const& old_schema, Schema const& schema) {
     return false;
 }
 
-bool ObjectStore::update_realm_with_schema(Group *group, Schema const& old_schema,
+void ObjectStore::update_realm_with_schema(Group *group, Schema const& old_schema,
                                            uint64_t version, Schema &schema,
                                            MigrationFunction migration) {
     // Recheck the schema version after beginning the write transaction as
@@ -414,8 +399,8 @@ bool ObjectStore::update_realm_with_schema(Group *group, Schema const& old_schem
     bool migrating = !is_schema_at_version(group, version);
 
     // create tables
-    bool changed = create_metadata_tables(group);
+    create_metadata_tables(group);
-    changed = create_tables(group, schema, migrating) || changed;
+    create_tables(group, schema, migrating);
 
     if (!migrating) {
         // If we aren't migrating, then verify that all of the tables which
@@ -423,10 +408,10 @@ bool ObjectStore::update_realm_with_schema(Group *group, Schema const& old_schem
         verify_schema(old_schema, schema, true);
     }
 
-    changed = update_indexes(group, schema) || changed;
+    update_indexes(group, schema);
 
     if (!migrating) {
-        return changed;
+        return;
     }
 
     // apply the migration block if provided and there's any old data
@@ -437,7 +422,6 @@ bool ObjectStore::update_realm_with_schema(Group *group, Schema const& old_schem
     }
 
     set_schema_version(group, version);
-    return true;
 }
 
 Schema ObjectStore::schema_from_group(const Group *group) {

src/object_store.hpp

@@ -52,12 +52,11 @@ namespace realm {
         static bool needs_update(Schema const& old_schema, Schema const& schema);
         
         // updates a Realm from old_schema to the given target schema, creating and updating tables as needed
-        // returns if any changes were made
         // passed in target schema is updated with the correct column mapping
         // optionally runs migration function if schema is out of date
         // NOTE: must be performed within a write transaction
         typedef std::function<void(Group *, Schema &)> MigrationFunction;
-        static bool update_realm_with_schema(Group *group, Schema const& old_schema, uint64_t version,
+        static void update_realm_with_schema(Group *group, Schema const& old_schema, uint64_t version,
                                              Schema &schema, MigrationFunction migration);
 
         // get a table for an object type
@@ -86,11 +85,11 @@ namespace realm {
         // create any metadata tables that don't already exist
         // must be in write transaction to set
         // returns true if it actually did anything
-        static bool create_metadata_tables(Group *group);
+        static void create_metadata_tables(Group *group);
 
         // set references to tables on targetSchema and create/update any missing or out-of-date tables
         // if update existing is true, updates existing tables, otherwise only adds and initializes new tables
-        static bool create_tables(realm::Group *group, Schema &target_schema, bool update_existing);
+        static void create_tables(realm::Group *group, Schema &target_schema, bool update_existing);
 
         // verify a target schema against an expected schema, setting the table_column property on each schema object
         // updates the column mapping on the target_schema

src/shared_realm.cpp

@@ -201,51 +201,75 @@ SharedRealm Realm::get_shared_realm(Config config)
     return realm;
 }
 
-bool Realm::update_schema(std::unique_ptr<Schema> schema, uint64_t version)
+void Realm::update_schema(std::unique_ptr<Schema> schema, uint64_t version)
 {
     schema->validate();
 
-    bool needs_update = !m_config.read_only && (m_config.schema_version != version || ObjectStore::needs_update(*m_config.schema, *schema));
+    auto needs_update = [&] {
-    if (!needs_update) {
+        // If the schema version matches, just verify that the schema itself also matches
+        bool needs_write = !m_config.read_only && (m_config.schema_version != version || ObjectStore::needs_update(*m_config.schema, *schema));
+        if (needs_write) {
+            return true;
+        }
+
         ObjectStore::verify_schema(*m_config.schema, *schema, m_config.read_only);
         m_config.schema = std::move(schema);
         m_config.schema_version = version;
         return false;
+    };
+
+    if (!needs_update()) {
+        return;
+    }
+
+    begin_transaction();
+    struct WriteTransactionGuard {
+        Realm& realm;
+        ~WriteTransactionGuard() {
+            if (realm.is_in_transaction()) {
+                realm.cancel_transaction();
+            }
+        }
+    } write_transaction_guard{*this};
+
+    // Recheck the schema version after beginning the write transaction
+    // If it changed then someone else initialized the schema and we need to
+    // recheck everything
+    auto current_schema_version = ObjectStore::get_schema_version(read_group());
+    if (current_schema_version != m_config.schema_version) {
+        m_config.schema_version = current_schema_version;
+        *m_config.schema = ObjectStore::schema_from_group(read_group());
+
+        if (!needs_update()) {
+            cancel_transaction();
+            return;
+        }
     }
 
-    // Store the old config/schema for the migration function, and update
-    // our schema to the new one
-    auto old_schema = std::move(m_config.schema);
     Config old_config(m_config);
-    old_config.read_only = true;
-    old_config.schema = std::move(old_schema);
-
-    m_config.schema = std::move(schema);
-    m_config.schema_version = version;
-
     auto migration_function = [&](Group*,  Schema&) {
         SharedRealm old_realm(new Realm(old_config));
-        auto updated_realm = shared_from_this();
+        // Need to open in read-write mode so that it uses a SharedGroup, but
+        // users shouldn't actually be able to write via the old realm
+        old_realm->m_config.read_only = true;
+
         if (m_config.migration_function) {
-            m_config.migration_function(old_realm, updated_realm);
+            m_config.migration_function(old_realm, shared_from_this());
         }
     };
 
     try {
-        // update and migrate
+        m_config.schema = std::move(schema);
-        begin_transaction();
+        m_config.schema_version = version;
-        bool changed = ObjectStore::update_realm_with_schema(read_group(), *old_config.schema,
+
-                                                             version, *m_config.schema,
+        ObjectStore::update_realm_with_schema(read_group(), *old_config.schema,
-                                                             migration_function);
+                                              version, *m_config.schema,
+                                              migration_function);
         commit_transaction();
-        return changed;
     }
     catch (...) {
-        if (is_in_transaction()) {
-            cancel_transaction();
-        }
-        m_config.schema_version = old_config.schema_version;
         m_config.schema = std::move(old_config.schema);
+        m_config.schema_version = old_config.schema_version;
         throw;
     }
 }

src/shared_realm.hpp

@@ -79,8 +79,7 @@ namespace realm {
         // updating indexes as necessary. Uses the existing migration function
         // on the Config, and the resulting Schema and version with updated
         // column mappings are set on the realms config upon success.
-        // returns if any changes were made
+        void update_schema(std::unique_ptr<Schema> schema, uint64_t version);
-        bool update_schema(std::unique_ptr<Schema> schema, uint64_t version);
 
         static uint64_t get_schema_version(Config const& config);