Make the network code compile #6262

jedelbo · 2023-02-01T13:34:32Z

The handling of error codes is a somewhat hybrid solution. To put it nicely.

danieltabacaru · 2023-02-02T15:26:48Z

src/realm/sync/noinst/client_impl_base.cpp

@@ -412,29 +425,37 @@ bool Connection::websocket_closed_handler(bool was_clean, Status status)
 return bool(m_websocket);
 }

- auto&& status_code = status.code();
- std::error_code error_code{static_cast<int>(status_code), websocket::websocket_close_status_category()};


the sync_session expects the websocket_close_status_category. Why not have a category for websocket error/close codes instead of the dummy category?

danieltabacaru · 2023-02-02T15:26:56Z

src/realm/sync/noinst/client_impl_base.cpp

- std::error_code error_code{static_cast<int>(status_code), websocket::websocket_close_status_category()};
+ int status_code = status.code();
+ std::error_code error_code;
+ if (status_code == ErrorCodes::SystemError) {


is this check really needed?

danieltabacaru · 2023-02-02T15:34:47Z

src/realm/error_codes.h

- RLM_ERR_WEBSOCKET_INAVALIDEXTENSION = 4409,
- RLM_ERR_WEBSOCKET_INTERNALSERVERERROR = 4410,
- RLM_ERR_WEBSOCKET_TLSHANDSHAKEFAILED = 4411,
+ RLM_ERR_WEBSOCKET_RESOLVE_FAILED = 4400,


We could consider adding a new enum just for websocket related error/close codes and remove the one in error_codes.hpp. Wouldn't that work?

Also, merge the ones here with WebSocketError since they all fit together

I guess the main problem is that the codes will have new values, since 1xxx may be be used already for example. For server values, we could translate them in websocket.cpp and pass the new values along. Of course, a custom websocket implementation would have to do the same.

jedelbo · 2023-02-03T09:53:43Z

@danieltabacaru I made a new attempt. Perhaps better.

danieltabacaru · 2023-02-03T11:30:51Z

src/realm/sync/noinst/client_impl_base.cpp

@@ -403,6 +403,19 @@ void Connection::websocket_error_handler()
 m_websocket_error_received = true;
 }

