Conversation
🤖 Augment PR SummarySummary: This PR moves protocol parsing to happen immediately after socket reads (instead of deferring to the IoLoopV2 loop body). Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
src/facade/dragonfly_connection.cc
Outdated
| break; | ||
| } | ||
| io_buf_.CommitWrite(*res); | ||
| ParseFromBuffer(); |
There was a problem hiding this comment.
ParseMCBatch() uses a do { ... } while (parsed_cmd_q_len_ < 128 && ...), so calling ParseFromBuffer() repeatedly from the drain loop can still enqueue at least one command even when the parsed-queue is already at/over the intended cap. Can you double-check this can’t lead to unbounded growth of parsed_cmd_q_len_ under heavy pipelining before ExecuteBatch() gets a chance to run?
Severity: medium
Other Locations
src/facade/dragonfly_connection.cc:2606src/facade/dragonfly_connection.cc:2738
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
10469fd to
b7d20de
Compare
Refactor the parsed command queue with named predicates: - HasInFlightCommands(): dispatched commands not yet replied - HasCommandToExecute(): head is ready with nothing in-flight Simplify IoLoopV2 recv loop: move socket draining from DoReadOnRecv into IoLoopV2 directly using pending_input_ flag, eliminating the indirect DoReadOnRecv(true) call and duplicated TryRecv logic. Refactor ReplyBatch to iterate via parsed_head_ directly instead of aliasing it through a for-loop reference variable with exchange(). Add test_get_many memcached test for multi-key get coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Roman Gershman <roman@dragonflydb.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ParseFromBuffer()right after reading, instead of deferring to the main loop bodyParseFromBufferfully depletesio_buf_(enforced by CHECK)CheckIoBufCapacityinto the drain loop so the buffer can grow between recv callsChanges
src/facade/dragonfly_connection.h— AddParseFromBuffer()declarationsrc/facade/dragonfly_connection.cc— AddParseFromBuffer()(calls ParseMCBatch/ParseRedisBatch). Call it afterCommitWritein the drain loop and afterWriteAndCommitin the multishot recv path. Remove redundantParseFromBuffer/NEED_MORE/CheckIoBufCapacityfrom main loop body. AddCHECK_EQ(io_buf_.InputLen(), 0u)after eager parse.🤖 Generated with Claude Code