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

fix: consume parser error on dfly load #4556

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: consume parser error on dfly load #4556

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04a1ced
fix: consume parser error on dfly load
kostasrim Feb 4, 2025
8c7e914
fixes
kostasrim Feb 4, 2025
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
Next Next commit
fix: consume parser error on dfly load 
Signed-off-by: kostas <[email protected]>
  • Loading branch information
@kostasrim
kostasrim committed Feb 4, 2025
commit 04a1cede5d20ac8a75c249441ab122ba033de38f
2 changes: 2 additions & 0 deletions src/server/dflycmd.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,8 @@ void DflyCmd::Load(CmdArgList args, RedisReplyBuilder* rb, ConnectionContext* cn
}

if (parser.HasError()) {
// consume error
parser.Error();
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 think this is redundant. Maybe we should remove the DCHECK from the destructor of the parser ?

Copy link
Contributor

Choose a reason for hiding this comment

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

No DCHECK gives us an understanding of whether we check the error or not. Maybe instead of consuming the error we can return a more appropriate error that should be sent via reply builder

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 I understand what the DCHECK does. All I am saying is that for certain code paths (like the one found here), there is no really a reason to consume the error. On the other hand you can do that in one go if (parse.Error()) to both check for error && consume it which I guess is fine also 🤷

return rb->SendError(kSyntaxErr);
}

Expand Down
Loading