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.

Sign up for GitHub

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

Jump to bottom

Add Script::load function #603

Merged
merged 4 commits into from Jun 9, 2022
Merged

Add Script::load function #603

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb3643d
Unify Script::invoke function
zhiburt May 5, 2022
a02aad0
Add Script::load method
zhiburt May 5, 2022
fb861af
Add tests for Script::load
zhiburt May 5, 2022
50a48b3
Refactoring Script functions; move cmd build to its own functions
zhiburt May 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 23 additions & 22 deletions src/script.rs
View file
Open in desktop
Expand Up @@ -124,26 +124,26 @@ impl<'a> ScriptInvocation<'a> {
/// Invokes the script and returns the result.
#[inline]
pub fn invoke<T: FromRedisValue>(&self, con: &mut dyn ConnectionLike) -> RedisResult<T> {
loop {
match cmd("EVALSHA")
.arg(self.script.hash.as_bytes())
.arg(self.keys.len())
.arg(&*self.keys)
.arg(&*self.args)
.query(con)
{
Ok(val) => {
return Ok(val);
}
Err(err) => {
if err.kind() == ErrorKind::NoScriptError {
cmd("SCRIPT")
.arg("LOAD")
.arg(self.script.code.as_bytes())
.query(con)?;
} else {
fail!(err);
}
let mut eval_cmd = cmd("EVALSHA");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you think we no longer need the loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you may see that a sync and async implementation is different.
I think that the async implementation with 1 retry is better. So I replace a loop implementation with it.

The loop may cause a number of retries or even "potentially" an infinite loop.
See when there's no script LOADed we load it, but then if someone FLUSHes redis EVAL will be an error an we will try to LOAD the script one more time.

Also it's beneficial because the unification of the code.

eval_cmd
.arg(self.script.hash.as_bytes())
.arg(self.keys.len())
.arg(&*self.keys)
.arg(&*self.args);

match eval_cmd.query(con) {
Ok(val) => Ok(val),
Err(err) => {
if err.kind() == ErrorKind::NoScriptError {
let mut load_cmd = cmd("SCRIPT");
load_cmd
.arg("LOAD")
.arg(self.script.code.as_bytes())
.query(con)?;

eval_cmd.query(con)
} else {
Err(err)
}
}
}
Expand All @@ -164,8 +164,6 @@ impl<'a> ScriptInvocation<'a> {
.arg(&*self.keys)
.arg(&*self.args);

let mut load_cmd = cmd("SCRIPT");
load_cmd.arg("LOAD").arg(self.script.code.as_bytes());
match eval_cmd.query_async(con).await {
Ok(val) => {
// Return the value from the script evaluation
Expand All @@ -174,6 +172,9 @@ impl<'a> ScriptInvocation<'a> {
Err(err) => {
// Load the script into Redis if the script hash wasn't there already
if err.kind() == ErrorKind::NoScriptError {
let mut load_cmd = cmd("SCRIPT");
load_cmd.arg("LOAD").arg(self.script.code.as_bytes());

load_cmd.query_async(con).await?;
eval_cmd.query_async(con).await
} else {
Expand Down