Update websocket error messages to be more descriptive #6298
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CHANGELOG.md
Outdated
| @@ -22,6 +22,7 @@ | |||
| * The lifecycle of the sync client is now separated from the event loop/socket provider it uses for async I/O/timers. The sync client will wait for all outstanding callbacks/sockets to be closed during destruction. The SyncSocketProvider passed to the sync client must run until after the sync client is destroyed but does not need to be stopped as part of tearing down the sync client. ([PR #6276](https://github.com/realm/realm-core/pull/6276)) | |||
| * The default event loop will now keep running until it is explicitly stopped rather than until there are no more timers/IO to process. Previously there was a timer set for very far in the future to force the event loop to keep running. ([PR #6265](https://github.com/realm/realm-core/pull/6265)) | |||
| * Disable failing check in Metrics_TransactionTimings test ([PR #6206](https://github.com/realm/realm-core/pull/6206)) | |||
| * Update websocket error messages to be more descriptive ([PR #6298](https://github.com/realm/realm-core/pull/6298)) | |||
There was a problem hiding this comment.
I know this is an internal message but it's a bit misleading. It's actually just surfacing to the user the actual websocket/http error.
jbreams
approved these changes
Feb 13, 2023
| @@ -477,7 +477,7 @@ class ClientImpl::Connection { | |||
| void handle_message_received(util::Span<const char> data); | |||
| void initiate_disconnect_wait(); | |||
| void handle_disconnect_wait(Status status); | |||
| void read_or_write_error(std::error_code); | |||
| void read_or_write_error(std::error_code ec, std::optional<std::string_view> msg = std::nullopt); | |||
There was a problem hiding this comment.
Do we ever call this without a message - do we need to make this new parameter a nullopt-by-default? Does it even need to be optional?
There was a problem hiding this comment.
In the case of a simulated read error, read_or_write_error is called without a message. I could add a fixed message (e.g. 'simulated read error') to use in this case.
There was a problem hiding this comment.
Yeah, in those like 1-2 cases, let's just call this with a fixed "simulated read error" message.
added 6 commits
February 13, 2023 10:28
…ore into mwb/fix-app-destroyed-test
3 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What, How & Why?
The 'app destroyed during token refresh' was failing occasionally since the error message passed to the error_handler no longer had the descriptive message for the websocket close status code. Instead of seeing the
'Bad WebSocket response 404 not found'message in the exception, it was just seeing'WebSocket: Fatal Error'and could not determine if this was an expected or unexpected exception that was thrown during this test.These changes pass the original
status.reason()string to the Connection error function in addition to websocket close status code to provide a more descriptive message for the error.☑️ ToDos
[ ] C-API, if public C++ API changed.