Unify remaining std::error_codes into Status/ErrorCodes in sync client #6869

jbreams · 2023-08-08T21:21:52Z

What, How & Why?

This is the last step of the big error unification refactor. It moves translation of ProtocolError codes into realm ErrorCodes into the sync client. It removes std::error_code from Status and friends. It adjusts tests that were depending on old std::error_code values.

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
C-API, if public C++ API changed.

…error_code

…rrors

…t_steps

danieltabacaru · 2023-08-10T08:02:13Z

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

@@ -485,8 +485,14 @@ static inline realm_version_id_t to_capi(const VersionID& v)
    return version_id;
 }

-realm_sync_error_code_t to_capi(const Status& status, std::string& message);
-void sync_error_to_error_code(const realm_sync_error_code_t& sync_error_code, std::error_code* error_code_out);
+static inline realm_error_t to_capi(const Status s)


nit: keep the const&

danieltabacaru · 2023-08-10T10:43:15Z

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

    util::Optional<ShouldBackup> delete_file;
    bool log_out_user = false;
    bool unrecognized_by_client = false;

+    // Can be called from within SyncSession with an AuthError to handle app-level authentication problems,
+    // so even if there's an error action, we need to set a fall-back to logging out the user and making


when there is no error action (i.e, a synthetized error), we'll set unrecognized_by_client to true. I don't think we want that. This behavior is different than before.

Whoops. I've actually mis-understood this code flow a bit and I've changed this so that if you want this behavior you need to pass in an action of LogOutUser

danieltabacaru · 2023-08-10T10:49:06Z

src/realm/status.hpp

-    }
-
-    std::error_code get_std_error_code() const
+    Status(ErrorCodes::Error code, std::string&& reason)


Do we want/need both constructors?

Yeah, so the first constructor makes a copy of a string. This constructor is if you've just created a Status with a new string via util::format() or something. So this is just a little optimization to avoid an allocation/copy.

danieltabacaru · 2023-08-10T10:56:44Z

src/realm/sync/client_base.hpp

    SessionErrorInfo(const ProtocolErrorInfo& info, Status status)
        : ProtocolErrorInfo(info)
        , status(std::move(status))
    {
    }

    SessionErrorInfo(Status status, IsFatal is_fatal)
-        : ProtocolErrorInfo(status.get_std_error_code().value(), status.reason(), is_fatal)
+        : ProtocolErrorInfo(0, {}, is_fatal)


shouldn't we pass the info from status to the ProtocolErrorInfo constructor?

So this is the case where if you are constructing a SessionErrorInfo for something besides a protocol error. I wasn't sure what the right thing to do here is/was, but I settled on leaving the ProtocolErrorInfo portion of the class in an invalid state rather than trying to reverse-translate Status's to ProtocolErrors.

danieltabacaru · 2023-08-10T12:07:45Z

src/realm/sync/noinst/client_impl_base.cpp

-        return {ErrorCodes::SyncProtocolInvariantFailed,
-                util::format("Received ERROR message with unknown error code %1", info.raw_error_code)};
+    auto status = protocol_error_to_status(static_cast<ProtocolError>(info.raw_error_code), info.message);
+    // If the server has requested an action but sent an invalid error code we can still process the error and pass an


I think the assumption is that the server always sends an action (cc @tkaye407), so maybe an assert is more appropriate.

The server should always send an action, yes

Addendum: we will only do it for realm devices that know about the JSONErrorMessage, but I dont think that matters in this case

I'm going to fix this in a separate PR.

danieltabacaru · 2023-08-10T12:11:22Z

src/realm/sync/noinst/client_impl_base.cpp

