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.

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

Attributes and Push Types #31

Open
kristoff-it opened this issue Nov 24, 2019 · 0 comments
Open

Attributes and Push Types #31

kristoff-it opened this issue Nov 24, 2019 · 0 comments

Comments

@kristoff-it
Copy link

I'm writing a low-level Redis client and I'm now at the stage where I'm thinking about implementing Pub/Sub support and I found an annoying-to-solve problem with regards to the push type and attributes.

Without considering attributes, since push types can be sent in-between normal replies from Redis, the parser must account for the possibility of finding something that is not the expected reply, but rather a push type. This is easy to solve as it just needs to check the type tag (> vs any of the others) and react accordingly.

Now, if we add attributes to the mix, this becomes a bit more complex.
My understanding of the RESP3 spec is that attributes are also valid for push types. If that's the case, then when the parser encounters a | it won't know if what lies ahead is a reply or a push. This is solvable by doing some look-ahead, but since this is the only case where such a thing is needed, and since I believe ease of implementation is one of the main goals, I'd recommend making some minor changes to avoid this problem.

To re-state the problem in more concrete terms, consider the following examples where the parser is being invoked by the user calling client.send(i64, "INCR", "key").

First case:

>4<CR><LF>
+pubsub<CR><LF>
+message<CR><LF>
+somechannel<CR><LF>
+this is the message<CR><LF>
:123<CR><LF>

Second case:

:123<CR><LF>
>4<CR><LF>
+pubsub<CR><LF>
+message<CR><LF>
+somechannel<CR><LF>
+this is the message<CR><LF>

As you can see in these two cases reading the first tag is enough to disambiguate. In the first case the parser will know that it needs to delegate the parsing of the first message accordingly and then retry in order to get the reply to INCR.

Third case:

|1<CR><LF>
    +channel-popularity<CR><LF>
    :100<CR><LF>
>4<CR><LF>
+pubsub<CR><LF>
+message<CR><LF>
+somechannel<CR><LF>
+this is the message<CR><LF>
:123<CR><LF>

Fourth case:

|1<CR><LF>
    +key-popularity<CR><LF>
    :5<CR><LF>
:123<CR><LF>
>4<CR><LF>
+pubsub<CR><LF>
+message<CR><LF>
+somechannel<CR><LF>
+this is the message<CR><LF>

Note how in the third and fourth case it's impossible to distinguish between the two just by looking at the first tag found in the stream. As stated earlier, this is not an insurmountable problem, but it does add an irregularity in the protocol by being the only case where reading the next type tag is not enough to parse a message correctly.

Not allowing top-level attributes for push messages, or using a different tag would be enough for ensuring we can disambiguate each case just by looking at the next byte in the stream.

Looking forward to hear some feedback about this.

Thanks,
Loris

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant