fix: consume parser error on dfly load #4556
Conversation
Signed-off-by: kostas <[email protected]>
src/server/dflycmd.cc
Outdated
| @@ -533,6 +533,8 @@ void DflyCmd::Load(CmdArgList args, RedisReplyBuilder* rb, ConnectionContext* cn | |||
| } | |||
|
|
|||
| if (parser.HasError()) { | |||
| // consume error | |||
| parser.Error(); | |||
There was a problem hiding this comment.
I think this is redundant. Maybe we should remove the DCHECK from the destructor of the parser ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🤷
| @@ -528,11 +528,7 @@ void DflyCmd::Load(CmdArgList args, RedisReplyBuilder* rb, ConnectionContext* cn | |||
| existing_keys = ServerFamily::LoadExistingKeys::kOverride; | |||
| } | |||
|
|
|||
| if (parser.HasNext()) { | |||
| parser.Error(); | |||
There was a problem hiding this comment.
Yeah, don't consume the error if you have extra arguments. We should reject dfly load path append nonsense :)
DFLY LOADtriggers the parser's destructorDCHECKwhich checks if the parsed error was consumed.Furthermore, fixes
dfly load path append nonsensesucceeding withok.IMO do we really need that
DCHECKin the destructor ? Sometimes, there is no really reason to even consume it (see this PR as an example).