Make SyncError/SessionErrorInfo Status-aware #6824
Conversation
| @@ -1111,16 +1112,21 @@ void Connection::close_due_to_protocol_error(std::error_code ec, std::optional<s | |||
| void Connection::close_due_to_client_side_error(std::error_code ec, std::optional<std::string_view> msg, | |||
There was a problem hiding this comment.
I guess this (std::error_code) will be removed/addressed in later PR's
There was a problem hiding this comment.
Yeah. I'm just trying to do this in stages.
| : ProtocolErrorInfo(ec.value(), msg, try_again) | ||
| , error_code(ec) | ||
|
|
||
| SessionErrorInfo(Status status, bool try_again) |
There was a problem hiding this comment.
Should this be a Status&& or was this intentional?
There was a problem hiding this comment.
It's both unintentional and does not need to be changed? I haven't verified that all these call sites pass by r-value but Status's are cheap to copy anyways?
| enum class ClientResetModeAllowed { DoNotClientReset, RecoveryPermitted, RecoveryNotPermitted }; | ||
|
|
||
| Status status; |
There was a problem hiding this comment.
Status should probably be a const, since there are two string_views that depend on its contents.
There was a problem hiding this comment.
Making this const makes SyncError non-assignable - i.e. you can't do something like SyncError copy = other_sync_error or std::optional<SyncError> maybe_sync_error; maybe_sync_error = sync_error; I'm not sure adding that restriction to this type is worth it compared to say a comment saying not to move out the Status if you intend to re-use the SyncError?
There was a problem hiding this comment.
Understood - the side effects of const are not worth the extra headaches... and a const status() accessor is less efficient.
A comment would probably be good, since one may not notice the string_view references without looking into the class implementation.
* 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 is a transitional PR to make SyncError and sync::SessionErrorInfo aware of Status and use it instead of std::error_code to capture error information. Just in this PR all the actual errors are still
std::error_codevia the SystemError type, but this change lets me start to transition our various error types without having to change all our tests/call sites at once.☑️ ToDos