Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RCORE-1982 Opening realm with cached user while offline results in fatal error and session does not retry connection #7469

Merged
merged 29 commits into from
Mar 20, 2024
Merged

RCORE-1982 Opening realm with cached user while offline results in fatal error and session does not retry connection #7469

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ff43531
Moved to using a pre-initialized sync-route instead of leaving empty …
Mar 12, 2024
ee21683
Merge branch 'master' of github.com:realm/realm-core into mwb/locatio…
Mar 12, 2024
a0fdcc7
Merge branch 'master' of github.com:realm/realm-core into mwb/locatio…
Mar 13, 2024
82f9e49
Added base url update logic when session connection fails - added ver…
Mar 14, 2024
ad7d068
Merge branch 'master' of github.com:realm/realm-core into mwb/locatio…
Mar 14, 2024
cdda1d4
Updated changelog; added default_base_url for C_API; added restart_se…
Mar 14, 2024
5706e02
Fixed c_api compile error
Mar 14, 2024
3fad39c
Silly uninitialized variable
Mar 14, 2024
bcdb8f5
Merge branch 'master' of github.com:realm/realm-core into mwb/locatio…
Mar 14, 2024
e4fe219
Updates from review; fixed deadlock in test
Mar 14, 2024
8431a1d
Updated test
Mar 14, 2024
963ae15
Changed default base url to function in CAPI
Mar 14, 2024
8f86fe0
Updates from review
Mar 14, 2024
b700c1e
Removed old test and updated translationg comments in create_ws_host_url
Mar 15, 2024
da97c9d
Merge branch 'master' of github.com:realm/realm-core into mwb/locatio…
Mar 15, 2024
f6e50f1
Updates from review
Mar 16, 2024
1c4ebf2
Merge branch 'master' of github.com:realm/realm-core into mwb/locatio…
Mar 16, 2024
5c70cec
Updated changelog after release buidl
Mar 16, 2024
7b8be30
Updates from review
Mar 17, 2024
780388b
Moved HookedSocketProvider and HookedTransport to sync_test_utils for…
Mar 18, 2024
5e8d002
Added test for updating the an invalid sync route using local server
Mar 19, 2024
8da1dbd
Merge branch 'master' of github.com:realm/realm-core into mwb/locatio…
Mar 19, 2024
7c16496
Expanded test to use multiple realms with and without multiplexing
Mar 19, 2024
f412b5b
Delete directory _after_ stopping app...
Mar 19, 2024
d3d0c7b
Merge branch 'master' of github.com:realm/realm-core into mwb/locatio…
Mar 19, 2024
a0ad7d4
Updates from review
Mar 19, 2024
b4b6391
Merge branch 'master' of github.com:realm/realm-core into mwb/locatio…
Mar 19, 2024
dc37ede
Updated comment and changelog from review
Mar 20, 2024
d9f32bc
Merge branch 'master' of github.com:realm/realm-core into mwb/locatio…
Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/realm/sync/noinst/client_impl_base.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,18 +584,18 @@ bool Connection::websocket_closed_handler(bool was_clean, WebSocketError error_c
break;
}
case WebSocketError::websocket_fatal_error: {
// Error is fatal if the sync_route has already been verified
SessionErrorInfo error_info(
{ErrorCodes::SyncConnectFailed, util::format("Failed to connect to sync: %1", msg)},
IsFatal{m_server_endpoint.is_verified});
ConnectionTerminationReason reason = ConnectionTerminationReason::http_response_says_fatal_error;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the correct reason if the errors is not fatal (i.e, is_verified = false)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - if the location hasn't been updated since the App was created, then we want this to not be a fatal error and perform a location update to try to request the correct sync route from the base URL address. Until the location info has been requested successfully, this error will continue to be treated as a nonfatal error and will retry to update the location info using the normal backoff delay.

Until the location info is verified, we're not sure if we got the failure because the URL was generated incorrectly or if the server is actually down/misconfigured.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment to make it more clear

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. My comment was mainly about http_response_says_fatal_error being used even if the error is non-fatal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason value is being updated to connect_operation_failed on line 596 if the sync route has not been verified (i.e. not fatal).

// If the connection fails/times out and the server has not been contacted yet, refresh the location
// to make sure the websocket URL is correct
if (!m_server_endpoint.is_verified) {
SessionErrorInfo error_info(
{ErrorCodes::SyncConnectFailed, util::format("Failed to connect to sync: %1", msg)},
IsFatal{false});
error_info.server_requests_action = ProtocolErrorInfo::Action::RefreshLocation;
involuntary_disconnect(std::move(error_info), ConnectionTerminationReason::connect_operation_failed);
break;
reason = ConnectionTerminationReason::connect_operation_failed;
}
involuntary_disconnect(SessionErrorInfo({ErrorCodes::ConnectionClosed, msg}, IsFatal{true}),
ConnectionTerminationReason::http_response_says_fatal_error);
involuntary_disconnect(std::move(error_info), reason);
break;
}
case WebSocketError::websocket_forbidden: {
Expand Down
19 changes: 9 additions & 10 deletions test/object-store/sync/app.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3242,8 +3242,7 @@ TEST_CASE("app: sync integration", "[sync][pbs][app][baas]") {
SyncTestFile r_config(user, partition, schema);

std::vector<SharedRealm> realms;
int i;
for (i = 0; i < num_realms; i++) {
for (int i = 0; i < num_realms; i++) {
SyncTestFile r_config(user, partition, schema);
realms.push_back(Realm::get_shared_realm(r_config));
}
Expand Down Expand Up @@ -4240,7 +4239,7 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") {
}

socket_provider->endpoint_verify_func = [&use_ssl, &expected_host,
&expected_port](sync::WebSocketEndpoint& ep) {
&expected_port](const sync::WebSocketEndpoint& ep) {
CHECK(ep.address == expected_host);
CHECK(ep.port == expected_port);
CHECK(ep.is_ssl == use_ssl);
Expand Down Expand Up @@ -4289,7 +4288,7 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") {
}

socket_provider->endpoint_verify_func = [&use_ssl, &initial_host,
&initial_port](sync::WebSocketEndpoint& ep) {
&initial_port](const sync::WebSocketEndpoint& ep) {
CHECK(ep.address == initial_host);
CHECK(ep.port == initial_port);
CHECK(ep.is_ssl == use_ssl);
Expand All @@ -4316,12 +4315,12 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") {
// After number of location verify attempts has passed, let the location succeed
if (--retry_count <= 0) {
redir_transport->reset(init_url, redir_url);
socket_provider->endpoint_verify_func = [&use_ssl, &expected_host,
&expected_port](sync::WebSocketEndpoint& ep) {
CHECK(ep.address == expected_host);
CHECK(ep.port == expected_port);
CHECK(ep.is_ssl == use_ssl);
};
socket_provider->endpoint_verify_func =
[&use_ssl, &expected_host, &expected_port](const sync::WebSocketEndpoint& ep) {
CHECK(ep.address == expected_host);
CHECK(ep.port == expected_port);
CHECK(ep.is_ssl == use_ssl);
};
return TestState::location_failed;
}
redir_transport->location_requested = false;
Expand Down
9 changes: 4 additions & 5 deletions test/object-store/util/sync/sync_test_utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,14 +257,13 @@ struct HookedSocketProvider : public sync::websocket::DefaultSocketProvider {
std::unique_ptr<sync::WebSocketInterface> connect(std::unique_ptr<sync::WebSocketObserver> observer,
sync::WebSocketEndpoint&& endpoint) override
{
sync::WebSocketEndpoint ep{std::move(endpoint)};
std::optional<SocketProviderError> error;
if (endpoint_verify_func) {
endpoint_verify_func(ep);
endpoint_verify_func(endpoint);
}

if (websocket_endpoint_resolver) {
ep = websocket_endpoint_resolver(std::move(ep));
endpoint = websocket_endpoint_resolver(std::move(endpoint));
}

if (websocket_connect_func) {
Expand All @@ -278,7 +277,7 @@ struct HookedSocketProvider : public sync::websocket::DefaultSocketProvider {
}

std::unique_ptr<sync::WebSocketInterface> websocket =
DefaultSocketProvider::connect(std::move(observer), std::move(ep));
DefaultSocketProvider::connect(std::move(observer), std::move(endpoint));
if (error && error->status_code > 0) {
auto default_websocket = dynamic_cast<sync::websocket::DefaultWebSocket*>(websocket.get());
if (default_websocket)
Expand All @@ -288,7 +287,7 @@ struct HookedSocketProvider : public sync::websocket::DefaultSocketProvider {
}

std::function<sync::WebSocketEndpoint(sync::WebSocketEndpoint&&)> websocket_endpoint_resolver;
std::function<void(sync::WebSocketEndpoint& endpoint)> endpoint_verify_func;
std::function<void(const sync::WebSocketEndpoint&)> endpoint_verify_func;
std::function<std::optional<SocketProviderError>()> websocket_connect_func;
};

Expand Down
Loading