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.

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

Unify RESP error serialization #252

Merged
merged 48 commits into from
Apr 8, 2024

Conversation

PaulusParssinen
Copy link
Contributor

@PaulusParssinen PaulusParssinen commented Apr 6, 2024

In #223 @vazois wished to remove the need from callers to format error messages.

In RESP spec, it is said that the serialized errors of the form -ERR [errorMessage]\r\n are "generic errors". I used this distinction to add WriteGenericError to RespWriteUtils.

The original plan was to have WriteGenericError, which would only take in the error message for the generic errors which are most common. However that specialization would complicate and confuse I abandoned it and instead went for the WriteError

Before this PR all error response serialization happened either by copying the static ASCII encoded error constants or by manually formatting them in the middle of parsing code. The parsing logic often returned a ROS<byte> that could either be a simple error or a CmdString.RESP_OK and used this as the indicator whether an operation was successful or not. Often this returned ROS<byte> was directly copied to the send buffer. This approach was very error prone, as the caller had to correctly format the error string to start with - and end with a terminator \r\n. This was hard to remember and while doing this PR I came across more where this was forgotten or lost during refactors.

This PR flows all simple error serialization string to RespWriteUtils.WriteError. This requires making the error serialization a separate operation from serializing "+OK\r\n" and such required some adjustments in many method overloads. A common pattern introduced in this PR and that already existed in the project is familiar and intuitive bool TryX(.., out Y value) pattern from BCL. In majority of the places where we previously returned a ROS<byte> of either error or OK, we now return boolean indicating whether the there was error or not. If there was error (it returned false), we can grab the error string from out parameter.

@PaulusParssinen
Copy link
Contributor Author

PaulusParssinen commented Apr 6, 2024

Starting to think special casing the generic errors would be a mistake and everything should flow into WriteError instead.. The split will be more confusing than it would help..

Having WriteGenericError taking in constants in different format (missing ERR prefix which was added in the write method) would eventually lead to duplication as there are codepaths where non-generic and generic errors are both used.

edit: As I'm doing the switchover, it's very clear that this is better approach. For example, StackExchange.Redis exceptions now have the errors in same form as we write them to CmdStrings etc.

@PaulusParssinen PaulusParssinen changed the title Unify generic error serialization Unify RESP error serialization Apr 7, 2024
@PaulusParssinen PaulusParssinen marked this pull request as ready for review April 7, 2024 00:43
@badrishc badrishc requested a review from vazois April 8, 2024 16:09
libs/cluster/Session/ClusterCommands.cs Outdated Show resolved Hide resolved
libs/cluster/Session/ClusterCommands.cs Outdated Show resolved Hide resolved
libs/cluster/Server/ClusterManagerWorkerState.cs Outdated Show resolved Hide resolved
@badrishc
Copy link
Contributor

badrishc commented Apr 8, 2024

Fantastic cleanup!!

@PaulusParssinen PaulusParssinen marked this pull request as draft April 8, 2024 21:40
@PaulusParssinen
Copy link
Contributor Author

PaulusParssinen commented Apr 8, 2024

Gonna do one more commit where I move more of these constant error strings to CmdStrings

@vazois
Copy link
Contributor

vazois commented Apr 8, 2024

Gonna do one more commit where I move more of these constant error strings to CmdStrings

Sounds good, though try to avoid big PRs since they are hard to review and could result in a lot of conflicts at merge time.

@PaulusParssinen
Copy link
Contributor Author

Gonna do one more commit where I move more of these constant error strings to CmdStrings

Sounds good, though try to avoid big PRs since they hard to review and could result in a lot of conflicts at merge time.

Yeah.. these tend to get a bit out of hand too easily😬I'll leave the constant as follow-up for anyone to do instead..

@PaulusParssinen PaulusParssinen marked this pull request as ready for review April 8, 2024 21:59
@vazois vazois merged commit 765e614 into microsoft:main Apr 8, 2024
21 checks passed
@PaulusParssinen PaulusParssinen deleted the write-generic-resp-error branch April 8, 2024 22:10
@github-actions github-actions bot locked and limited conversation to collaborators Jun 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants