chore: enable ReplyGuard in ReplyBuilder2 #3705
Conversation
Signed-off-by: kostas <kostas@dragonflydb.io>
| @@ -856,11 +856,13 @@ void ReqSerializer::SendCommand(std::string_view str) { | |||
|
|
|||
| void RedisReplyBuilder2Base::SendNull() { | |||
| ReplyScope scope(this); | |||
| has_replied_ = true; | |||
There was a problem hiding this comment.
please set this flag when we drop a scope, not manually everywhere
There was a problem hiding this comment.
I am aware but this would break MCReplyBuilder2. ReplyScope accepts a SinkReplyBuilderv2. However, has_replied_ belongs to a different class, SinkReplyBuilder. The problem is that this argument for RedisReplyBuilder2 would work just fine because the RedisReplyBuilder2Base inherits from: SinkReplyBuilder2, RedisReplyBuilder so it's safe to cast it to RedisReplyBuilder. However this is not the case for MCReplyBuilder2.
I agree has_replied_ should be part of ReplyScope. For this we need:
- Clean up the hierarchy of inheritence
- Or even introduce a new has_replied_ flag. However this would no work very well because we use
SinkReplyBuilder*which would poll the base class flag and we also can't really remove it because we don't want to break previously existed classes
Which both are beyond the scope of this pr
There was a problem hiding this comment.
it should've been just part of the SinkReplyBuilder2
There was a problem hiding this comment.
uh I was not expecting you would reply early :( I will push a quick change on that once I get a 5 minute free time :)
Although that means we break SinkReplyBuilder. Imo what we can do is just make HasReplied virtual and it will solve all of our problems. I will take care of this.
P.s. glad to see you commenting ❤️
This PR 1f36c99 disabled the ReplyGuard.
Resolves #3690