code review

src/realm.h

@@ -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;
 
 /**

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.error_usercode_callback)
+            c_error.user_code_error = ErrorStorage::get_thread_local()->get_and_clear_usercode_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.error_usercode_callback = error.error_user_code_callback;
 
     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 error_user_code_callback = 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 error_usercode_callback = 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.error_user_code_callback = 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& usercode_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& usercode_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.
+            usercode_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.
+            usercode_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& usercode_error);
 
 } // namespace realm::_impl::client_reset
 

test/object-store/c_api/c_api.cpp

@@ -5474,7 +5474,7 @@ TEST_CASE("C API - client reset", "[sync][pbs][c_api][client reset][baas]") {
                     ResetRealmFiles::instance().reset_realm(sync_error.c_original_file_path_key);
                     error_handler_counter.fetch_add(1);
                     baas_client_stop.store(true);
-                    usercode_errors.push_back(sync_error.usercode_error);
+                    usercode_errors.push_back(sync_error.user_code_error);
                 },
                 nullptr, nullptr);