+    if (status == ErrorCodes::UnknownError && info.server_requests_action != ProtocolErrorInfo::Action::NoAction) {
+        return {
+            ErrorCodes::SyncProtocolInvariantFailed,
+            util::format("Received ERROR message with unknown error code %1 and no action.", info.raw_error_code)};


an action is received in this case

Yup, this logic is wrong I've undone it and will fix it up in a separate review.

I'll take that you'll update this message as well in that pr.

danieltabacaru · 2023-08-10T14:49:28Z

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

    }
    if (user) {
        user->log_out();
    }

    if (auto error_handler = config(&SyncConfig::error_handler)) {
-        auto user_facing_error =
-            SyncError(Status{realm::sync::ProtocolError::bad_authentication, context_message}, true);
+        auto user_facing_error = SyncError({ErrorCodes::AuthError, status.reason()}, true);


you need to set the correct action (LogOutUser).

So my confusion here is that this is not getting passed to handle_error(), it's getting passed to error_handler and the action actually has no effect. I can set it here for completeness, but it's not going to get used.

ah, I see. It's calling the user callback directly.

is it the right thing though? 🤔

Not sure what you mean? What else would we do here?

so I was wondering if we should call handle_error() instead, so we actually log out the user.

But we log out the user right above on line 248

I completely missed that. I think the only difference (from being handled in handle_error()) then is that the session may remain active. But it's the same as before so maybe not an issue.

The session should already be closed at this point: this path is called when a SyncSession is being revived and will never become_active() on a bad auth or if the websocket connection failed for bad auth or redirect during open.

danieltabacaru · 2023-08-10T15:58:02Z

src/realm/sync/protocol.hpp

@@ -364,34 +365,17 @@ enum class ProtocolError {
    // clang-format on
 };

+Status protocol_error_to_status(ProtocolError raw_error_code, std::string_view msg);


you have this declared at line 377.

Fixed. I think this was a merge issue 😞

danieltabacaru · 2023-08-10T15:58:18Z

src/realm/sync/protocol.cpp

+            case ProtocolError::client_file_expired:
+                [[fallthrough]];
+            case ProtocolError::bad_client_file:
+                return ErrorCodes::SyncClientResetRequired;


there are probably other cases when a client reset is required (e.g, migrate_to_flx). what is this used for?

These are error codes that specifically always mean that you should client reset. migrate_to_flx has the error code WrongSyncType.

michael-wb

Looks good - there's a lot of good changes (and a lot of work) in the error unification project.
Two tiny comments

michael-wb · 2023-08-10T18:43:08Z

test/sync_fixtures.hpp

@@ -712,8 +712,7 @@ class MultiClientServerFixture {
                    return;
                REALM_ASSERT(error);
                unit_test::TestContext& test_context = m_test_context;
-                test_context.logger->error("Client disconnect: %1: %2 (is_fatal=%3)",
-                                           error->status.get_std_error_code(), error->message, error->is_fatal);
+                test_context.logger->error("Client disconnect: %1 (is_fatal=%3)", error->status, error->is_fatal);


is_fatal index is wrong

michael-wb · 2023-08-10T19:35:59Z

src/realm/sync/protocol.hpp

        // sync websocket is closed with specific error codes.
        RefreshUser,
        RefreshLocation,
+        LogOutUser,
    };

    ProtocolErrorInfo() = default;


This isn't related to your changes, but should we explicitly set the default value for server_requests_action to be Action::NoAction - the compiler will use whatever value is at 0, whatever it is...

* Add new ErrorCodes for sync error unification (#6829) * Make SyncError/SessionErrorInfo Status-aware (#6824) * Replace ClientError error_code with ErrorCodes/Status (#6846) * Handle websocket errors entirely within sync client (#6859) * Unify remaining std::error_codes into Status/ErrorCodes in sync client (#6869) * fix changelog merge with master

jbreams added 30 commits July 12, 2023 15:35

big shabang

0c081b4

remove more std::error_codes

66eee63

add websocket_error.hpp header

91a29b8

remove C API realm_sync_error_code and remaining public uses of std::…

04a83d2

…error_code

more test failures

ee757e6

remove unused code

8371bac

make IntegrationException unambiguous

10e5041

even less unambiguous

043fe3f

Merge remote-tracking branch 'origin/master' into jbr/collapse_sync_e…

5abc4cc

…rrors

Merge remote-tracking branch 'origin/master' into jbr/collapse_sync_e…

4c2a8b8

…rrors

Merge remote-tracking branch 'origin/master' into jbr/collapse_sync_e…

daa8fb9

…rrors

remove extrainfo part of status

f7a6b5f

Merge remote-tracking branch 'origin/master' into jbr/collapse_sync_e…

5167a8d

…rrors

Replace ClientError error_code with ErrorCodes/Status

104ca6d

fixes from PR

4fe8d93

changelog

c53c6c8

Handle websocket errors entirely within sync client

753f2ec

Merge branch 'feature/sync_error_unification' into jbr/websocket_error

00ff52e

add missing header

2367f93

undo weird formatting changes

8cb0c68

clang format again

f1e5fce

whoops

e6e8fa9

fix a bunch of IsFatal mixups

7d4ead0

handle redirects

8ecbed3

formatting fixups

982c9d5

fix emscripten build

caa3721

Refresh user on unauthorized error

5833adf

nits

a175488

Merge branch 'jbr/websocket_error' into jbr/protocol_errors

f74beec

Merge branch 'feature/sync_error_unification' into jbr/sync_error_las…

06cdd8f

…t_steps

cla-bot bot added the cla: yes label Aug 8, 2023

github-actions bot assigned jbreams Aug 8, 2023

jbreams added 3 commits August 8, 2023 17:34

fixes

952277c

handle auth errors correctly

a96a3f2

fixups

49df1d3

jbreams changed the title ~~Jbr/sync error last steps~~ Unify remaining std::error_codes into Status/ErrorCodes in sync client Aug 9, 2023

jbreams marked this pull request as ready for review August 9, 2023 13:31

jbreams requested review from danieltabacaru and michael-wb August 9, 2023 13:31

danieltabacaru reviewed Aug 10, 2023

View reviewed changes

fixes from PR

ec5d292

jbreams requested a review from danieltabacaru August 10, 2023 14:06

danieltabacaru reviewed Aug 10, 2023

View reviewed changes

update error message

f2d5c8f

danieltabacaru reviewed Aug 10, 2023

View reviewed changes

remove duplicate

d164b3f

michael-wb approved these changes Aug 10, 2023

View reviewed changes

danieltabacaru approved these changes Aug 10, 2023

View reviewed changes

changelog

3e36355

jbreams merged commit 1bd0a8d into feature/sync_error_unification Aug 11, 2023
2 checks passed

jbreams deleted the jbr/sync_error_last_steps branch August 11, 2023 14:20

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Unify remaining std::error_codes into Status/ErrorCodes in sync client #6869

Unify remaining std::error_codes into Status/ErrorCodes in sync client #6869

Conversation

jbreams commented Aug 8, 2023 • edited Loading

What, How & Why?

☑️ ToDos

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

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

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbreams commented Aug 8, 2023 •

edited

Loading