Implement the inline command format #1278
Open
Conversation
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
|
The issues with the tests all have to do with readHead and endReadHead handling. The one change to 'normal' handling was to advance endReadHead earlier (after all, Garnet already has the command and all the arguments). This handles GarnetObjectStoreDisabledError issue - otherwise, there's a case where the previous LLEN gets in the same command line as the current LLEN. That's masked by AttemptSkipLine() in main - but that code was replaced by the inline command code. I kinda forgot about it when going after the other issue, which is why it came back briefly: The other 4 failures, happen to point at a different unrelated bug which I wasn't aware of. All were cases where GetOperationDirection() got a mixed case argument and tried to upper case it. MakeUpperCase() used bytesRead - readHead as length. When called from Parse code this is fine since there ptr is the entire read. In GetOperationDirection() ptr is just the direction argument, and its length is much smaller than the read length it gets in main. Fortunately, it accidentally still works. In this branch, it got 0 for input and didn't uppercase. In both branches, the correct fix is to give it the actual (smaller) length which the caller already had available and not do bytesRead - readHead at all. |
prvyk
force-pushed
the
inlinecommands
branch
4 times, most recently
from
ece70a7 to
56d8454
Compare
kevin-montrose
requested changes
Jul 15, 2025
Added an EnableInlineCommands option based on the connection protection mechanism. Current default is to enable inline command parsing for local connections only (Loopback/socket/etc.) and ban it otherwise. I wanted to strike a balance between keeping it safe and making Garnet more easily accessible when client is not available. |
prvyk
force-pushed
the
inlinecommands
branch
5 times, most recently
from
2a67384 to
82248b8
Compare
|
I'd like to note that when many (normal RESP encoded) commands are sent with concurrently, as in ScatterGatherGet and GarnetObjectStoreDisabledError tests, somehow command parts get into the inline/AttemptSkipLine paths. It's something that happens in main too (put a breakpoint in RespCommand.cs:line 2778, and run the tests in Debug mode). Following this PR GarnetObjectStoreDisabledError does not exhibit this, but ScatterGatherGet yet does. I suspect it's a bug, but diagnosing it is outside scope of this PR. What's left is working around it, the 6a1e206 and 946f0c9 changes trigged this condition and I had to workaround it each time. |
kevin-montrose
approved these changes
Jul 29, 2025
prvyk
force-pushed
the
inlinecommands
branch
2 times, most recently
from
856c684 to
fa7dcb6
Compare
prvyk
force-pushed
the
inlinecommands
branch
3 times, most recently
from
d7804b0 to
524ea90
Compare
…tion mechanism - default is to allow for local connections only.
This reverts commit 21d9cac.
… reset readHead when parsing fails.
…crlf, in inline command packages.
…ed, not crlf, in inline command packages." This path can also be called for RESP code in some circumstances, and it's best to keep safe here.
…we have to work around it.
|
BDN run on main:
|
|
BDN run on inlinecommands branch:
|
|
Variance looks fine. Merge latest and get a clean set of checks and After some discussion in this week's standup, I'm going to wait for @badrishc to weigh in. LGTM however. |
prvyk
force-pushed
the
inlinecommands
branch
from
75d1ae2 to
ee9f306
Compare
prvyk
force-pushed
the
inlinecommands
branch
from
ee9f306 to
50fb9a9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR implements parsing command line supplied using Redis's inline command format.
https://redis.io/docs/latest/develop/reference/protocol-spec/#inline-commands
The inline command format allows people to connect with telnet and just write commands, without requiring other command line tools. It can be useful to avoid *-cli quoting and to see raw responses. Also a very few poorly written clients may send the inline format, so it can be useful there too.
The basic implementation strategy was to take the current code, spread it out a bit, and in the same point after seeing the buffer does not contain a RESP string, try to parse as inline command: We separate the buffer to an array of ArgSlice pointing to the original buffer. Each ArgSlice pointing to a 'word' of the original command line. The first two 'words' are printed to a temporary RESP formatted string, and then we run the regular parsing on it. If a command was matched, we initialize the parseState from the ArgSlice array. The intention is to keep the main path fast and (almost) untouched, while the inline path does the necessary work to match itself to the rest of the code. It was also preferred to not implement another command matching for this.
Examples:
The PR implements reading the format fully (as far as I can tell, given it's not very specified), with quoting and escaping, matching even rather odd reference behaviour.
(In the longer run, a strict Redis compatibility mode to enable/disable some things will be useful. e.g. disable quoting/escaping the first command word)
Another thing to note is that Garnet doesn't close the connection on protocol errors (unlike redis/valkey), so it's much nicer to type there compared to references which will close the connection whenever quotes are mismatched.