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

Changed parse_call_reply to return a BulkString. #93

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MikkelSnitker
Copy link

@MikkelSnitker MikkelSnitker commented Jul 9, 2020

This PR changes the parse_call_reply function to return a BulkString instead of af SimpleString.

The current implementation treats all string values, returned by calling a redis function within a module, as a SimpleString. This issue arises if you pass the the return value of call from your module.

The change i propose will treat string values returned from call as a BulkString, as the underlying string can contain line brakes

This PR changes the parse_call_reply function to return a BulkString instead of af  SimpleString. 

The current implementation treats all string values, returned by calling a redis function within a module, as a SimpleString. This issue arises if you pass the the return value of call from your module.

The change i propose will treat string values returned from call as a BulkString as BulkStrings can contain line breaks, as the underlying string can contain line brakes
@yoav-steinberg
Copy link
Contributor

yoav-steinberg commented Jul 13, 2020

Another option would be to remove the distinction between Bulk/Simple strings from the RedisValue enum. This might be more in line with how the module API works (there's no such distinction).
Then all we'll need to do is change the implementation in Context.reply() so it decides how to handle the string value based on how many lines there are in it. I think this is what the original module design was aiming for.

We can further extent this to have the RedisValue::String(s) variant also include a boolean called force_bulk just so we can force a bulk reply in cases where we explicitly want to make sure our module's command returns a bulk (if there are such cases).

Copy link
Contributor

@yoav-steinberg yoav-steinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this change, see comment above.

@phartleyp
Copy link
Contributor

phartleyp commented May 7, 2022

@yoav-steinberg I was about to re-submit this exact change before noticing that it had previously been submitted.

I spent way too long troubleshooting code that was affected by this issue. For example, this code looks harmless but contains a bug:

/// Redis module command that returns the value of at key `key`
pub fn cmd_example(ctx: &Context, _args: Vec<RedisString>) -> RedisResult {
    ctx.call("GET", &["key"])
}

This code works fine if the key's value does not contain \r\n (i.e. it is a SimpleString). However, if the key's value contains \r\n (i.e. it should be a BulkString), redismodule-rs still maps it in to a SimpleString. This causes the Redis client (redis-py in my case) to hang because RESP protocol is not respected.

It seems that this pull request is delayed because there is desire to make Context::call() more intelligent about the RedisValue enum that it returns. But until that happens, using BulkString instead of SimpleString would still be a safer choice.

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

Successfully merging this pull request may close these issues.

3 participants