RCORE-2060 Enabling 'cancel_waits_on_nonfatal_error' does not cancel waits during location update while offline #7528

michael-wb · 2024-03-27T02:30:08Z

What, How & Why?

After the location update fixes in #7469, the operation of @AutoOpen in the Swift SDK was still an issue if the Realm needed to update the location and was currently offline. In order for @AutoOpen to work, the sync session needs to cancel any waiters with an error if the websocket connection to the server is not successful, which is done by setting the SyncConfig::cancel_waits_on_nonfatal_error flag to true.

This PR fixes this operation and adds tests for the SyncConfig::cancel_waits_on_nonfatal_error setting since these did not currently exist.

Fixes #7527

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
~~[ ] C-API, if public C++ API changed~~
~~[ ] bindgen/spec.yml, if public C++ API changed~~

…ring location update

…cel-waits-on-nonfatal-error

test/object-store/util/sync/sync_test_utils.hpp

coveralls-official · 2024-03-27T03:07:16Z

Pull Request Test Coverage Report for Build michael.wilkersonbarker_1057

Details

212 of 257 (82.49%) changed or added relevant lines in 6 files are covered.
71 unchanged lines in 15 files lost coverage.
Overall coverage remained the same at 90.751%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/realm/exceptions.cpp	1	3	33.33%
test/object-store/util/test_file.cpp	27	41	65.85%
test/object-store/realm.cpp	173	202	85.64%

Files with Coverage Reduction	New Missed Lines	%
src/realm/index_string.hpp	1	93.48%
test/test_query2.cpp	1	98.73%
src/realm/array_blobs_big.cpp	2	98.58%
src/realm/query_expression.cpp	2	86.62%
src/realm/util/to_string.cpp	2	97.62%
src/realm/sync/noinst/protocol_codec.hpp	3	73.5%
src/realm/table.cpp	3	90.08%
test/fuzz_group.cpp	3	51.67%
test/test_thread.cpp	3	64.84%
src/realm/util/assert.hpp	4	87.1%

Totals
Change from base Build 2262:	0.0%
Covered Lines:	212445
Relevant Lines:	234096

💛 - Coveralls

test/object-store/realm.cpp

…cel-waits-on-nonfatal-error

michael-wb · 2024-03-27T12:47:03Z

Hi @tgoyne or @dianaafanador3 - would either of you mind trying these changes out with the Swift @AutoOpen class to make sure it fixes the current issues (e.g. gets stuck in .connecting state while offline)?
Thank you! 🙂

jbreams · 2024-03-27T13:07:29Z

test/object-store/realm.cpp

