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

Fix errors with reload lua scripts #124

Merged
merged 2 commits into from
Nov 9, 2020
Merged

Fix errors with reload lua scripts #124

merged 2 commits into from
Nov 9, 2020

Conversation

novln
Copy link
Contributor

@novln novln commented Nov 6, 2020

This refactoring started when I had an issue testing if the reloading was safe for concurrent use, using SCRIPT FLUSH command on a Redis instance.

Reusing the benchmark provided and adding the flag -race with multiple goroutines, I've detected that there was several issues.

The first one was with the second execution of EvalSha, the args... were missing, producing an error on the Lua increment script because there was no argument given.

cmd = store.client.EvalSha(ctx, sha, keys)

The second one was with an data race detected because multiple goroutines could read (without protection) when one could write to modify store.luaIncrSHA. Better safe than sorry, I've added a method to ensure that there is no data race, even if the impact is pretty minimal.

store.luaIncrSHA = incrLuaSHA

cmd := store.evalSHA(ctx, store.luaIncrSHA, []string{key}, 1, rate.Period.Milliseconds())

One thing led to another and I was down the rabbit hole so, I've done some refactoring.

@git-hulk Feel free to take a look.

@git-hulk
Copy link
Contributor

git-hulk commented Nov 7, 2020

it looks good to me

@novln novln merged commit b298ea8 into master Nov 9, 2020
@novln novln deleted the fix/redis-script-flush branch November 9, 2020 10:43
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.

2 participants