save (update) operation on repository deletes the key and inserts it back [DATAREDIS-560]

[spring-projects-issues]

sumanth yamala opened DATAREDIS-560 and commented

When using the KeyValueRepository interface and enableRedisRepositories:

After the initial creation - repository.save() - everything is fine here.
When doing an update operation: calling repository.save() - correctly detects that this is an update and calls
SimpleKeyValueRepository().save() --> KeyValueTemplate.update(....) --> which calls the callback method:
doInRedis(RedisConnection connection ).. In this method I see an issue:

Issue: A call is being made to delete the key
boolean isNew = connection.del(objectKey) == 0;
and it is promptly added back in the same callback method.

During this period if another thread is looking up the key - it indicates the key is not there and shortly afterwards it reappears.

Two issues here:

1. Integrity of redis is not maintained - a key is being deleted in the process of an update.
2. In the event of a crash around this critical section I believe the integrity of the data is getting compromised.

How to reproduce this:
Do an update operation on the repository in a debugger and suspend the thread after the delete operation. Use another client to look up the key.

Should the connection.del(objectKey) be changed to connection.exisits(objectKey) to fix this.

---

Affects: 1.7.4 (Hopper SR4)

Issue Links:

• DATAREDIS-741 Improve repository operation documentation

[spring-projects-issues]

Christoph Strobl commented

We intermedeatly remove the hash to make sure there's no leftovers from values previously set which is what would happen if we just use HMSET.

HMSET key-1 field_1 value_1 field_2 value_2
HMSET key-1 field_1 update_1 field_3 update_3
HGETALL key-1
  1) "field_1"
  2) "update_1"
  3) "field_2"  // that one should not be there any more
  4) "value_2" 
  5) "field_3"
  6) "update_3"

Intersecting existing values with new ones to add increases traffic and load on both the server and client. So I'm not convinced that's the way to go. It might be possible to wrap those calls in MULTI/EXEC, but I need to check how that works out in cluster with both the connection handling and slot assignment.

You can use PartialUpdate to change/add/remove stuff from existing hashes

[spring-projects-issues]

sumanth yamala commented

Thanks for the update. Any example code for using PartialUpdate

[spring-projects-issues]

sumanth yamala commented

Was unable to use PartialUpdate ( as I am on 1.7.4). I had a work around by implementing a RedisCallback and doing an update on a a subset of hash keys

[spring-projects-issues]

Marian Seitner commented

I also stumbled across this issue. I understand the motivation behind DEL followed by HMSET, but IMHO save() should really just map to HMSET. The "leftovers" problem can only occur after a structural change of an entity and it should be left to the user's discretion how to deal with it. This behaviour should at least be documented and ideally somehow configurable, e.g. per repository

[spring-projects-issues]

Christoph Strobl commented

Marian Seitner point taken. I agree that the documentation should be improved and state more clearly which commands are triggered. I've created DATAREDIS-741 to tackle that.

Also documentation of PartialUpdate should be polished up and at least mention the option of using the domain object itself as source for an update. This creates the hash out of it and just uses HMSET to store it

[spring-projects-issues]

Josef Reichardt commented

I've recently stumbled upon this issue. It took me some time to figure out what the problem was until I finally found the spot.

Do I understand correctly that a repository is not currently capable of reading data in a REST endpoint that is also written at runtime (for example, in another REST endpoint)?

Is the only "solution" to use a RedisTemplate instead of a repository? Or have I misunderstood this ticket here?

To make sure that my application doesn't cause the error, I created a demo application that includes unit tests to reproduce the error: https://github.com/Josef-Reichardt/redis-concurrent-read-write-error

Maybe this will help so that the bug can be fixed sometime, even if this ticket is a bit older?