+                location_refresh_called = true;
+                if (mode == location_fails) {
+                    // Fake "offline/request timed out" custom error response
+                    return app::Response{0, 28, {}, "Operation timed out"};


Does the 28 have some special significance?

Not really - it just corresponded with CURLE_OPERATION_TIMEDOUT (28), since there really isn't a default error returned when the operation times out and is more transport implementation specific.
https://curl.se/libcurl/c/libcurl-errors.html#:~:text=CURLE_OPERATION_TIMEDOUT

I added a define for the value to try to explain why the value is being used.

test/object-store/sync/app.cpp

danieltabacaru · 2024-03-27T13:45:23Z

@michael-wb Is is a guarantee that AutoOpen sets SyncConfig::cancel_waits_on_nonfatal_error to true?

michael-wb · 2024-03-27T13:51:25Z

@michael-wb Is is a guarantee that AutoOpen sets SyncConfig::cancel_waits_on_nonfatal_error to true?

Yes - here is where it is being set: https://github.com/realm/realm-swift/blob/master/RealmSwift/SwiftUI.swift#L1533-L1540

…cel-waits-on-nonfatal-error

test/object-store/realm.cpp

danieltabacaru · 2024-03-27T14:39:34Z

test/object-store/realm.cpp

+
+        SyncTestFile config(oas, "realm");
+        auto user = config.sync_config->user;
+        config.sync_config->cancel_waits_on_nonfatal_error = true;


should we add a test (maybe already have one?) when this is false?

I don't think it's needed...
When this is false, the session will continue with retrying to connect to the server, so the test case expectations will be different. In addition, all the other aynsc open tests (as well as many others), validate the sync client operation with this flag set to false (the default).

yeah, what I meant is to test that behavior and expectations too (in a separate test not as a section). But I think you added a test in the previous PR.

Yes, the tests added in the previous PR should test the normal cases; although I could add an additional test specifically for async open if you think that is necessary.

test/object-store/realm.cpp

danieltabacaru

🚢 🇮🇹

danieltabacaru · 2024-03-27T15:52:26Z

test/object-store/realm.cpp

+        TestMode mode = GENERATE(location_fails, token_fails, token_not_authorized);
+
+        SECTION(util::format("access token expired when realm is opened - mode: %1", txt_test_mode(mode))) {
+            logger->trace(">>> access token expired when realm is opened - mode: %1", txt_test_mode(mode));


nit: if this is mainly for CI, I don't think we run the tests with trace logs (could use debug)

These were originally in for debugging the tests, but I ended up removing them from the test.

danieltabacaru · 2024-03-27T16:03:09Z

test/object-store/realm.cpp

+                REQUIRE_FALSE(error.is_fatal);
+                return;
+            }
+            if (error.status.code() == ErrorCodes::AuthError) {


looking at the websocket replies, these are the only two errors allowed. you could just:

REQUIRE(error.status.code() == ErrorCodes::AuthError); REQUIRE(error.is_fatal);

tgoyne · 2024-03-27T16:26:12Z

test/object-store/realm.cpp

+
+        TestMode mode = GENERATE(location_fails, token_fails, token_not_authorized);
+
+        SECTION(util::format("access token expired when realm is opened - mode: %1", txt_test_mode(mode))) {


Catch has DYNAMIC_SECTION for this: DYNAMIC_SECTION("access token expired when realm is opened - mode: " << txt_test_mode(mode)).

This pattern of doing configuration in the SECTION and then the shared test afterwards has the problem that it results in catch not being able to tell us which section failed, and relying on logging for that is awkward. I think the intended way to do this with Catch would be using GENERATE on a second enum and switching on that instead of using sections. CAPTURE(mode) can be used to make it print the generated mode on failure.

good to know 👍

Thanks @tgoyne - I updated the tests to use the DYNAMIC_SECTION so the individual sections would show up in the test results.

dianaafanador3 · 2024-03-28T16:11:42Z

Hi @tgoyne or @dianaafanador3 - would either of you mind trying these changes out with the Swift @AutoOpen class to make sure it fixes the current issues (e.g. gets stuck in .connecting state while offline)? Thank you! 🙂

So I tested this with the template and we are no longer getting the connection when using AutoOpen, but I do get a lot of connection errors when recovering the connection. The message of the error is not very detailed.
Error Domain=io.realm.sync Code=4 "error" UserInfo={NSLocalizedDescription=error}
The error message is coming from core

michael-wb · 2024-03-28T16:23:37Z

Thank you @dianaafanador3 - do you have a log file (maybe share via slack) and how is the connection recovered? using SyncSession::handle_reconnect()?

…cel-waits-on-nonfatal-error

dianaafanador3

Errors are not longer showing during the manual testing, and offline data is synced when the connection is recovered.

…cel-waits-on-nonfatal-error

michael-wb · 2024-04-26T13:49:50Z

@danieltabacaru / @jbreams - This has been updated to the latest master. Can I please get a quick sanity check before I merge for today's release? Thanks

danieltabacaru · 2024-04-26T14:47:22Z

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

@@ -1284,14 +1285,17 @@ void App::refresh_access_token(const std::shared_ptr<User>& 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(),


nit: user_id

danieltabacaru · 2024-04-26T15:05:20Z

test/object-store/util/test_file.hpp

+    // 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);


why is this needed? 🤔 Can't you create a new TestAppSession if needed?

This is so I can shut down and restart the app and retain the same metadata realm and logged in user in order to simulate the "restart client app with cached user" issue where the location info hasn't been queried yet when the sync session is started.
If I create a new TestAppSession, it will create a new directory and a new logged in user (which will request the location info in the process).

got it. it may be worth being able to create a TestAppSession that retains the same metadata realm (i.e, by passing the directory and check if the realm exists, otherwise start from fresh).

I would prefer to have TestAppSession remain a strictly scoped type and have a way to reuse a metadata directory between instances, but it's also not a big deal and not worth spending much time on.

I can update this in a future PR.

tgoyne · 2024-04-26T15:39:05Z

src/realm/exceptions.cpp

 {
    try {
+        if (exc_ptr) {


This doesn't appear to be used anywhere.

I was previously using this to get the exception thrown in the Async open start callback, but not anymore. Removed this unused code.

tgoyne · 2024-04-26T15:41:20Z

test/object-store/realm.cpp

@@ -1348,6 +1495,111 @@ TEST_CASE("Get Realm using Async Open", "[sync][pbs][async open]") {
    }
 }

+#if REALM_ENABLE_AUTH_TESTS
+#if REALM_APP_SERVICES


REALM_ENABLE_AUTH_TESTS implies REALM_APP_SERVICES so they don't both need to be checked.

* Prepare release * [bindgen] expose both SyncUser and app::User (#7634) * 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 --------- Co-authored-by: Kenneth Geisshirt <[email protected]> Co-authored-by: Michael Wilkerson-Barker <[email protected]>

…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

Michael Wilkerson-Barker added 2 commits March 26, 2024 21:58

added tests using cancel_waits_on_nonfatal_error and fix operation du…

2990263

…ring location update

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-can…

b5d2dc5

…cel-waits-on-nonfatal-error

cla-bot bot added the cla: yes label Mar 27, 2024

github-actions bot assigned michael-wb Mar 27, 2024

tgoyne reviewed Mar 27, 2024

View reviewed changes

test/object-store/util/sync/sync_test_utils.hpp Outdated Show resolved Hide resolved

Michael Wilkerson-Barker added 4 commits March 26, 2024 23:08

Updated changelog and updated comments/debug statements

c060daf

fix swift build and test and tsan errors

91a54f1

Update from review

5650107

Fixed TSAN failures

ea76f65

tgoyne reviewed Mar 27, 2024

View reviewed changes

test/object-store/realm.cpp Outdated Show resolved Hide resolved

Michael Wilkerson-Barker added 3 commits March 27, 2024 08:23

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-can…

0776dd8

…cel-waits-on-nonfatal-error

Updates from review

7298787

Fixed lint failure

5ac829b

michael-wb requested review from jbreams and danieltabacaru March 27, 2024 12:43

jbreams reviewed Mar 27, 2024

View reviewed changes

danieltabacaru reviewed Mar 27, 2024

View reviewed changes

test/object-store/sync/app.cpp Outdated Show resolved Hide resolved

Michael Wilkerson-Barker added 2 commits March 27, 2024 10:05

Updates from review

b3bb528

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-can…

3bad95e

…cel-waits-on-nonfatal-error

danieltabacaru reviewed Mar 27, 2024

View reviewed changes

test/object-store/realm.cpp Outdated Show resolved Hide resolved

danieltabacaru reviewed Mar 27, 2024

View reviewed changes

test/object-store/realm.cpp Outdated Show resolved Hide resolved

danieltabacaru reviewed Mar 27, 2024

View reviewed changes

test/object-store/realm.cpp Show resolved Hide resolved

More updates from review

3c158a9

danieltabacaru approved these changes Mar 27, 2024

View reviewed changes

tgoyne reviewed Mar 27, 2024

View reviewed changes

Michael Wilkerson-Barker added 8 commits April 3, 2024 00:41

Added test to replicate swift autoopen feature

3f29c30

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-can…

a6d55e9

…cel-waits-on-nonfatal-error

Updated test

1ef3639

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-can…

52a1d54

…cel-waits-on-nonfatal-error

Fixed swift build issue

669d09c

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-can…

b83a346

…cel-waits-on-nonfatal-error

Fixed test failures

b2e7406

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-can…

5830b46

…cel-waits-on-nonfatal-error

michael-wb requested a review from dianaafanador3 April 26, 2024 02:03

removed an unused function

c1a25c0

dianaafanador3 approved these changes Apr 26, 2024

View reviewed changes

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-can…

dce8409

…cel-waits-on-nonfatal-error

danieltabacaru approved these changes Apr 26, 2024

View reviewed changes

michael-wb changed the base branch from master to release/14.6.0 April 26, 2024 15:32

Fixed debug message

a34d5f6

michael-wb changed the base branch from release/14.6.0 to prepare-14.6.0 April 26, 2024 15:39

tgoyne reviewed Apr 26, 2024

View reviewed changes

Updates from review

2e0a68f

michael-wb merged commit bddec6c into prepare-14.6.0 Apr 26, 2024
6 of 7 checks passed

michael-wb deleted the mwb/fix-cancel-waits-on-nonfatal-error branch April 26, 2024 16:19

michael-wb mentioned this pull request Apr 26, 2024

Prepare release #7631

Merged

michael-wb mentioned this pull request Apr 26, 2024

Release 14.6.0 merge into master #7637

Closed

This was referenced May 2, 2024

Replace TestAppSession close()/reopen() with scope based TestAppSession instance #7672

Merged

Crash in Realm Sync, realm::SyncSession::handle_location_update_failed, realm_core_v_13_26_0 realm/realm-swift#8508

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RCORE-2060 Enabling 'cancel_waits_on_nonfatal_error' does not cancel waits during location update while offline #7528

RCORE-2060 Enabling 'cancel_waits_on_nonfatal_error' does not cancel waits during location update while offline #7528

michael-wb commented Mar 27, 2024 •

edited

Loading

coveralls-official bot commented Mar 27, 2024 •

edited

Loading

michael-wb commented Mar 27, 2024

jbreams Mar 27, 2024

michael-wb Mar 27, 2024

michael-wb Mar 27, 2024

danieltabacaru commented Mar 27, 2024

michael-wb commented Mar 27, 2024

danieltabacaru Mar 27, 2024

michael-wb Mar 27, 2024

danieltabacaru Mar 27, 2024 •

edited

Loading

michael-wb Mar 27, 2024

danieltabacaru left a comment

danieltabacaru Mar 27, 2024

michael-wb Mar 27, 2024

danieltabacaru Mar 27, 2024

tgoyne Mar 27, 2024

danieltabacaru Mar 27, 2024

michael-wb Mar 27, 2024

dianaafanador3 commented Mar 28, 2024 •

edited

Loading

michael-wb commented Mar 28, 2024

dianaafanador3 left a comment

michael-wb commented Apr 26, 2024

danieltabacaru Apr 26, 2024

danieltabacaru Apr 26, 2024

michael-wb Apr 26, 2024

danieltabacaru Apr 26, 2024

tgoyne Apr 26, 2024

michael-wb Apr 26, 2024

tgoyne Apr 26, 2024

michael-wb Apr 26, 2024

tgoyne Apr 26, 2024


		TestMode mode = GENERATE(location_fails, token_fails, token_not_authorized);

		SECTION(util::format("access token expired when realm is opened - mode: %1", txt_test_mode(mode))) {

RCORE-2060 Enabling 'cancel_waits_on_nonfatal_error' does not cancel waits during location update while offline #7528

RCORE-2060 Enabling 'cancel_waits_on_nonfatal_error' does not cancel waits during location update while offline #7528

Conversation

michael-wb commented Mar 27, 2024 • edited Loading

What, How & Why?

☑️ ToDos

coveralls-official bot commented Mar 27, 2024 • edited Loading

Pull Request Test Coverage Report for Build michael.wilkersonbarker_1057

Details

💛 - Coveralls

michael-wb commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru commented Mar 27, 2024

michael-wb commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianaafanador3 commented Mar 28, 2024 • edited Loading

michael-wb commented Mar 28, 2024

dianaafanador3 left a comment

Choose a reason for hiding this comment

michael-wb commented Apr 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-wb commented Mar 27, 2024 •

edited

Loading

coveralls-official bot commented Mar 27, 2024 •

edited

Loading

danieltabacaru Mar 27, 2024 •

edited

Loading

dianaafanador3 commented Mar 28, 2024 •

edited

Loading