Handle websocket errors entirely within sync client #6859
Conversation
| SessionErrorInfo error_info{std::move(status), try_again}; | ||
| involuntary_disconnect(std::move(error_info), | ||
| ConnectionTerminationReason::websocket_protocol_violation); // Throws | ||
| close_due_to_client_side_error({ErrorCodes::SyncProtocolInvariantFailed, msg}, IsFatal{false}, |
There was a problem hiding this comment.
should we try to get rid off of calling involuntary_disconnect directly and call close_due_to_... methods instead? I see you fixed it here.
There was a problem hiding this comment.
I don't know? The difference between them right now is that close_due_to_ constructs a SessionErrorInfo for you. I'd almost go for getting rid of close_due_to_ methods in favor of involuntary_disconnect instead? But I don't feel very strongly either way.
There was a problem hiding this comment.
Right. It could be involuntary_disconnect instead, but I don't really like having both.
There was a problem hiding this comment.
Okay, can I do that in a separate PR or do you need to see it done in this one?
There was a problem hiding this comment.
not really now. It would be nice at some point.
| @@ -1924,7 +1934,7 @@ void Session::send_ident_message() | |||
| m_client_file_ident.ident, m_client_file_ident.salt, m_progress.download.server_version, | |||
| m_progress.download.last_integrated_client_version, m_progress.latest_server_version.version, | |||
| m_progress.latest_server_version.salt, active_query_set.version(), active_query_body.size(), | |||
| active_query_body); // Throws | |||
| active_query_body); // T | |||
| @@ -258,22 +262,24 @@ struct ProtocolErrorInfo { | |||
| ClientReset, | |||
| ClientResetNoRecovery, | |||
| MigrateToFLX, | |||
| RevertToPBS | |||
| RevertToPBS, | |||
| RefreshUser, | |||
There was a problem hiding this comment.
Does the server send these two new actions? Or is it something we set internally? If it's the latter, maybe consider adding a comment.
There was a problem hiding this comment.
Right now it's something we set internally, but the server could send it eventually.
| case sync::ProtocolErrorInfo::Action::RefreshUser: | ||
| if (auto u = user()) { | ||
| u->refresh_custom_data(false, handle_refresh(shared_from_this(), false)); | ||
| return; | ||
| } | ||
| break; | ||
| case sync::ProtocolErrorInfo::Action::RefreshLocation: | ||
| if (auto u = user()) { | ||
| u->refresh_custom_data(true, handle_refresh(shared_from_this(), true)); | ||
| return; | ||
| } | ||
| break; |
| @@ -78,15 +78,13 @@ class DefaultWebSocketImpl final : public DefaultWebSocket, public Config { | |||
| { | |||
| m_logger.error("Reading failed: %1", ec.message()); // Throws | |||
| constexpr bool was_clean = false; | |||
| websocket_error_and_close_handler( | |||
| was_clean, Status{make_error_code(WebSocketError::websocket_read_error), ec.message()}); | |||
| websocket_error_and_close_handler(was_clean, WebSocketError::websocket_read_error, ec.message()); | |||
src/realm/sync/network/websocket.cpp
Outdated
| { | ||
| return "realm::sync::websocket::WebSocketError"; | ||
| if (str == nullptr) { | ||
| os << "WebSocket: Unkhown Error (" << static_cast<std::underlying_type_t<WebSocketError>>(code) << ")"; |
| @@ -431,7 +431,8 @@ TEST_CASE("sync: error handling", "[sync][session]") { | |||
| } | |||
|
|
|||
| sync::SessionErrorInfo initial_error{ | |||
| Status{std::error_code{code, realm::sync::protocol_error_category()}, "Something bad happened"}, true}; | |||
| Status{std::error_code{code, realm::sync::protocol_error_category()}, "Something bad happened"}, | |||
There was a problem hiding this comment.
It will be nice once we're finally done with these...
* 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
What, How & Why?
This makes the WebSocketError type just a normal enum that SocketProvider implementations can pass back to the sync client without having to use a special kind of error wrapper. Then, it makes it so all the websocket error handling happens inside of the sync client itself. If a specific websocket error requires a specific action out in object-store, this is handled by an error action rather than by exposing the implementation details of networking to object-store. Finally, this de-duplicates the
try_againandis_fatalflag so that there's only anis_fatalflag that is a TaggedBool.☑️ ToDos