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

Fix Parsing Regression in GarnetClient and GarnetClientSession #456

Merged

Conversation

vazois
Copy link
Contributor

@vazois vazois commented Jun 9, 2024

This PR started as an attempt to fix the parsing regression introduced in 5202499.
It involved into a general refactoring of RespReadUtils to clearly separate the parts used only by the clients to parse RESP responses vs those used internally by the server.
This distinction is crucial because the server does not access negative length headers (i.e. $-1\r\n) but the client can accept them as part of a response packet.

Following is a summary of the related fixes/enhancements:

  • Added RespReadResponseUtils wrapper to cleanly separate client specific Resp parsing methods.
  • Eliminate allowNull flag in RespReadUtils since we do not allow null parsing on the server side.
  • Split ReadLengthHeader into ReadUnsignedLengthHeader and ReadSignedLengthHeader to be used with server and client respectively.
  • Make change to RespOnlineBench to retry requests when an exception occurs (e.g. -ERR MOVED).

@vazois vazois marked this pull request as ready for review June 9, 2024 22:09
@vazois vazois marked this pull request as draft June 10, 2024 18:11
libs/common/RespReadUtils.cs Outdated Show resolved Hide resolved
@vazois vazois force-pushed the vazois/fix-garnetclientsession-regression branch 2 times, most recently from 765dac3 to 9c52e2c Compare June 10, 2024 22:43
@vazois vazois marked this pull request as ready for review June 10, 2024 22:44
@vazois vazois force-pushed the vazois/fix-garnetclientsession-regression branch from e31c3f3 to 9a35c7e Compare June 11, 2024 01:59
libs/common/RespReadUtils.cs Show resolved Hide resolved
libs/common/RespReadUtils.cs Outdated Show resolved Hide resolved
libs/common/RespReadUtils.cs Outdated Show resolved Hide resolved
@vazois vazois force-pushed the vazois/fix-garnetclientsession-regression branch from 3d7d3b2 to 6022841 Compare June 11, 2024 23:19
@vazois vazois force-pushed the vazois/fix-garnetclientsession-regression branch from 6022841 to d38ccb2 Compare June 12, 2024 00:05
@TalZaccai TalZaccai self-requested a review June 12, 2024 01:05
@vazois vazois merged commit 2647813 into microsoft:main Jun 12, 2024
23 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 2024
@vazois vazois deleted the vazois/fix-garnetclientsession-regression branch September 17, 2024 17:58
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.

4 participants