+namespace {


this is not needed anymore

danieltabacaru · 2023-02-03T11:36:41Z

src/realm/sync/network/websocket.cpp

@@ -1109,10 +1109,10 @@ class CloseStatusErrorCategory : public std::error_category {
 {
 // Converts an error_code to one of the pre-defined status codes in
 // https://tools.ietf.org/html/rfc6455#section-7.4.1
- if (error_code == 1000 || error_code == 0) {
- return ErrorCodes::error_string(ErrorCodes::OK);
+ if (error_code == 0) {


I think a value of 0 is not possible anymore. This was used when ErrorCodes::OK was 0 and there was no WebSocket_OK but I see you addressed that. I think you don't need to handle this in a special way anymore.

danieltabacaru · 2023-02-03T12:07:06Z

We should use realm_web_socket_errno_e instead of status_error_code_e in realm.h for realm_sync_socket_callback_complete and realm_sync_socket_websocket_closed

danieltabacaru · 2023-02-03T12:14:11Z

I think this is better than the previous approach.

jbreams · 2023-02-13T16:01:53Z

src/realm/error_codes.hpp

+ UnknownError = RLM_ERR_UNKNOWN,
+ };
+
+ enum WebSocketError : int32_t {


Do we want to keep this in the ErrorCodes namespace? Should we just move these into src/realm/sync/network/websocket.hpp as a regular error enum? I think error_codes.hpp should be for errors we want users/sdks to have to handle - these seem like more internal status codes specific to the websocket implementation.

That sounds like a good idea. Added the change.

…WEBSOCKET for sync errors in C API

jbreams

I really really strongly deeply don't like it that we've added std::error_code to Status, but it doesn't look like that was not done as part of this PR. So, LGTM when the tests pass.

michael-wb

The changes look good, but this branch likely needs a pull from the parent, since it's still using an old version of the install_baas.sh script, which is causing the object-store-tests to fail to start.

jedelbo · 2023-02-23T13:37:23Z

I really really strongly deeply don't like it that we've added std::error_code to Status, but it doesn't look like that was not done as part of this PR. So, LGTM when the tests pass.

I agree. This was done as a way of adapting to the new exception system without re-implementing the whole sync code (#5462 some 10 months ago). This should definitely be changed at some point in the future.

danieltabacaru · 2023-02-23T13:42:51Z

Jørgen and I are working on merging the parent branch into this so the tests can run.

michael-wb

I believe the missing restart_session is causing the redirect failure.

michael-wb · 2023-02-23T17:37:13Z

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

- }
+ if (error_code == sync::websocket::WebSocketError::websocket_unauthorized ||
+ error_code == sync::websocket::WebSocketError::websocket_abnormal_closure ||
+ error_code == sync::websocket::WebSocketError::websocket_moved_permanently) {


Missing the restart_session that is passed into u->refresh_custom_data()

that was it!

michael-wb · 2023-02-23T17:38:17Z

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

+ error = SyncError(new_error_code, error.reason(), error.is_fatal);
+ }


Also set error.is_unrecognized_by_client = true;, since this was being done before.

this should not be a problem, since these errors (coarse-grained websocket errors) are recognized by the client

jedelbo requested a review from danieltabacaru February 1, 2023 13:34

cla-bot bot added the cla: yes label Feb 1, 2023

github-actions bot assigned jedelbo Feb 1, 2023

jedelbo force-pushed the je/fix-websocket branch 2 times, most recently from 7480e1c to 0944929 Compare February 2, 2023 14:17

Make the network code compile

faa91d1

The handling of error codes is a somewhat hybrid solution. To put it nicely.

jedelbo force-pushed the je/fix-websocket branch from 0944929 to faa91d1 Compare February 2, 2023 15:13

danieltabacaru reviewed Feb 2, 2023

View reviewed changes

More clean approach

a51136f

jedelbo requested a review from danieltabacaru February 3, 2023 09:53

danieltabacaru reviewed Feb 3, 2023

View reviewed changes

Update after review

725dcd7

danieltabacaru requested review from michael-wb and jbreams February 6, 2023 20:00

danieltabacaru added 2 commits February 9, 2023 13:28

Clean-up

8a432d1

fixes

3a2f760

jbreams reviewed Feb 13, 2023

View reviewed changes

Move WebSocketError definition to websocket.hpp

83cc6f2

jedelbo requested a review from jbreams February 20, 2023 12:05

danieltabacaru added 3 commits February 22, 2023 12:41

WebSocket errors refactoring

2486b51

Introduce coarse-grained websocket errors to ErrorCodes::Error

f86236d

Replace RLM_SYNC_ERROR_CATEGORY_RESOLVE with RLM_SYNC_ERROR_CATEGORY_…

0f7e135

…WEBSOCKET for sync errors in C API

jbreams approved these changes Feb 22, 2023

View reviewed changes

michael-wb approved these changes Feb 22, 2023

View reviewed changes

danieltabacaru and others added 3 commits February 23, 2023 14:44

Merge branch 'je/exception-unification-merge' into je/fix-websocket

883c1a9

Merge branch 'je/exception-unification-merge' into je/fix-websocket

6d02199

Fix object-store tests

9f029d2

michael-wb reviewed Feb 23, 2023

View reviewed changes

Fix wrong merge in sync_session.cpp

5767d6d

danieltabacaru merged commit 650865c into je/exception-unification-merge Feb 24, 2023

danieltabacaru deleted the je/fix-websocket branch February 24, 2023 07:03

desistefanova mentioned this pull request Feb 25, 2023

Upgrade core 13.5.0 and fix error messages realm/realm-dart#1182

Merged

kiburtse mentioned this pull request Mar 3, 2023

Merge master, adapt exceptions #6361

Merged

3 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the network code compile #6262

Make the network code compile #6262

jedelbo commented Feb 1, 2023

danieltabacaru Feb 2, 2023

danieltabacaru Feb 2, 2023

danieltabacaru Feb 2, 2023

danieltabacaru Feb 2, 2023

danieltabacaru Feb 2, 2023

jedelbo commented Feb 3, 2023

danieltabacaru Feb 3, 2023

danieltabacaru Feb 3, 2023

danieltabacaru commented Feb 3, 2023

danieltabacaru commented Feb 3, 2023

jbreams Feb 13, 2023

jedelbo Feb 20, 2023

jbreams left a comment

michael-wb left a comment

jedelbo commented Feb 23, 2023

danieltabacaru commented Feb 23, 2023

michael-wb left a comment

michael-wb Feb 23, 2023

danieltabacaru Feb 23, 2023

michael-wb Feb 23, 2023

danieltabacaru Feb 23, 2023

		error = SyncError(new_error_code, error.reason(), error.is_fatal);
		}

Make the network code compile #6262

Make the network code compile #6262

Conversation

jedelbo commented Feb 1, 2023

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

jedelbo commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru commented Feb 3, 2023

danieltabacaru commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbreams left a comment

Choose a reason for hiding this comment

michael-wb left a comment

Choose a reason for hiding this comment

jedelbo commented Feb 23, 2023

danieltabacaru commented Feb 23, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment