code review

src/realm.h

@@ -235,7 +235,7 @@ typedef struct realm_error {
     const char* message;
     // When error is RLM_ERR_CALLBACK this is an opaque pointer to an SDK-owned error object
     // thrown by user code inside a callback with realm_register_user_code_callback_error(), otherwise null.
-    void* usercode_error;
+    void* user_code_error;
     const char* path;
 } realm_error_t;
 
@@ -3410,7 +3410,7 @@ typedef struct realm_sync_error {
 
     realm_sync_error_compensating_write_info_t* compensating_writes;
     size_t compensating_writes_length;
-    void* usercode_error;
+    void* user_code_error;
 } realm_sync_error_t;
 
 /**
@@ -3881,8 +3881,8 @@ RLM_API void realm_sync_session_handle_error_for_testing(const realm_sync_sessio
 /**
  * In case of exception thrown in user code callbacks, this api will allow the sdk to store the user code exception
  * and retrieve a it later via realm_get_last_error.
- * Most importantly the SDK is responsible to handle the memory pointed by usercode_error.
- * @param usercode_error pointer representing whatever object the SDK treats as exception/error.
+ * Most importantly the SDK is responsible to handle the memory pointed by user_code_error.
+ * @param user_code_error pointer representing whatever object the SDK treats as exception/error.
  */
 RLM_API void realm_register_user_code_callback_error(realm_userdata_t usercode_error) RLM_API_NOEXCEPT;
 

src/realm/exceptions.cpp

@@ -110,14 +110,14 @@ RuntimeError::RuntimeError(Status&& status)
 }
 RuntimeError::~RuntimeError() noexcept = default;
 
-UserCodeCallbackError::UserCodeCallbackError(ErrorCodes::Error code, std::string_view msg, void* usercode_error)
+UserCodeCallbackError::UserCodeCallbackError(ErrorCodes::Error code, std::string_view msg, void* user_code_error)
     : RuntimeError(code, msg)
-    , usercode_error(usercode_error)
+    , user_code_error(user_code_error)
 {
 }
-UserCodeCallbackError::UserCodeCallbackError(Status&& status, void* usercode_error)
+UserCodeCallbackError::UserCodeCallbackError(Status&& status, void* user_code_error)
     : RuntimeError(std::move(status))
-    , usercode_error(usercode_error)
+    , user_code_error(user_code_error)
 {
 }
 UserCodeCallbackError::~UserCodeCallbackError() noexcept = default;

src/realm/exceptions.hpp

@@ -114,12 +114,12 @@ struct RuntimeError : Exception {
 };
 
 struct UserCodeCallbackError : RuntimeError {
-    UserCodeCallbackError(ErrorCodes::Error code, std::string_view msg, void* usercode_error);
-    UserCodeCallbackError(Status&& status, void* usercode_error);
+    UserCodeCallbackError(ErrorCodes::Error code, std::string_view msg, void* user_code_error);
+    UserCodeCallbackError(Status&& status, void* user_code_error);
     ~UserCodeCallbackError() noexcept override;
 
     // opaque ptr in which SDKs can store user code errors and exceptions
-    void* usercode_error = nullptr;
+    void* user_code_error = nullptr;
 };
 
 /// Thrown when creating references that are too large to be contained in our ref_type (size_t)

src/realm/object-store/c_api/config.cpp

@@ -101,7 +101,7 @@ RLM_API void realm_config_set_migration_function(realm_config_t* config, realm_m
             realm_t r2{new_realm};
             realm_schema_t sch{&schema};
             if (!(func)(userdata, &r1, &r2, &sch)) {
-                throw CallbackFailed{ErrorStorage::get_thread_local()->get_and_clear_usercode_error()};
+                throw CallbackFailed{ErrorStorage::get_thread_local()->get_and_clear_user_code_error()};
             }
         };
         config->migration_function = std::move(migration_func);
