Merge pull request #7609 from realm/tg/session-lifecycle

RCORE-2092 Simplify the SessionWrapper lifecycle a bit

Author: tgoyne

CHANGELOG.md

@@ -17,6 +17,8 @@
 
 ### Internals
 * Work around a bug in VC++ that resulted in runtime errors when running the tests in a debug build (#[7741](https://github.com/realm/realm-core/issues/7741)).
+* Refactor `sync::Session` to eliminate the bind() step of session creation ([#7609](https://github.com/realm/realm-core/pull/7609)).
+* Add ScopeExitFail which only calls the handler if exiting the scope via an uncaught exception ([#7609](https://github.com/realm/realm-core/pull/7609)).
 
 ----------------------------------------------
 

src/realm/db.cpp

@@ -1189,10 +1189,9 @@ void DB::open(const std::string& path, const DBOptions& options)
             SlabAlloc::DetachGuard alloc_detach_guard(alloc);
             alloc.note_reader_start(this);
             // must come after the alloc detach guard
-            auto handler = [this, &alloc]() noexcept {
+            auto reader_end_guard = make_scope_exit([this, &alloc]() noexcept {
                 alloc.note_reader_end(this);
-            };
-            auto reader_end_guard = make_scope_exit(handler);
+            });
 
             // Check validity of top array (to give more meaningful errors
             // early)

src/realm/object-store/c_api/types.hpp

@@ -870,7 +870,7 @@ struct realm_sync_socket_callback : realm::c_api::WrapC,
     }
 };
 
-struct CBindingThreadObserver : public realm::BindingCallbackThreadObserver {
+struct CBindingThreadObserver final : public realm::BindingCallbackThreadObserver {
 public:
     CBindingThreadObserver(realm_on_object_store_thread_callback_t on_thread_create,
                            realm_on_object_store_thread_callback_t on_thread_destroy,
@@ -882,14 +882,11 @@ struct CBindingThreadObserver : public realm::BindingCallbackThreadObserver {
         , m_user_data{userdata, [&free_userdata] {
                           if (free_userdata)
                               return free_userdata;
-                          else
-                              return CBindingThreadObserver::m_default_free_userdata;
+                          return CBindingThreadObserver::m_default_free_userdata;
                       }()}
     {
     }
 
-    virtual ~CBindingThreadObserver() = default;
-
     void did_create_thread() override
     {
         if (m_create_callback_func)
@@ -918,25 +915,25 @@ struct CBindingThreadObserver : public realm::BindingCallbackThreadObserver {
     /// {@
     /// For testing: Return the values in this CBindingThreadObserver for comparing if two objects
     /// have the same callback functions and userdata ptr values.
-    inline realm_on_object_store_thread_callback_t test_get_create_callback_func() const noexcept
+    realm_on_object_store_thread_callback_t test_get_create_callback_func() const noexcept
     {
         return m_create_callback_func;
     }
-    inline realm_on_object_store_thread_callback_t test_get_destroy_callback_func() const noexcept
+    realm_on_object_store_thread_callback_t test_get_destroy_callback_func() const noexcept
     {
         return m_destroy_callback_func;
     }
-    inline realm_on_object_store_error_callback_t test_get_error_callback_func() const noexcept
+    realm_on_object_store_error_callback_t test_get_error_callback_func() const noexcept
     {
         return m_error_callback_func;
     }
-    inline realm_userdata_t test_get_userdata_ptr() const noexcept
+    realm_userdata_t test_get_userdata_ptr() const noexcept
     {
         return m_user_data.get();
     }
     /// @}
 
-protected:
+private:
     CBindingThreadObserver() = default;
 
     static constexpr realm_free_userdata_func_t m_default_free_userdata = [](realm_userdata_t) {};

src/realm/object-store/sync/impl/sync_client.hpp

@@ -123,7 +123,7 @@ struct SyncClient {
     std::unique_ptr<sync::Session> make_session(std::shared_ptr<DB> db,
                                                 std::shared_ptr<sync::SubscriptionStore> flx_sub_store,
                                                 std::shared_ptr<sync::MigrationStore> migration_store,
-                                                sync::Session::Config config)
+                                                sync::Session::Config&& config)
     {
         return std::make_unique<sync::Session>(m_client, std::move(db), std::move(flx_sub_store),
                                                std::move(migration_store), std::move(config));

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

@@ -108,10 +108,7 @@ void SyncSession::become_active()
     }
 
     // when entering from the Dying state the session will still be bound
-    if (!m_session) {
-        create_sync_session();
-        m_session->bind();
-    }
+    create_sync_session();
 
     // Register all the pending wait-for-completion blocks. This can
     // potentially add a redundant callback if we're coming from the Dying
@@ -896,6 +893,8 @@ void SyncSession::create_sync_session()
     SyncConfig& sync_config = *m_config.sync_config;
     REALM_ASSERT(sync_config.user);
 
+    std::weak_ptr<SyncSession> weak_self = weak_from_this();
+
     sync::Session::Config session_config;
     session_config.signed_user_token = sync_config.user->access_token();
     session_config.user_id = sync_config.user->user_id();
@@ -912,8 +911,8 @@ void SyncSession::create_sync_session()
 
     if (sync_config.on_sync_client_event_hook) {
         session_config.on_sync_client_event_hook = [hook = sync_config.on_sync_client_event_hook,
-                                                    anchor = weak_from_this()](const SyncClientHookData& data) {
-            return hook(anchor, data);
+                                                    weak_self](const SyncClientHookData& data) {
+            return hook(weak_self, data);
         };
     }
 
@@ -954,46 +953,41 @@ void SyncSession::create_sync_session()
         m_server_requests_action = sync::ProtocolErrorInfo::Action::NoAction;
     }
 
-    m_session = m_client.make_session(m_db, m_flx_subscription_store, m_migration_store, std::move(session_config));
-
-    std::weak_ptr<SyncSession> weak_self = weak_from_this();
-
-    // Set up the wrapped progress handler callback
-    m_session->set_progress_handler([weak_self](uint_fast64_t downloaded, uint_fast64_t downloadable,
-                                                uint_fast64_t uploaded, uint_fast64_t uploadable,
-                                                uint_fast64_t snapshot_version, double download_estimate,
-                                                double upload_estimate, int64_t query_version) {
+    session_config.progress_handler = [weak_self](uint_fast64_t downloaded, uint_fast64_t downloadable,
+                                                  uint_fast64_t uploaded, uint_fast64_t uploadable,
+                                                  uint_fast64_t snapshot_version, double download_estimate,
+                                                  double upload_estimate, int64_t query_version) {
         if (auto self = weak_self.lock()) {
             self->handle_progress_update(downloaded, downloadable, uploaded, uploadable, snapshot_version,
                                          download_estimate, upload_estimate, query_version);
         }
-    });
+    };
 
-    // Sets up the connection state listener. This callback is used for both reporting errors as well as changes to
-    // the connection state.
-    m_session->set_connection_state_change_listener(
-        [weak_self](sync::ConnectionState state, std::optional<sync::SessionErrorInfo> error) {
-            using cs = sync::ConnectionState;
-            ConnectionState new_state = [&] {
-                switch (state) {
-                    case cs::disconnected:
-                        return ConnectionState::Disconnected;
-                    case cs::connecting:
-                        return ConnectionState::Connecting;
-                    case cs::connected:
-                        return ConnectionState::Connected;
-                }
-                REALM_UNREACHABLE();
-            }();
-            // If the OS SyncSession object is destroyed, we ignore any events from the underlying Session as there is
-            // nothing useful we can do with them.
-            if (auto self = weak_self.lock()) {
-                self->update_connection_state(new_state);
-                if (error) {
-                    self->handle_error(std::move(*error));
-                }
+    session_config.connection_state_change_listener = [weak_self](sync::ConnectionState state,
+                                                                  std::optional<sync::SessionErrorInfo> error) {
+        using cs = sync::ConnectionState;
+        ConnectionState new_state = [&] {
+            switch (state) {
+                case cs::disconnected:
+                    return ConnectionState::Disconnected;
+                case cs::connecting:
+                    return ConnectionState::Connecting;
+                case cs::connected:
+                    return ConnectionState::Connected;
             }
-        });
+            REALM_UNREACHABLE();
+        }();
+        // If the OS SyncSession object is destroyed, we ignore any events from the underlying Session as there is
+        // nothing useful we can do with them.
+        if (auto self = weak_self.lock()) {
+            self->update_connection_state(new_state);
+            if (error) {
+                self->handle_error(std::move(*error));
+            }
+        }
+    };
+
+    m_session = m_client.make_session(m_db, m_flx_subscription_store, m_migration_store, std::move(session_config));
 }
 
 void SyncSession::update_connection_state(ConnectionState new_state)

src/realm/sync/binding_callback_thread_observer.hpp

@@ -20,78 +20,31 @@
 #define REALM_OS_BINDING_CALLBACK_THREAD_OBSERVER_HPP
 
 #include <exception>
-#include <functional>
-#include <optional>
-
 
 namespace realm {
-// Interface for bindings interested in registering callbacks before/after the ObjectStore thread runs.
-// This is for example helpful to attach/detach the pthread to the JavaVM in order to be able to perform JNI calls.
+// Interface for observing the lifecycle of the worker thread used by
+// DefaultSocketProvider. This is required to be able to attach/detach the thread
+// to the JVM to be able to perform JNI calls.
 struct BindingCallbackThreadObserver {
-    using NotificationCallback = std::function<void()>;
-    using ErrorCallback = std::function<bool(const std::exception&)>;
-
-    // Create a BindingCallbackThreadObserver that can be used in SyncClientConfig
-    BindingCallbackThreadObserver(std::optional<NotificationCallback>&& did_create_thread,
-                                  std::optional<NotificationCallback>&& will_destroy_thread,
-                                  std::optional<ErrorCallback>&& error_handler)
-        : m_create_thread_callback{std::move(did_create_thread)}
-        , m_destroy_thread_callback{std::move(will_destroy_thread)}
-        , m_handle_error_callback{std::move(error_handler)}
-    {
-    }
-
     virtual ~BindingCallbackThreadObserver() = default;
 
-    ///
-    /// Execution Functions - check for a valid instance and if the function was set
-    ///
-
-    // Call the stored create thread callback function with the id of this thread
-    // Can be overridden to provide a custom implementation
-    virtual void did_create_thread()
-    {
-        if (m_create_thread_callback) {
-            (*m_create_thread_callback)();
-        }
-    }
-
-    // Call the stored destroy thread callback function with the id of this thread
-    // Can be overridden to provide a custom implementation
-    virtual void will_destroy_thread()
-    {
-        if (m_destroy_thread_callback) {
-            (*m_destroy_thread_callback)();
-        }
-    }
-
-    // Call the stored handle error callback function with the id of this thread
-    // IMPORTANT: If a function is supplied that handles the exception, it must
-    // call abort() or cause the application to crash since the SyncClient will
-    // be in a bad state if this occurs and will not be able to shut down properly.
-    // Can be overridden to provide a custom implementation
-    // Return true if the exception was handled by this function, otherwise false
-    virtual bool handle_error(const std::exception& e)
+    // Called on the thread shortly after it is created. This is guaranteed to
+    // be called before any other callbacks to the SDK are made.
+    virtual void did_create_thread() {}
+    // Called on the thread shortly before it is destroyed. No further callbacks
+    // to the SDK on the thread will be made after this is called.
+    virtual void will_destroy_thread() {}
+    // If has_handle_error() returns true, any uncaught exceptions from the
+    // event loop are passed to this. If this returns true, the thread exits
+    // cleanly, while if it returns false the exception is rethrown.
+    virtual bool handle_error(const std::exception&)
     {
-        if (!m_handle_error_callback)
-            return false;
-
-        return (*m_handle_error_callback)(e);
+        return false;
     }
-
-    // Return true if this event loop observer has a handle error callback defined
     virtual bool has_handle_error()
     {
-        return bool(m_handle_error_callback);
+        return false;
     }
-
-protected:
-    // Default constructor
-    BindingCallbackThreadObserver() = default;
-
-    std::optional<NotificationCallback> m_create_thread_callback;
-    std::optional<NotificationCallback> m_destroy_thread_callback;
-    std::optional<ErrorCallback> m_handle_error_callback;
 };
 
 } // namespace realm