From abd0b784fd805c49d381e850c72ad18fd9e580dc Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Mon, 11 Mar 2024 15:15:16 -0400 Subject: [PATCH] Updates from review --- bindgen/spec.yml | 30 +++++++++++-------- src/realm.h | 2 +- src/realm/object-store/c_api/sync.cpp | 10 +++---- .../object-store/sync/impl/sync_client.hpp | 18 +++++------ src/realm/object-store/sync/sync_manager.cpp | 4 --- src/realm/object-store/sync/sync_manager.hpp | 5 +--- src/realm/sync/noinst/client_impl_base.cpp | 9 ------ src/realm/sync/protocol.hpp | 15 +++------- test/object-store/c_api/c_api.cpp | 9 +++--- 9 files changed, 41 insertions(+), 61 deletions(-) diff --git a/bindgen/spec.yml b/bindgen/spec.yml index 80f24df5789..a110b27057b 100644 --- a/bindgen/spec.yml +++ b/bindgen/spec.yml @@ -553,6 +553,21 @@ records: before_notify: 'Nullable>' schema_did_change: 'Nullable>' # new schema available as r.schema + ResumptionDelayInfo: + fields: + max_resumption_delay_interval: + type: std::chrono::milliseconds + default: 3000000 + resumption_delay_interval: + type: std::chrono::milliseconds + default: 1000 + resumption_delay_backoff_multiplier: + type: int + default: 2 + delay_jitter_divisor: + type: int + default: 4 + SyncClientTimeouts: fields: connect_timeout: @@ -570,18 +585,9 @@ records: fast_reconnect_limit: type: uint64_t default: 60000 - resumption_delay_interval: - type: uint64_t - default: 1000 - max_resumption_delay_interval: - type: uint64_t - default: 300000 - resumption_delay_backoff_multiplier: - type: int - default: 2 - resumption_delay_jitter_divisor: - type: uint8_t - default: 4 + reconnect_backoff_info: + type: ResumptionDelayInfo + default: {} SyncClientConfig: fields: diff --git a/src/realm.h b/src/realm.h index 6c26f9cb0a0..f105baca6a5 100644 --- a/src/realm.h +++ b/src/realm.h @@ -3678,7 +3678,7 @@ RLM_API void realm_sync_client_config_set_max_resumption_delay_interval(realm_sy RLM_API void realm_sync_client_config_set_resumption_delay_backoff_multiplier(realm_sync_client_config_t*, int) RLM_API_NOEXCEPT; RLM_API void realm_sync_client_config_set_resumption_delay_jitter_divisor(realm_sync_client_config_t*, - uint8_t) RLM_API_NOEXCEPT; + int) RLM_API_NOEXCEPT; RLM_API void realm_sync_client_config_set_sync_socket(realm_sync_client_config_t*, realm_sync_socket_t*) RLM_API_NOEXCEPT; RLM_API void realm_sync_client_config_set_default_binding_thread_observer( diff --git a/src/realm/object-store/c_api/sync.cpp b/src/realm/object-store/c_api/sync.cpp index 86a22a63ef7..82b7d617d55 100644 --- a/src/realm/object-store/c_api/sync.cpp +++ b/src/realm/object-store/c_api/sync.cpp @@ -215,25 +215,25 @@ RLM_API void realm_sync_client_config_set_fast_reconnect_limit(realm_sync_client RLM_API void realm_sync_client_config_set_resumption_delay_interval(realm_sync_client_config_t* config, uint64_t interval) noexcept { - config->timeouts.resumption_delay_interval = interval; + config->timeouts.reconnect_backoff_info.resumption_delay_interval = std::chrono::milliseconds{interval}; } RLM_API void realm_sync_client_config_set_max_resumption_delay_interval(realm_sync_client_config_t* config, uint64_t interval) noexcept { - config->timeouts.max_resumption_delay_interval = interval; + config->timeouts.reconnect_backoff_info.max_resumption_delay_interval = std::chrono::milliseconds{interval}; } RLM_API void realm_sync_client_config_set_resumption_delay_backoff_multiplier(realm_sync_client_config_t* config, int multiplier) noexcept { - config->timeouts.resumption_delay_backoff_multiplier = multiplier; + config->timeouts.reconnect_backoff_info.resumption_delay_backoff_multiplier = multiplier; } RLM_API void realm_sync_client_config_set_resumption_delay_jitter_divisor(realm_sync_client_config_t* config, - uint8_t divisor) noexcept + int divisor) noexcept { - config->timeouts.resumption_delay_jitter_divisor = divisor; + config->timeouts.reconnect_backoff_info.delay_jitter_divisor = divisor; } /// Register an app local callback handler for bindings interested in registering callbacks before/after diff --git a/src/realm/object-store/sync/impl/sync_client.hpp b/src/realm/object-store/sync/impl/sync_client.hpp index d08e52961bc..39a9265aafc 100644 --- a/src/realm/object-store/sync/impl/sync_client.hpp +++ b/src/realm/object-store/sync/impl/sync_client.hpp @@ -74,17 +74,13 @@ struct SyncClient { c.pong_keepalive_timeout = config.timeouts.pong_keepalive_timeout; if (config.timeouts.fast_reconnect_limit > 1000) c.fast_reconnect_limit = config.timeouts.fast_reconnect_limit; - if (config.timeouts.resumption_delay_interval >= 1000) - c.reconnect_backoff_info.resumption_delay_interval = - std::chrono::milliseconds(config.timeouts.resumption_delay_interval); - if (config.timeouts.max_resumption_delay_interval > 30000) - c.reconnect_backoff_info.max_resumption_delay_interval = - std::chrono::milliseconds(config.timeouts.max_resumption_delay_interval); - if (config.timeouts.resumption_delay_backoff_multiplier >= 1) - c.reconnect_backoff_info.resumption_delay_backoff_multiplier = - config.timeouts.resumption_delay_backoff_multiplier; - c.reconnect_backoff_info.delay_jitter_divisor = config.timeouts.resumption_delay_jitter_divisor; - + c.reconnect_backoff_info = config.timeouts.reconnect_backoff_info; + if (c.reconnect_backoff_info.resumption_delay_interval.count() < 1000) + logger->info("A resumption delay interval less than 1000 (1 second) is not recommended"); + if (c.reconnect_backoff_info.resumption_delay_backoff_multiplier < 1) + throw InvalidArgument("Delay backoff multiplier in reconnect backoff info cannot be less than 1"); + if (c.reconnect_backoff_info.delay_jitter_divisor < 0) + throw InvalidArgument("Delay jitter divisor in reconnect backoff info cannot be less than 0"); return c; }()) , m_logger_ptr(logger) diff --git a/src/realm/object-store/sync/sync_manager.cpp b/src/realm/object-store/sync/sync_manager.cpp index 78aed225d89..4a4b6726c4c 100644 --- a/src/realm/object-store/sync/sync_manager.cpp +++ b/src/realm/object-store/sync/sync_manager.cpp @@ -40,10 +40,6 @@ SyncClientTimeouts::SyncClientTimeouts() , ping_keepalive_period(sync::default_ping_keepalive_period) , pong_keepalive_timeout(sync::default_pong_keepalive_timeout) , fast_reconnect_limit(sync::default_fast_reconnect_limit) - , resumption_delay_interval(sync::default_resumption_delay_interval) - , max_resumption_delay_interval(sync::default_max_resumption_delay_interval) - , resumption_delay_backoff_multiplier(sync::default_resumption_delay_backoff_multiplier) - , resumption_delay_jitter_divisor(sync::default_delay_jitter_divisor) { } diff --git a/src/realm/object-store/sync/sync_manager.hpp b/src/realm/object-store/sync/sync_manager.hpp index 1f905ccb0e6..461bfa78393 100644 --- a/src/realm/object-store/sync/sync_manager.hpp +++ b/src/realm/object-store/sync/sync_manager.hpp @@ -62,10 +62,7 @@ struct SyncClientTimeouts { uint64_t pong_keepalive_timeout; uint64_t fast_reconnect_limit; // Used for requesting location metadata at startup and reconnecting sync connections. - uint64_t resumption_delay_interval; // in milliseconds (min 1000) - uint64_t max_resumption_delay_interval; // in milliseconds (min 30000) - int resumption_delay_backoff_multiplier; // multiplier applied after each attempt (min 1) - uint8_t resumption_delay_jitter_divisor; // fractional divisor to determine jitter (0: disabled) + sync::ResumptionDelayInfo reconnect_backoff_info; }; struct SyncClientConfig { diff --git a/src/realm/sync/noinst/client_impl_base.cpp b/src/realm/sync/noinst/client_impl_base.cpp index ebe92f170d8..1441536dfa4 100644 --- a/src/realm/sync/noinst/client_impl_base.cpp +++ b/src/realm/sync/noinst/client_impl_base.cpp @@ -1296,15 +1296,6 @@ void Connection::receive_error_message(const ProtocolErrorInfo& info, session_id info.message, info.raw_error_code, info.is_fatal, session_ident, info.server_requests_action); // Throws - if (info.resumption_delay_interval) { - logger.debug(" (reconnect backoff info: max_delay=%1ms, initial_delay=%2ms, multiplier=%3, " - "jitter=1/%4)", - info.resumption_delay_interval->max_resumption_delay_interval.count(), - info.resumption_delay_interval->resumption_delay_interval.count(), - info.resumption_delay_interval->resumption_delay_backoff_multiplier, - info.resumption_delay_interval->delay_jitter_divisor); - } - bool known_error_code = bool(get_protocol_error_message(info.raw_error_code)); if (REALM_LIKELY(known_error_code)) { ProtocolError error_code = ProtocolError(info.raw_error_code); diff --git a/src/realm/sync/protocol.hpp b/src/realm/sync/protocol.hpp index e315f345f40..32455019ee0 100644 --- a/src/realm/sync/protocol.hpp +++ b/src/realm/sync/protocol.hpp @@ -240,28 +240,21 @@ struct CompensatingWriteErrorInfo { std::string reason; }; -static constexpr milliseconds_type default_max_resumption_delay_interval = 300000; // 5 minutes -static constexpr milliseconds_type default_resumption_delay_interval = 1000; // 1 second -static constexpr int default_resumption_delay_backoff_multiplier = 2; // Double after each attempt -static constexpr uint8_t default_delay_jitter_divisor = 4; // 25% jitter - struct ResumptionDelayInfo { // This is the maximum delay between trying to resume a session/connection. - std::chrono::milliseconds max_resumption_delay_interval = - std::chrono::milliseconds{default_max_resumption_delay_interval}; + std::chrono::milliseconds max_resumption_delay_interval = std::chrono::minutes{5}; // The initial delay between trying to resume a session/connection. - std::chrono::milliseconds resumption_delay_interval = - std::chrono::milliseconds{default_resumption_delay_interval}; + std::chrono::milliseconds resumption_delay_interval = std::chrono::seconds{1}; // After each failure of the same type, the last delay will be multiplied by this value // until it is greater-than-or-equal to the max_resumption_delay_interval. - int resumption_delay_backoff_multiplier = default_resumption_delay_backoff_multiplier; + int resumption_delay_backoff_multiplier = 2; // When calculating a new delay interval, a random value betwen zero and the result off // dividing the current delay value by the delay_jitter_divisor will be subtracted from the // delay interval. The default is to subtract up to 25% of the current delay interval. // // This is to reduce the likelyhood of a connection storm if the server goes down and // all clients attempt to reconnect at once. - int delay_jitter_divisor = default_delay_jitter_divisor; + int delay_jitter_divisor = 4; }; class IsFatalTag {}; diff --git a/test/object-store/c_api/c_api.cpp b/test/object-store/c_api/c_api.cpp index 974fb003c71..8d921a5a3c9 100644 --- a/test/object-store/c_api/c_api.cpp +++ b/test/object-store/c_api/c_api.cpp @@ -572,13 +572,14 @@ TEST_CASE("C API (non-database)", "[c_api]") { realm_sync_client_config_set_fast_reconnect_limit(test_sync_client_config.get(), 1099); CHECK(test_sync_client_config->timeouts.fast_reconnect_limit == 1099); realm_sync_client_config_set_resumption_delay_interval(test_sync_client_config.get(), 1024); - CHECK(test_sync_client_config->timeouts.resumption_delay_interval == 1024); + CHECK(test_sync_client_config->timeouts.reconnect_backoff_info.resumption_delay_interval.count() == 1024); realm_sync_client_config_set_max_resumption_delay_interval(test_sync_client_config.get(), 600024); - CHECK(test_sync_client_config->timeouts.max_resumption_delay_interval == 600024); + CHECK(test_sync_client_config->timeouts.reconnect_backoff_info.max_resumption_delay_interval.count() == + 600024); realm_sync_client_config_set_resumption_delay_backoff_multiplier(test_sync_client_config.get(), 1010); - CHECK(test_sync_client_config->timeouts.resumption_delay_backoff_multiplier == 1010); + CHECK(test_sync_client_config->timeouts.reconnect_backoff_info.resumption_delay_backoff_multiplier == 1010); realm_sync_client_config_set_resumption_delay_jitter_divisor(test_sync_client_config.get(), 212); - CHECK(test_sync_client_config->timeouts.resumption_delay_jitter_divisor == 212); + CHECK(test_sync_client_config->timeouts.reconnect_backoff_info.delay_jitter_divisor == 212); } SECTION("realm_app_config_t") {