Skip to content

Make optimistic version locking optional #856

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
+804 −45
Draft

Make optimistic version locking optional #856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a9b870
make ver optional
sgtsquiggs Jun 10, 2025
0b51819
revert unnecessary changes
sgtsquiggs Jun 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 39 additions & 14 deletions om/hash.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,19 @@ func (r *HashRepository[T]) FetchCache(ctx context.Context, id string, ttl time.
return v, err
}

func (r *HashRepository[T]) toExec(entity *T) (val reflect.Value, exec rueidis.LuaExec) {
val = reflect.ValueOf(entity).Elem()
func (r *HashRepository[T]) toExec(entity *T) (verf reflect.Value, exec rueidis.LuaExec) {
val := reflect.ValueOf(entity).Elem()
if !r.schema.verless {
verf = val.Field(r.schema.ver.idx)
} else {
verf = reflect.ValueOf(int64(0)) // verless, set verf to a dummy value
}
fields := r.factory.NewConverter(val).ToHash()
keyVal := fields[r.schema.key.name]
verVal := fields[r.schema.ver.name]
var verVal string
if !r.schema.verless {
verVal = fields[r.schema.ver.name]
}
extVal := int64(0)
if r.schema.ext != nil {
if ext, ok := val.Field(r.schema.ext.idx).Interface().(time.Time); ok && !ext.IsZero() {
Expand All @@ -98,34 +106,42 @@ func (r *HashRepository[T]) toExec(entity *T) (val reflect.Value, exec rueidis.L
// Save the entity under the redis key of `{prefix}:{id}`.
// It also uses the `redis:",ver"` field and lua script to perform optimistic locking and prevent lost update.
func (r *HashRepository[T]) Save(ctx context.Context, entity *T) (err error) {
val, exec := r.toExec(entity)
verf, exec := r.toExec(entity)
str, err := hashSaveScript.Exec(ctx, r.client, exec.Keys, exec.Args).ToString()
if rueidis.IsRedisNil(err) {
if r.schema.verless {
return nil
}
Comment on lines +112 to +114
Copy link
Collaborator

@rueian rueian Oct 20, 2025

Choose a reason for hiding this comment

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

When will this happen? If it never happens, can we just delete it?

Suggested change
if r.schema.verless {
return nil
}

Copy link
Collaborator

@rueian rueian Oct 20, 2025

Choose a reason for hiding this comment

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

On the other hand, if the case does happen, we should not return nil either.

Copy link
Author

@sgtsquiggs sgtsquiggs Oct 20, 2025

Choose a reason for hiding this comment

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

I modified the script (here) to return nil if the ver arg is not present. Alternatively I could have used another script that does not perform optimistic locking, and forked based on r.schema.verless.

Copy link
Collaborator

@rueian rueian Oct 20, 2025

Choose a reason for hiding this comment

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

Oh, can we align with the original behavior to return ARGV[2] as well?

Copy link
Author

@sgtsquiggs sgtsquiggs Oct 20, 2025

Choose a reason for hiding this comment

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

Absolutely. Wrote this PR quickly so I could fork and use om - looking at this code today I don't remember a lot of it and it looks a little dodgy to me. I'll try to reimplement a bit cleaner this week.

return ErrVersionMismatch
}
if err == nil {
} else if err == nil {
ver, _ := strconv.ParseInt(str, 10, 64)
val.Field(r.schema.ver.idx).SetInt(ver)
verf.SetInt(ver)
}
return err
}

// SaveMulti batches multiple HashRepository.Save at once
func (r *HashRepository[T]) SaveMulti(ctx context.Context, entities ...*T) []error {
errs := make([]error, len(entities))
vals := make([]reflect.Value, len(entities))
verf := make([]reflect.Value, len(entities))
exec := make([]rueidis.LuaExec, len(entities))
for i, entity := range entities {
vals[i], exec[i] = r.toExec(entity)
verf[i], exec[i] = r.toExec(entity)
}
for i, resp := range hashSaveScript.ExecMulti(ctx, r.client, exec...) {
if str, err := resp.ToString(); err != nil {
if errs[i] = err; rueidis.IsRedisNil(err) {
errs[i] = ErrVersionMismatch
str, err := resp.ToString()
if rueidis.IsRedisNil(err) {
if r.schema.verless {
continue
Comment on lines +134 to +135
Copy link
Collaborator

@rueian rueian Oct 20, 2025

Choose a reason for hiding this comment

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

Same here. Can we delete this?

}
} else {
errs[i] = ErrVersionMismatch
continue
}
if err == nil {
ver, _ := strconv.ParseInt(str, 10, 64)
vals[i].Field(r.schema.ver.idx).SetInt(ver)
verf[i].SetInt(ver)
} else {
errs[i] = err
}
}
return errs
Expand Down Expand Up @@ -274,6 +290,15 @@ func (r *HashRepository[T]) fromFields(fields map[string]string) (*T, error) {
}

var hashSaveScript = rueidis.NewLuaScript(`
if (ARGV[1] == '')
then
local e = (#ARGV % 2 == 1) and table.remove(ARGV) or nil
if redis.call('HSET',KEYS[1],unpack(ARGV))
then
if e then redis.call('PEXPIREAT',KEYS[1],e) end
end
return nil
end
local v = redis.call('HGET',KEYS[1],ARGV[1])
if (not v or v == ARGV[2])
then
Expand Down
Loading