Separate TestSyncManager and OfflineAppSession

Co-authored-by: James Stone <[email protected]>

CHANGELOG.md

@@ -22,7 +22,7 @@
 * Fix a minor race condition when backing up Realm files before a client reset which could have lead to overwriting an existing file. ([PR #7341](https://github.com/realm/realm-core/pull/7341)).
 
 ### Breaking changes
-* None.
+* SyncManager no longer supports reconfiguring after calling reset_for_testing(). SyncManager::configure() has been folded into the constructor, and reset_for_testing() has been renamed to tear_down_for_testing(). ([PR #7351](https://github.com/realm/realm-core/pull/7351))
 
 ### Compatibility
 * Fileformat: Generates files with format v23. Reads and automatically upgrade from fileformat v5.

src/realm/object-store/audit.mm

@@ -683,7 +683,7 @@ explicit AuditRealmPool(Private, std::shared_ptr<SyncUser> user, std::string con
                                  }),
                   s_pools.end());
 
-    auto app_id = user->sync_manager()->app().lock()->config().app_id;
+    auto app_id = user->sync_manager()->app_id();
     auto it = std::find_if(s_pools.begin(), s_pools.end(), [&](auto& pool) {
         return pool.user_identity == user->identity() && pool.partition_prefix == partition_prefix &&
                pool.app_id == app_id;
@@ -889,7 +889,6 @@ explicit AuditRealmPool(Private, std::shared_ptr<SyncUser> user, std::string con
 
     Realm::Config config;
     config.automatic_change_notifications = false;
-    config.cache = false;
     config.path = util::format("%1%2.realm", m_path_root, partition);
     config.scheduler = util::Scheduler::make_dummy();
     config.schema = Schema{schema};

src/realm/object-store/sync/app.cpp

@@ -313,8 +313,7 @@ void App::configure(const SyncClientConfig& sync_client_config)
     // Start with an empty sync route in the sync manager. It will ensure the
     // location has been updated at least once when the first sync session is
     // started by requesting a new access token.
-    m_sync_manager = std::make_shared<SyncManager>();
-    m_sync_manager->configure(shared_from_this(), util::none, sync_client_config);
+    m_sync_manager = SyncManager::create(shared_from_this(), {}, sync_client_config, config().app_id);
 }
 
 bool App::init_logger()

src/realm/object-store/sync/sync_manager.cpp

@@ -43,58 +43,37 @@ SyncClientTimeouts::SyncClientTimeouts()
 {
 }
 
-SyncManager::SyncManager() = default;
-
-void SyncManager::configure(std::shared_ptr<app::App> app, std::optional<std::string> sync_route,
-                            const SyncClientConfig& config)
+std::shared_ptr<SyncManager> SyncManager::create(std::shared_ptr<app::App> app, std::optional<std::string> sync_route,
+                                                 const SyncClientConfig& config, const std::string& app_id)
 {
-    std::vector<std::shared_ptr<SyncUser>> users_to_add;
-    {
-        // Locking the mutex here ensures that it is released before locking m_user_mutex
-        util::CheckedLockGuard lock(m_mutex);
-        m_app = app;
-        m_sync_route = sync_route;
-        m_config = std::move(config);
-        if (m_sync_client)
-            return;
-
-        // create a new logger - if the logger_factory is updated later, a new
-        // logger will be created at that time.
-        do_make_logger();
-
-        {
-            util::CheckedLockGuard lock(m_file_system_mutex);
+    return std::make_shared<SyncManager>(Private(), std::move(app), std::move(sync_route), config, app_id);
+}
 
-            // Set up the file manager.
-            if (m_file_manager) {
-                // Changing the base path for tests requires calling reset_for_testing()
-                // first, and otherwise isn't supported
-                REALM_ASSERT(m_file_manager->base_path() == m_config.base_file_path);
-            }
-            else {
-                m_file_manager = std::make_unique<SyncFileManager>(m_config.base_file_path, app->config().app_id);
-            }
+SyncManager::SyncManager(Private, std::shared_ptr<app::App> app, std::optional<std::string> sync_route,
+                         const SyncClientConfig& config, const std::string& app_id)
+    : m_config(config)
+    , m_file_manager(std::make_unique<SyncFileManager>(m_config.base_file_path, app_id))
+    , m_sync_route(sync_route)
+    , m_app(app)
+    , m_app_id(app_id)
+{
+    // create the initial logger - if the logger_factory is updated later, a new
+    // logger will be created at that time.
+    do_make_logger();
 
-            // Set up the metadata manager, and perform initial loading/purging work.
-            if (m_metadata_manager || m_config.metadata_mode == MetadataMode::NoMetadata) {
-                return;
-            }
+    if (m_config.metadata_mode == MetadataMode::NoMetadata) {
+        return;
+    }
 
-            bool encrypt = m_config.metadata_mode == MetadataMode::Encryption;
-            m_metadata_manager = std::make_unique<SyncMetadataManager>(m_file_manager->metadata_path(), encrypt,
-                                                                       m_config.custom_encryption_key);
+    bool encrypt = m_config.metadata_mode == MetadataMode::Encryption;
+    m_metadata_manager = std::make_unique<SyncMetadataManager>(m_file_manager->metadata_path(), encrypt,
+                                                               m_config.custom_encryption_key);
 
-            m_metadata_manager->perform_launch_actions(*m_file_manager);
+    m_metadata_manager->perform_launch_actions(*m_file_manager);
 
-            // Load persisted users into the users map.
-            for (auto user : m_metadata_manager->all_logged_in_users()) {
-                users_to_add.push_back(std::make_shared<SyncUser>(SyncUser::Private(), user, this));
-            }
-        }
-    }
-    {
-        util::CheckedLockGuard lock(m_user_mutex);
-        m_users.insert(m_users.end(), users_to_add.begin(), users_to_add.end());
+    // Load persisted users into the users map.
+    for (auto user : m_metadata_manager->all_logged_in_users()) {
+        m_users.push_back(std::make_shared<SyncUser>(SyncUser::Private(), user, this));
     }
 }
 
@@ -107,8 +86,10 @@ bool SyncManager::immediately_run_file_actions(const std::string& realm_path)
     return false;
 }
 
-void SyncManager::reset_for_testing()
+void SyncManager::tear_down_for_testing()
 {
+    close_all_sessions();
+
     {
         util::CheckedLockGuard lock(m_file_system_mutex);
         m_metadata_manager = nullptr;
@@ -146,8 +127,8 @@ void SyncManager::reset_for_testing()
             std::this_thread::sleep_for(std::chrono::milliseconds(10));
             lock.lock();
         }
-        // Callers of `SyncManager::reset_for_testing` should ensure there are no existing sessions
-        // prior to calling `reset_for_testing`.
+        // Callers of `SyncManager::tear_down_for_testing` should ensure there are no existing sessions
+        // prior to calling `tear_down_for_testing`.
         if (!no_sessions) {
             util::CheckedLockGuard lock(m_mutex);
             for (auto session : m_sessions) {
@@ -168,9 +149,6 @@ void SyncManager::reset_for_testing()
         util::CheckedLockGuard lock(m_mutex);
         // Destroy the client now that we have no remaining sessions.
         m_sync_client = nullptr;
-
-        // Reset even more state.
-        m_config = {};
         m_logger_ptr.reset();
         m_sync_route.reset();
     }

src/realm/object-store/sync/sync_manager.hpp

@@ -105,9 +105,7 @@ struct SyncClientConfig {
 };
 
 class SyncManager : public std::enable_shared_from_this<SyncManager> {
-    friend class SyncSession;
-    friend class ::TestSyncManager;
-    friend class ::TestAppSession;
+    struct Private {};
 
 public:
     using MetadataMode = SyncClientConfig::MetadataMode;
@@ -215,11 +213,11 @@ class SyncManager : public std::enable_shared_from_this<SyncManager> {
     std::string recovery_directory_path(util::Optional<std::string> const& custom_dir_name = none) const
         REQUIRES(!m_file_system_mutex);
 
-    // Reset the singleton state for testing purposes. DO NOT CALL OUTSIDE OF TESTING CODE.
-    // Precondition: any synced Realms or `SyncSession`s must be closed or rendered inactive prior to
-    // calling this method.
-    void reset_for_testing() REQUIRES(!m_mutex, !m_file_system_mutex, !m_user_mutex, !m_session_mutex);
-
+    // DO NOT CALL OUTSIDE OF TESTING CODE.
+    // Forcibly close all remaining sync sessions, stop the sync client, and
+    // discard all state. The SyncManager must never be used again after this
+    // function has been called.
+    void tear_down_for_testing() REQUIRES(!m_mutex, !m_file_system_mutex, !m_user_mutex, !m_session_mutex);
 
     // Immediately closes any open sync sessions for this sync manager
     void close_all_sessions() REQUIRES(!m_mutex, !m_session_mutex);
@@ -249,6 +247,11 @@ class SyncManager : public std::enable_shared_from_this<SyncManager> {
         return m_app;
     }
 
+    const std::string& app_id() const
+    {
+        return m_app_id;
+    }
+
     SyncClientConfig config() const REQUIRES(!m_mutex)
     {
         util::CheckedLockGuard lock(m_mutex);
@@ -258,29 +261,23 @@ class SyncManager : public std::enable_shared_from_this<SyncManager> {
     // Return the cached logger
     const std::shared_ptr<util::Logger>& get_logger() const REQUIRES(!m_mutex);
 
-    SyncManager();
-    SyncManager(const SyncManager&) = delete;
-    SyncManager& operator=(const SyncManager&) = delete;
-
     struct OnlyForTesting {
         friend class TestHelper;
 
         static void voluntary_disconnect_all_connections(SyncManager&);
     };
 
-protected:
-    friend class SyncUser;
-    friend class SyncSesson;
-
-    using std::enable_shared_from_this<SyncManager>::shared_from_this;
-    using std::enable_shared_from_this<SyncManager>::weak_from_this;
+    static std::shared_ptr<SyncManager> create(std::shared_ptr<app::App> app, std::optional<std::string> sync_route,
+                                               const SyncClientConfig& config, const std::string& app_id);
+    SyncManager(Private, std::shared_ptr<app::App> app, std::optional<std::string> sync_route,
+                const SyncClientConfig& config, const std::string& app_id);
 
 private:
     friend class app::App;
-
-    void configure(std::shared_ptr<app::App> app, std::optional<std::string> sync_route,
-                   const SyncClientConfig& config)
-        REQUIRES(!m_mutex, !m_file_system_mutex, !m_user_mutex, !m_session_mutex);
+    friend class SyncSession;
+    friend class SyncUser;
+    friend class ::TestSyncManager;
+    friend class ::TestAppSession;
 
     // Stop tracking the session for the given path if it is inactive.
     // No-op if the session is either still active or in the active sessions list
@@ -332,10 +329,11 @@ class SyncManager : public std::enable_shared_from_this<SyncManager> {
     bool do_has_existing_sessions() REQUIRES(m_session_mutex);
 
     // The sync route URL to connect to the server. This can be initially empty, but should not
-    // be cleared once it has been set to a value, except by `reset_for_testing()`.
+    // be cleared once it has been set to a value, except by `tear_down_for_testing()`.
     std::optional<std::string> m_sync_route GUARDED_BY(m_mutex);
 
     std::weak_ptr<app::App> m_app GUARDED_BY(m_mutex);
+    const std::string m_app_id;
 };
 
 } // namespace realm

test/object-store/audit.cpp

@@ -265,7 +265,7 @@ struct TestClock {
 
 TEST_CASE("audit object serialization", "[sync][pbs][audit]") {
     TestSyncManager test_session;
-    SyncTestFile config(test_session.app(), "parent");
+    SyncTestFile config(test_session, "parent");
     config.automatic_change_notifications = false;
     config.schema_version = 1;
     config.schema = Schema{
@@ -1066,7 +1066,7 @@ TEST_CASE("audit management", "[sync][pbs][audit]") {
     TestClock clock;
 
     TestSyncManager test_session;
-    SyncTestFile config(test_session.app(), "parent");
+    SyncTestFile config(test_session, "parent");
     config.automatic_change_notifications = false;
     config.schema_version = 1;
     config.schema = Schema{
@@ -1087,7 +1087,7 @@ TEST_CASE("audit management", "[sync][pbs][audit]") {
     realm->sync_session()->close();
 
     SECTION("config validation") {
-        SyncTestFile config(test_session.app(), "parent2");
+        SyncTestFile config(test_session, "parent2");
         config.automatic_change_notifications = false;
         config.audit_config = std::make_shared<AuditConfig>();
         SECTION("invalid prefix") {
@@ -1495,7 +1495,7 @@ TEST_CASE("audit realm sharding", "[sync][pbs][audit]") {
     // a lot of local unuploaded data.
     TestSyncManager test_session{{}, {.start_immediately = false}};
 
-    SyncTestFile config(test_session.app(), "parent");
+    SyncTestFile config(test_session, "parent");
     config.automatic_change_notifications = false;
     config.schema_version = 1;
     config.schema = Schema{
@@ -1604,7 +1604,7 @@ TEST_CASE("audit realm sharding", "[sync][pbs][audit]") {
         test_session.sync_server().start();
 
         // Open a different Realm with the same user and audit prefix
-        SyncTestFile config(test_session.app(), "other");
+        SyncTestFile config(test_session, "other");
         config.audit_config = std::make_shared<AuditConfig>();
         config.audit_config->logger = audit_logger;
         auto realm = Realm::get_shared_realm(config);
@@ -1631,7 +1631,7 @@ TEST_CASE("audit realm sharding", "[sync][pbs][audit]") {
         test_session.sync_server().start();
 
         // Open the same Realm with a different audit prefix
-        SyncTestFile config(test_session.app(), "parent");
+        SyncTestFile config(test_session, "parent");
         config.audit_config = std::make_shared<AuditConfig>();
         config.audit_config->logger = audit_logger;
         config.audit_config->partition_value_prefix = "other";
@@ -1719,7 +1719,7 @@ TEST_CASE("audit integration tests", "[sync][pbs][audit][baas]") {
         realm->sync_session()->close();
         generate_event(realm);
 
-        auto events = get_audit_events_from_baas(session, *config.sync_config->user, 1);
+        auto events = get_audit_events_from_baas(session, *session.app()->current_user(), 1);
         REQUIRE(events.size() == 1);
         REQUIRE(events[0].activity == "scope");
         REQUIRE(events[0].event == "read");
@@ -1728,18 +1728,20 @@ TEST_CASE("audit integration tests", "[sync][pbs][audit][baas]") {
     }
 
     SECTION("different user from parent Realm") {
+        auto sync_user = session.app()->current_user();
         create_user_and_log_in(session.app());
-        config.audit_config->audit_user = session.app()->current_user();
+        auto audit_user = session.app()->current_user();
+        config.audit_config->audit_user = audit_user;
         auto realm = Realm::get_shared_realm(config);
         // If audit uses the sync user this'll make it fail as that user is logged out
-        config.sync_config->user->log_out();
+        sync_user->log_out();
 
         generate_event(realm);
-        REQUIRE(get_audit_events_from_baas(session, *config.audit_config->audit_user, 1).size() == 1);
+        REQUIRE(get_audit_events_from_baas(session, *audit_user, 1).size() == 1);
     }
 
     SECTION("different app from parent Realm") {
-        auto audit_user = config.sync_config->user;
+        auto audit_user = session.app()->current_user();
 
         // Create an app which does not include AuditEvent in the schema so that
         // things will break if audit tries to use it
@@ -1766,7 +1768,7 @@ TEST_CASE("audit integration tests", "[sync][pbs][audit][baas]") {
         generate_event(realm, 3);
 
         using Metadata = std::map<std::string, std::string>;
-        auto events = get_audit_events_from_baas(session, *config.sync_config->user, 4);
+        auto events = get_audit_events_from_baas(session, *session.app()->current_user(), 4);
         REQUIRE(events[0].metadata.empty());
         REQUIRE(events[1].metadata == Metadata({{"metadata 1", "value 1"}}));
         REQUIRE(events[2].metadata == Metadata({{"metadata 2", "value 2"}}));
@@ -1814,7 +1816,7 @@ TEST_CASE("audit integration tests", "[sync][pbs][audit][baas]") {
     }
 
     SECTION("incoming changesets are discarded") {
-        app::MongoClient remote_client = config.sync_config->user->mongo_client("BackingDB");
+        app::MongoClient remote_client = session.app()->current_user()->mongo_client("BackingDB");
         app::MongoDatabase db = remote_client.db(session.app_session().config.mongo_dbname);
         app::MongoCollection collection = db["AuditEvent"];
 
@@ -1905,7 +1907,7 @@ TEST_CASE("audit integration tests", "[sync][pbs][audit][baas]") {
 
             realm->sync_session()->force_close();
             generate_event(realm, 0);
-            get_audit_events_from_baas(session, *config.audit_config->audit_user, 1);
+            get_audit_events_from_baas(session, *session.app()->current_user(), 1);
         }
     }
 

test/object-store/benchmarks/client_reset.cpp

@@ -192,13 +192,12 @@ TEST_CASE("client reset", "[sync][pbs][benchmark][client reset]") {
     };
 
     TestSyncManager init_sync_manager;
-    SyncTestFile config(init_sync_manager.app(), "default");
-    config.cache = false;
+    SyncTestFile config(init_sync_manager, "default");
     config.automatic_change_notifications = false;
     config.schema = schema;
     ClientResyncMode reset_mode = GENERATE(ClientResyncMode::DiscardLocal, ClientResyncMode::Recover);
     config.sync_config->client_resync_mode = reset_mode;
-    SyncTestFile config2(init_sync_manager.app(), "default");
+    SyncTestFile config2(init_sync_manager, "default");
 
     auto populate_objects = [&](SharedRealm realm, size_t num_objects) {
         TableRef table = get_table(*realm, "object");

test/object-store/benchmarks/object.cpp

@@ -781,7 +781,6 @@ TEST_CASE("Benchmark object notification delivery", "[benchmark][notifications]"
     InMemoryTestFile config;
     config.automatic_change_notifications = false;
     config.schema = Schema{{"object", {{"value", PropertyType::Int}}}};
-    config.cache = false;
     auto r = Realm::get_shared_realm(config);
 
     r->begin_transaction();