From 90c72b66c45dbf7eef04352525f17b42d9d5c9f4 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Fri, 26 Apr 2024 12:19:50 -0400 Subject: [PATCH] RCORE-2060 Enabling 'cancel_waits_on_nonfatal_error' does not cancel waits during location update while offline (#7528) * added tests using cancel_waits_on_nonfatal_error and fix operation during location update * Updated changelog and updated comments/debug statements * fix swift build and test and tsan errors * Added test to replicate swift autoopen feature * Fixed swift build issue * removed an unused function * Updates from review --- CHANGELOG.md | 1 + src/realm/object-store/sync/app.cpp | 8 +- src/realm/object-store/sync/sync_session.cpp | 6 + test/object-store/realm.cpp | 254 +++++++++++++++++- .../object-store/util/sync/baas_admin_api.cpp | 5 +- test/object-store/util/test_file.cpp | 61 ++++- test/object-store/util/test_file.hpp | 6 + 7 files changed, 328 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0f0a034c98..7b9e1a293aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ * Non-streaming download sync progress notification is fixed for flexible sync Realms where before it was sometimes stopping to emit values right after the registration of the callback (PR [#7561](https://github.com/realm/realm-core/issues/7561)). * Schema initialization could hit an assertion failure if the sync client applied a downloaded changeset while the Realm file was in the process of being opened ([#7041](https://github.com/realm/realm-core/issues/7041), since v11.4.0). * Queries using query paths on Mixed values returns inconsistent results ([#7587](https://github.com/realm/realm-core/issues/7587), since v14.0.0) +* Enabling 'cancel_waits_on_nonfatal_error' does not cancel waits during location update while offline ([#7527](https://github.com/realm/realm-core/issues/7527), since v13.26.0) ### Breaking changes * The following things have been renamed or moved as part of moving all of the App Services functionality to the app namespace: diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index 884d80ec7e3..73f0d07cc4f 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -842,6 +842,7 @@ void App::log_out(const std::shared_ptr& user, SyncUser::State new_state, return; } + log_debug("App: log_out(%1)", user->user_id()); auto request = make_request(HttpMethod::del, url_for_path("/auth/session"), user, RequestTokenType::RefreshToken, ""); @@ -1284,14 +1285,17 @@ void App::refresh_access_token(const std::shared_ptr& user, bool update_lo return; } - log_debug("App: refresh_access_token: email: %1 %2", user->user_profile().email(), - update_location ? "(updating location)" : ""); + log_debug("App: refresh_access_token: user_id: %1%2", user->user_id(), + update_location ? " (updating location)" : ""); // If update_location is set, force the location info to be updated before sending the request do_request( make_request(HttpMethod::post, url_for_path("/auth/session"), user, RequestTokenType::RefreshToken, ""), [completion = std::move(completion), self = shared_from_this(), user](auto&&, const Response& response) { if (auto error = AppUtils::check_for_errors(response)) { + self->log_error("App: refresh_access_token: %1 -> %2 ERROR: %3", user->user_id(), + response.http_status_code, error->what()); + return completion(std::move(error)); } diff --git a/src/realm/object-store/sync/sync_session.cpp b/src/realm/object-store/sync/sync_session.cpp index 81472662196..697a1cad122 100644 --- a/src/realm/object-store/sync/sync_session.cpp +++ b/src/realm/object-store/sync/sync_session.cpp @@ -340,9 +340,15 @@ SyncSession::handle_refresh(const std::shared_ptr& session, bool re // internal backoff timer which will happen automatically so nothing needs to // happen here. util::CheckedUniqueLock lock(session->m_state_mutex); + // If updating access token while opening realm, just become active at this point + // and try to use the current access token. if (session->m_state == State::WaitingForAccessToken) { session->become_active(); } + // If `cancel_waits_on_nonfatal_error` is true, then cancel the waiters and pass along the error + else if (session->config(&SyncConfig::cancel_waits_on_nonfatal_error)) { + session->cancel_pending_waits(std::move(lock), error->to_status()); // unlocks the mutex + } } } else { diff --git a/test/object-store/realm.cpp b/test/object-store/realm.cpp index cc549be7af9..0ee72b3759d 100644 --- a/test/object-store/realm.cpp +++ b/test/object-store/realm.cpp @@ -45,6 +45,11 @@ #if REALM_ENABLE_SYNC #include +#include +#include +#ifdef REALM_ENABLE_AUTH_TESTS +#include +#endif // REALM_ENABLE_AUTH_TESTS #include #include @@ -52,7 +57,7 @@ #include #include -#endif +#endif // REALM_ENABLE_SYNC #include #include @@ -62,7 +67,7 @@ #include #if REALM_HAVE_UV #include -#endif +#endif // REALM_HAVE_UV namespace realm { class TestHelper { @@ -1215,6 +1220,148 @@ TEST_CASE("Get Realm using Async Open", "[sync][pbs][async open]") { REQUIRE(got_error); } +#if REALM_APP_SERVICES + + SECTION("waiters are cancelled if cancel_waits_on_nonfatal_error") { + auto logger = util::Logger::get_default_logger(); + auto transport = std::make_shared>(); + auto socket_provider = std::make_shared(logger, "some user agent"); + enum TestMode { expired_at_start, expired_by_websocket, websocket_fails }; + enum FailureMode { location_fails, token_fails, token_not_authorized }; + auto txt_test_mode = [](TestMode mode) { + switch (mode) { + case TestMode::expired_at_start: + return "access token expired when realm is opened"; + case TestMode::expired_by_websocket: + return "access token expired by websocket"; + case TestMode::websocket_fails: + return "websocket returns connection failed"; + default: + return "Unknown TestMode"; + } + }; + auto txt_failure_mode = [](FailureMode mode) { + switch (mode) { + case FailureMode::location_fails: + return "location update fails"; + case FailureMode::token_fails: + return "access token refresh fails"; + case FailureMode::token_not_authorized: + return "websocket connect not authorized"; + default: + return "Unknown FailureMode"; + } + }; + + app::AppConfig app_config; + set_app_config_defaults(app_config, transport); + app_config.sync_client_config.socket_provider = socket_provider; + app_config.base_file_path = util::make_temp_dir(); + app_config.metadata_mode = app::AppConfig::MetadataMode::NoEncryption; + + auto the_app = app::App::get_app(app::App::CacheMode::Disabled, app_config); + create_user_and_log_in(the_app); + auto user = the_app->current_user(); + // User should be logged in at this point + REQUIRE(user->is_logged_in()); + + bool not_authorized = false; + bool token_refresh_called = false; + bool location_refresh_called = false; + + TestMode test_mode = GENERATE(expired_at_start, expired_by_websocket, websocket_fails); + FailureMode failure = GENERATE(location_fails, token_fails, token_not_authorized); + + DYNAMIC_SECTION(txt_test_mode(test_mode) << " - " << txt_failure_mode(failure)) { + logger->info("TEST: %1 - %2", txt_test_mode(test_mode), txt_failure_mode(failure)); + if (test_mode == TestMode::expired_at_start) { + // invalidate the user's cached access token + auto app_user = the_app->current_user(); + app_user->update_data_for_testing([&](app::UserData& data) { + data.access_token = RealmJWT(expired_token); + }); + } + else if (test_mode == TestMode::expired_by_websocket) { + // tell websocket to return not authorized to refresh access token + not_authorized = true; + } + } + + the_app.reset(); + + auto err_handler = [](std::shared_ptr session, SyncError error) { + auto logger = util::Logger::get_default_logger(); + logger->debug("The sync error handler caught an error: '%1' for '%2'", error.status, session->path()); + // Ignore connection failed non-fatal errors and check for access token refresh unauthorized fatal errors + if (error.status.code() == ErrorCodes::SyncConnectFailed) { + REQUIRE_FALSE(error.is_fatal); + return; + } + // If it's not SyncConnectFailed, then it should be AuthError + REQUIRE(error.status.code() == ErrorCodes::AuthError); + REQUIRE(error.is_fatal); + }; + + transport->request_hook = [&](const app::Request& req) -> std::optional { + static constexpr int CURLE_OPERATION_TIMEDOUT = 28; + std::lock_guard lock(mutex); + if (req.url.find("/auth/session") != std::string::npos) { + token_refresh_called = true; + if (failure == FailureMode::token_not_authorized) { + return app::Response{403, 0, {}, "403 not authorized"}; + } + if (failure == FailureMode::token_fails) { + return app::Response{0, CURLE_OPERATION_TIMEDOUT, {}, "Operation timed out"}; + } + } + else if (req.url.find("/location") != std::string::npos) { + location_refresh_called = true; + if (failure == FailureMode::location_fails) { + // Fake "offline/request timed out" custom error response + return app::Response{0, CURLE_OPERATION_TIMEDOUT, {}, "Operation timed out"}; + } + } + return std::nullopt; + }; + + socket_provider->websocket_connect_func = [&]() -> std::optional { + if (not_authorized) { + not_authorized = false; // one shot + return SocketProviderError(sync::websocket::WebSocketError::websocket_unauthorized, + "403 not authorized"); + } + return SocketProviderError(sync::websocket::WebSocketError::websocket_connection_failed, + "Operation timed out"); + }; + + the_app = app::App::get_app(app::App::CacheMode::Disabled, app_config); + SyncTestFile config(the_app->current_user(), "realm"); + config.sync_config->cancel_waits_on_nonfatal_error = true; + config.sync_config->error_handler = err_handler; + + // User should be logged in at this point + REQUIRE(config.sync_config->user->is_logged_in()); + + auto task = Realm::get_synchronized_realm(config); + auto pf = util::make_promise_future(); + task->start([&pf](auto ref, auto error) mutable { + REQUIRE(!ref); + REQUIRE(error); + pf.promise.emplace_value(error); + }); + + auto result = pf.future.get_no_throw(); + REQUIRE(result.is_ok()); + REQUIRE(result.get_value()); + std::lock_guard lock(mutex); + REQUIRE(location_refresh_called); + if (failure != FailureMode::location_fails) { + REQUIRE(token_refresh_called); + } + } + +#endif // REALM_APP_SERVICES + SECTION("read-only mode sets the schema version") { { SharedRealm realm = Realm::get_shared_realm(config); @@ -1348,6 +1495,109 @@ TEST_CASE("Get Realm using Async Open", "[sync][pbs][async open]") { } } +#if REALM_ENABLE_AUTH_TESTS + +TEST_CASE("Syhcnronized realm: AutoOpen", "[sync][baas][pbs][async open]") { + const auto partition = random_string(100); + auto schema = get_default_schema(); + enum TestMode { expired_at_start, expired_by_websocket, websocket_fails }; + enum FailureMode { location_fails, token_fails, token_not_authorized }; + + auto logger = util::Logger::get_default_logger(); + auto transport = std::make_shared>(); + auto socket_provider = std::make_shared(logger, "some user agent"); + std::mutex mutex; + + // Create the app session and get the logged in user identity + auto server_app_config = minimal_app_config("autoopen-realm", schema); + TestAppSession session(create_app(server_app_config), transport, DeleteApp{true}, realm::ReconnectMode::normal, + socket_provider); + auto user = session.app()->current_user(); + std::string identity = user->user_id(); + REQUIRE(user->is_logged_in()); + REQUIRE(!identity.empty()); + // Reopen the App instance and retrieve the cached user + session.reopen(false); + user = session.app()->get_existing_logged_in_user(identity); + + SyncTestFile config(user, partition, schema); + config.sync_config->cancel_waits_on_nonfatal_error = true; + config.sync_config->error_handler = [&logger](std::shared_ptr session, SyncError error) { + logger->debug("The sync error handler caught an error: '%1' for '%2'", error.status, session->path()); + // Ignore connection failed non-fatal errors and check for access token refresh unauthorized fatal errors + if (error.status.code() == ErrorCodes::SyncConnectFailed) { + REQUIRE_FALSE(error.is_fatal); + return; + } + // If it's not SyncConnectFailed, then it should be AuthError + REQUIRE(error.status.code() == ErrorCodes::AuthError); + REQUIRE(error.is_fatal); + }; + + bool not_authorized = false; + bool token_refresh_called = false; + bool location_refresh_called = false; + + FailureMode failure = FailureMode::location_fails; + + transport->request_hook = [&](const app::Request& req) -> std::optional { + static constexpr int CURLE_OPERATION_TIMEDOUT = 28; + std::lock_guard lock(mutex); + if (req.url.find("/auth/session") != std::string::npos) { + token_refresh_called = true; + if (failure == FailureMode::token_not_authorized) { + return app::Response{403, 0, {}, "403 not authorized"}; + } + if (failure == FailureMode::token_fails) { + return app::Response{0, CURLE_OPERATION_TIMEDOUT, {}, "Operation timed out"}; + } + } + else if (req.url.find("/location") != std::string::npos) { + location_refresh_called = true; + if (failure == FailureMode::location_fails) { + // Fake "offline/request timed out" custom error response + return app::Response{0, CURLE_OPERATION_TIMEDOUT, {}, "Operation timed out"}; + } + } + return std::nullopt; + }; + + socket_provider->websocket_connect_func = [&]() -> std::optional { + if (not_authorized) { + not_authorized = false; // one shot + return SocketProviderError(sync::websocket::WebSocketError::websocket_unauthorized, "403 not authorized"); + } + return SocketProviderError(sync::websocket::WebSocketError::websocket_connection_failed, + "Operation timed out"); + }; + + auto task = Realm::get_synchronized_realm(config); + auto pf = util::make_promise_future(); + task->start([&pf](auto ref, auto error) mutable { + REQUIRE(!ref); + REQUIRE(error); + pf.promise.emplace_value(error); + }); + + auto result = pf.future.get_no_throw(); + REQUIRE(result.is_ok()); + REQUIRE(result.get_value()); + { + std::lock_guard lock(mutex); + REQUIRE(location_refresh_called); + if (failure != FailureMode::location_fails) { + REQUIRE(token_refresh_called); + } + } + + transport->request_hook = nullptr; + socket_provider->websocket_connect_func = nullptr; + auto r = Realm::get_shared_realm(config); + wait_for_download(*r); +} + +#endif // REALM_ENABLE_AUTH_TESTS + TEST_CASE("SharedRealm: convert", "[sync][pbs][convert]") { TestSyncManager tsm; ObjectSchema object_schema = {"object", diff --git a/test/object-store/util/sync/baas_admin_api.cpp b/test/object-store/util/sync/baas_admin_api.cpp index 3dba7bf0a79..4467993079f 100644 --- a/test/object-store/util/sync/baas_admin_api.cpp +++ b/test/object-store/util/sync/baas_admin_api.cpp @@ -346,8 +346,11 @@ app::Response do_http_request(const app::Request& request) auto logger = util::Logger::get_default_logger(); if (response_code != CURLE_OK) { + std::string message = curl_easy_strerror(response_code); logger->error("curl_easy_perform() failed when sending request to '%1' with body '%2': %3", request.url, - request.body, curl_easy_strerror(response_code)); + request.body, message); + // Return a failing response with the CURL error as the custom code + return {0, response_code, {}, message}; } if (logger->would_log(util::Logger::Level::trace)) { std::string coid = [&] { diff --git a/test/object-store/util/test_file.cpp b/test/object-store/util/test_file.cpp index 6f6409417f2..50e58d8af5c 100644 --- a/test/object-store/util/test_file.cpp +++ b/test/object-store/util/test_file.cpp @@ -381,6 +381,52 @@ TestAppSession::~TestAppSession() } } +void TestAppSession::close(bool tear_down) +{ + try { + if (tear_down) { + // If tearing down, make sure there's an app to work with + if (!m_app) { + reopen(false); + } + REALM_ASSERT(m_app); + // Clean up the app data + m_app->sync_manager()->tear_down_for_testing(); + } + else if (m_app) { + // Otherwise, make sure all the session are closed + m_app->sync_manager()->close_all_sessions(); + } + m_app.reset(); + + // If tearing down, clean up the test file directory + if (tear_down && !m_base_file_path.empty() && util::File::exists(m_base_file_path)) { + util::try_remove_dir_recursive(m_base_file_path); + m_base_file_path.clear(); + } + } + catch (const std::exception& ex) { + std::cerr << "Error tearing down TestAppSession: " << ex.what() << "\n"; + } + // Ensure all cached apps are cleared + app::App::clear_cached_apps(); +} + +void TestAppSession::reopen(bool log_in) +{ + REALM_ASSERT(!m_base_file_path.empty()); + if (m_app) { + close(false); + } + m_app = app::App::get_app(app::App::CacheMode::Disabled, app_config); + + // initialize sync client + m_app->sync_manager()->get_sync_client(); + if (log_in) { + log_in_user(m_app, user_creds); + } +} + std::vector TestAppSession::get_documents(app::User& user, const std::string& object_type, size_t expected_count) const { @@ -478,8 +524,7 @@ OfflineAppSession::OfflineAppSession(OfflineAppSession::Config config) , m_delete_storage(config.delete_storage) { REALM_ASSERT(m_transport); - app::AppConfig app_config; - set_app_config_defaults(app_config, m_transport); + set_app_config_defaults(m_app_config, m_transport); if (config.storage_path) { m_base_file_path = *config.storage_path; @@ -489,16 +534,16 @@ OfflineAppSession::OfflineAppSession(OfflineAppSession::Config config) m_base_file_path = util::make_temp_dir(); } - app_config.base_file_path = m_base_file_path; - app_config.metadata_mode = config.metadata_mode; + m_app_config.base_file_path = m_base_file_path; + m_app_config.metadata_mode = config.metadata_mode; if (config.base_url) { - app_config.base_url = *config.base_url; + m_app_config.base_url = *config.base_url; } if (config.app_id) { - app_config.app_id = *config.app_id; + m_app_config.app_id = *config.app_id; } - app_config.sync_client_config.socket_provider = config.socket_provider; - m_app = app::App::get_app(app::App::CacheMode::Disabled, app_config); + m_app_config.sync_client_config.socket_provider = config.socket_provider; + m_app = app::App::get_app(app::App::CacheMode::Disabled, m_app_config); } OfflineAppSession::~OfflineAppSession() diff --git a/test/object-store/util/test_file.hpp b/test/object-store/util/test_file.hpp index 00b9afcdb74..73853a0fc64 100644 --- a/test/object-store/util/test_file.hpp +++ b/test/object-store/util/test_file.hpp @@ -313,6 +313,7 @@ class OfflineAppSession { } private: + realm::app::AppConfig m_app_config; std::shared_ptr m_app; std::string m_base_file_path; std::shared_ptr m_transport; @@ -347,6 +348,11 @@ class TestAppSession { return m_app->sync_manager(); } + // Close the app instance (or tear down the TestAppSession) + void close(bool tear_down = false); + // Re-open the app instance using app_config + void reopen(bool log_in = false); + realm::app::AppConfig app_config; std::vector get_documents(realm::app::User& user, const std::string& object_type,