@@ -123,7 +123,7 @@ RLM_API void realm_config_set_data_initialization_function(realm_config_t* confi
         auto init_func = [=](SharedRealm realm) {
             realm_t r{realm};
             if (!(func)(userdata, &r)) {
-                throw CallbackFailed{ErrorStorage::get_thread_local()->get_and_clear_usercode_error()};
+                throw CallbackFailed{ErrorStorage::get_thread_local()->get_and_clear_user_code_error()};
             }
         };
         config->initialization_function = std::move(init_func);
@@ -144,8 +144,8 @@ RLM_API void realm_config_set_should_compact_on_launch_function(realm_config_t*
     if (func) {
         auto should_func = [=](uint64_t total_bytes, uint64_t used_bytes) -> bool {
             auto result = func(userdata, total_bytes, used_bytes);
-            if (auto usercode_error = ErrorStorage::get_thread_local()->get_and_clear_usercode_error())
-                throw CallbackFailed{usercode_error};
+            if (auto user_code_error = ErrorStorage::get_thread_local()->get_and_clear_user_code_error())
+                throw CallbackFailed{user_code_error};
             return result;
         };
         config->should_compact_on_launch_function = std::move(should_func);

src/realm/object-store/c_api/error.cpp

@@ -15,15 +15,15 @@ namespace realm::c_api {
 ErrorStorage::ErrorStorage(std::exception_ptr ptr) noexcept
     : m_err(none)
     , m_message_buf()
-    , m_usercode_error(nullptr)
+    , m_user_code_error(nullptr)
 {
     assign(std::move(ptr));
 }
 
 ErrorStorage::ErrorStorage(const ErrorStorage& other)
     : m_err(other.m_err)
     , m_message_buf(other.m_message_buf)
-    , m_usercode_error(other.m_usercode_error)
+    , m_user_code_error(other.m_user_code_error)
 {
     if (m_err) {
         m_err->message = m_message_buf.c_str();
@@ -34,7 +34,7 @@ ErrorStorage& ErrorStorage::operator=(const ErrorStorage& other)
 {
     m_err = other.m_err;
     m_message_buf = other.m_message_buf;
-    m_usercode_error = other.m_usercode_error;
+    m_user_code_error = other.m_user_code_error;
     if (m_err) {
         m_err->message = m_message_buf.c_str();
     }
@@ -44,7 +44,7 @@ ErrorStorage& ErrorStorage::operator=(const ErrorStorage& other)
 ErrorStorage::ErrorStorage(ErrorStorage&& other)
     : m_err(std::move(other.m_err))
     , m_message_buf(std::move(other.m_message_buf))
-    , m_usercode_error(std::move(other.m_usercode_error))
+    , m_user_code_error(std::move(other.m_user_code_error))
 {
     if (m_err) {
         m_err->message = m_message_buf.c_str();
@@ -56,7 +56,7 @@ ErrorStorage& ErrorStorage::operator=(ErrorStorage&& other)
 {
     m_err = std::move(other.m_err);
     m_message_buf = std::move(other.m_message_buf);
-    m_usercode_error = std::move(other.m_usercode_error);
+    m_user_code_error = std::move(other.m_user_code_error);
     if (m_err) {
         m_err->message = m_message_buf.c_str();
     }
@@ -83,7 +83,7 @@ void ErrorStorage::assign(std::exception_ptr eptr) noexcept
     }
 
     m_err.emplace();
-    m_err->usercode_error = nullptr;
+    m_err->user_code_error = nullptr;
     m_err->path = nullptr;
     auto populate_error = [&](const std::exception& ex, ErrorCodes::Error error_code) {
         m_err->error = realm_errno_e(error_code);
@@ -108,7 +108,7 @@ void ErrorStorage::assign(std::exception_ptr eptr) noexcept
     catch (const Exception& ex) {
         populate_error(ex, ex.code());
         if (ex.code() == ErrorCodes::CallbackFailed) {
-            m_err->usercode_error = static_cast<const CallbackFailed&>(ex).usercode_error;
+            m_err->user_code_error = static_cast<const CallbackFailed&>(ex).user_code_error;
         }
         if (ErrorCodes::error_categories(ex.code()).test(ErrorCategory::file_access)) {
             auto& file_access_error = static_cast<const FileAccessError&>(ex);
@@ -168,15 +168,15 @@ bool ErrorStorage::clear() noexcept
     return ret;
 }
 
-void ErrorStorage::set_usercode_error(void* usercode_error)
+void ErrorStorage::set_user_code_error(void* user_code_error)
 {
-    m_usercode_error = usercode_error;
+    m_user_code_error = user_code_error;
 }
 
-void* ErrorStorage::get_and_clear_usercode_error()
+void* ErrorStorage::get_and_clear_user_code_error()
 {
-    auto ret = m_usercode_error;
-    m_usercode_error = nullptr;
+    auto ret = m_user_code_error;
+    m_user_code_error = nullptr;
     return ret;
 }
 
@@ -247,7 +247,7 @@ RLM_EXPORT bool realm_wrap_exceptions(void (*func)()) noexcept
     });
 }
 
-RLM_API void realm_register_user_code_callback_error(void* usercode_error) noexcept
+RLM_API void realm_register_user_code_callback_error(void* user_code_error) noexcept
 {
-    realm::c_api::ErrorStorage::get_thread_local()->set_usercode_error(usercode_error);
+    realm::c_api::ErrorStorage::get_thread_local()->set_user_code_error(user_code_error);
 }

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

@@ -46,14 +46,14 @@ class ErrorStorage {
     bool get_as_realm_error_t(realm_error_t* out) const noexcept;
     bool clear() noexcept;
 
-    void set_usercode_error(void* usercode_error);
-    void* get_and_clear_usercode_error();
+    void set_user_code_error(void* usercode_error);
+    void* get_and_clear_user_code_error();
 
 private:
     util::Optional<realm_error_t> m_err;
     std::string m_message_buf;
     std::string m_path_buf;
-    void* m_usercode_error;
+    void* m_user_code_error;
 };
 
 } // namespace realm::c_api

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

@@ -264,7 +264,11 @@ RLM_API void realm_sync_config_set_error_handler(realm_sync_config_t* config, re
         c_error.server_requests_action = static_cast<realm_sync_error_action_e>(error.server_requests_action);
         c_error.c_original_file_path_key = error.c_original_file_path_key;
         c_error.c_recovery_file_path_key = error.c_recovery_file_path_key;
-        c_error.usercode_error = error.usecode_error;
+        c_error.user_code_error = nullptr;
+
+        if (error.user_code_error)
+            c_error.user_code_error = ErrorStorage::get_thread_local()->get_and_clear_user_code_error();
+
 
         std::vector<realm_sync_error_user_info_t> c_user_info;
         c_user_info.reserve(error.user_info.size());
@@ -389,7 +393,7 @@ RLM_API void realm_sync_config_set_before_client_reset_handler(realm_sync_config
     auto cb = [callback, userdata = SharedUserdata(userdata, FreeUserdata(userdata_free))](SharedRealm before_realm) {
         realm_t r1{before_realm};
         if (!callback(userdata.get(), &r1)) {
-            throw CallbackFailed{ErrorStorage::get_thread_local()->get_and_clear_usercode_error()};
+            throw CallbackFailed{};
         }
     };
     config->notify_before_client_reset = std::move(cb);
@@ -405,7 +409,7 @@ RLM_API void realm_sync_config_set_after_client_reset_handler(realm_sync_config_
         realm_t r1{before_realm};
         auto tsr = realm_t::thread_safe_reference(std::move(after_realm));
         if (!callback(userdata.get(), &r1, &tsr, did_recover)) {
-            throw CallbackFailed{ErrorStorage::get_thread_local()->get_and_clear_usercode_error()};
+            throw CallbackFailed{};
         }
     };
     config->notify_after_client_reset = std::move(cb);

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

@@ -738,7 +738,7 @@ void SyncSession::handle_error(sync::SessionErrorInfo error)
     // `action` is used over `shouldClientReset` and `isRecoveryModeDisabled`.
     sync_error.server_requests_action = error.server_requests_action;
     sync_error.is_unrecognized_by_client = unrecognized_by_client;
-    sync_error.usecode_error = error.error_user_code_callback;
+    sync_error.user_code_error = error.user_code_error;
 
     if (delete_file)
         update_error_and_mark_file_for_deletion(sync_error, *delete_file);

src/realm/sync/client_base.hpp

@@ -219,6 +219,7 @@ struct ClientConfig {
     bool fix_up_object_ids = false;
 };
 
+
 /// \brief Information about an error causing a session to be temporarily
 /// disconnected from the server.
 ///
@@ -258,16 +259,14 @@ struct SessionErrorInfo : public ProtocolErrorInfo {
     {
     }
 
-    SessionErrorInfo(Status status, IsFatal is_fatal, void* error_user_code_callback = nullptr)
+    SessionErrorInfo(Status status, IsFatal is_fatal)
         : ProtocolErrorInfo(0, {}, is_fatal)
         , status(std::move(status))
-        , error_user_code_callback(error_user_code_callback) // propagate the error generated in the user code
-                                                             // callback like in other callbacks
     {
     }
 
     Status status;
-    void* error_user_code_callback = nullptr;
+    bool user_code_error = false;
 };
 
 enum class ConnectionState { disconnected, connecting, connected };

src/realm/sync/config.hpp

@@ -73,9 +73,8 @@ struct SyncError {
     // If this error resulted from a compensating write, this vector will contain information about each object
     // that caused a compensating write and why the write was illegal.
     std::vector<sync::CompensatingWriteErrorInfo> compensating_writes_info;
-    // if the error is coming from the client reset callbacks in user code, we record the error in order for the SDKs
-    // to be able to re-throw it and give more info about the error itself to the user.
-    void* usecode_error = nullptr;
+    // track that the user callback provided has failed (useful for fetching the SDK provided exception itself)
+    bool user_code_error = false;
 
     SyncError(Status status, bool is_fatal, std::optional<std::string_view> server_log = std::nullopt,
               std::vector<sync::CompensatingWriteErrorInfo> compensating_writes = {});

src/realm/sync/noinst/client_impl_base.cpp

@@ -2196,7 +2196,7 @@ void Session::send_test_command_message()
     enlist_to_send();
 }
 
-bool Session::client_reset_if_needed()
+bool Session::client_reset_if_needed(bool& user_code_error)
 {
     // Regardless of what happens, once we return from this function we will
     // no longer be in the middle of a client reset
@@ -2216,7 +2216,7 @@ bool Session::client_reset_if_needed()
         logger, *get_db(), *client_reset_config->fresh_copy, client_reset_config->mode,
         std::move(client_reset_config->notify_before_client_reset),
         std::move(client_reset_config->notify_after_client_reset), m_client_file_ident, get_flx_subscription_store(),
-        on_flx_version_complete, client_reset_config->recovery_is_allowed);
+        on_flx_version_complete, client_reset_config->recovery_is_allowed, user_code_error);
     if (!did_reset) {
         return false;
     }
@@ -2294,23 +2294,21 @@ Status Session::receive_ident_message(SaltedFileIdent client_file_ident)
     // and if not, we do it here
     bool did_client_reset = false;
 
-    auto on_session_error = [this](const auto& e, void* error_code = nullptr) {
+    auto on_session_error = [this](const auto& e, bool user_code_error) {
         auto err_msg = util::format("A fatal error occurred during client reset: '%1'", e.what());
-        SessionErrorInfo err_info(Status{ErrorCodes::AutoClientResetFailed, err_msg}, IsFatal{true}, error_code);
+        SessionErrorInfo err_info(Status{ErrorCodes::AutoClientResetFailed, err_msg}, IsFatal{true});
+        err_info.user_code_error = user_code_error;
         logger.error(err_msg.c_str());
         suspend(err_info);
         return Status::OK();
     };
 
+    bool user_code_error = false;
     try {
-        did_client_reset = client_reset_if_needed();
-    }
-    catch (const UserCodeCallbackError& e) {
-        // pass opaque ptr back to SDKs in case of failure in the client reset callbacks
-        return on_session_error(e, e.usercode_error);
+        did_client_reset = client_reset_if_needed(user_code_error);
     }
     catch (const std::exception& e) {
-        return on_session_error(e);
+        return on_session_error(e, user_code_error);
     }
     if (!did_client_reset) {
         get_history().set_client_file_ident(client_file_ident,

src/realm/sync/noinst/client_impl_base.hpp

@@ -992,7 +992,7 @@ class ClientImpl::Session {
     // Processes any pending FLX bootstraps, if one exists. Otherwise this is a noop.
     void process_pending_flx_bootstrap();
 
-    bool client_reset_if_needed();
+    bool client_reset_if_needed(bool& user_code_error);
     void handle_pending_client_reset_acknowledgement();
 
     void update_subscription_version_info();

src/realm/sync/noinst/client_reset_operation.cpp

@@ -54,7 +54,8 @@ bool is_fresh_path(const std::string& path)
 bool perform_client_reset(util::Logger& logger, DB& db, DB& fresh_db, ClientResyncMode mode,
                           CallbackBeforeType notify_before, CallbackAfterType notify_after,
                           sync::SaltedFileIdent new_file_ident, sync::SubscriptionStore* sub_store,
-                          util::FunctionRef<void(int64_t)> on_flx_version, bool recovery_is_allowed)
+                          util::FunctionRef<void(int64_t)> on_flx_version, bool recovery_is_allowed,
+                          bool& user_code_error)
 {
     REALM_ASSERT(mode != ClientResyncMode::Manual);
     logger.debug("Possibly beginning client reset operation: realm_path = %1, mode = %2, recovery_allowed = %3",
@@ -84,8 +85,18 @@ bool perform_client_reset(util::Logger& logger, DB& db, DB& fresh_db, ClientResy
         logger.debug("Local Realm file has never been written to, so skipping client reset.");
         return false;
     }
+    VersionID frozen_before_state_version = latest_version;
+    if (notify_before) {
+        try {
+            frozen_before_state_version = notify_before();
+        }
+        catch (const std::exception& e) {
+            // the user code has failed. Setup a flag.
+            user_code_error = true;
+            throw e;
+        }
+    }
 
-    VersionID frozen_before_state_version = notify_before ? notify_before() : latest_version;
 
     // If m_notify_after is set, pin the previous state to keep it around.
     TransactionRef previous_state;
@@ -98,7 +109,14 @@ bool perform_client_reset(util::Logger& logger, DB& db, DB& fresh_db, ClientResy
                                             on_flx_version); // throws
 
     if (notify_after) {
-        notify_after(previous_state->get_version_of_current_transaction(), did_recover_out);
+        try {
+            notify_after(previous_state->get_version_of_current_transaction(), did_recover_out);
+        }
+        catch (const std::exception& e) {
+            // the user code has failed. Setup a flag.
+            user_code_error = true;
+            throw e;
+        }
     }
 
     return true;

src/realm/sync/noinst/client_reset_operation.hpp

@@ -40,7 +40,8 @@ bool is_fresh_path(const std::string& realm_path);
 bool perform_client_reset(util::Logger& logger, DB& target_db, DB& fresh_db, ClientResyncMode mode,
                           CallbackBeforeType notify_before, CallbackAfterType notify_after,
                           sync::SaltedFileIdent new_file_ident, sync::SubscriptionStore*,
-                          util::FunctionRef<void(int64_t)> on_flx_version, bool recovery_is_allowed);
+                          util::FunctionRef<void(int64_t)> on_flx_version, bool recovery_is_allowed,
+                          bool& user_code_error);
 
 } // namespace realm::_impl::client_reset