Create item - key racecondition

[ghost]

Hi,

If I'm right, currently there is a slight race condition when the record is created. Consider this model

class Account
   include RedisModelExtension

  redis_field :email, :string
  redis_key :id, [:email]
end

Before the model is saved there is a test for existence of key in redis. The code reads like this

raise Error if redis.exists key
redis.hmset key args

But in theory before the item is actually saved, the key can be set from another connection. IMHO it should look like this instead

redis.watch key
if redis.exists key
  redis.unwatch key
  raise Error
end
redis.multi do
  redis.hmset key args
end

What do you think?

[ghost]

Which reminds me, that there is a simillar problem with aliases. Consider this scenarion

• 1. model is beeing destroyed, first call is DEL key
• 2a. second model is being created, first call is EXISTS key (returns 0)
• 2b. stores the set HMSET key ...
• 2c. creates aliases with SADD and happily overwrites the reference key to the first model
• 3. now the first model gets to the point where he deletes all aliasses with SREM

The result is that second model is saved, but without aliases. The soulution for this would probably be much more simple. Putting destroy in transaction

redis.multi do
  redis.del key
  delete_aliases
end

[ondrejbartas]

Ok,

first thing for save:

Redis works with atomic operations - there is only one operation in one time.

When you are using autoincrement ID - it is getted by incr method of redis it will provide getting uniq id. Test of existence of redis key before saving it is only for renaming redis key i.e. : you have address in key - you change address - redis model will rename old key to new address

and second one problem with aliases:

You are saying that you are deleting item with same redis key? If this is case - then you are right We should add multi

[ghost]

Autoincrement ids are fine. INCR will make sure that id is unique. But the example is not using that. I did mistake with that exists clause though. So the problem is actually even more simplier... When using custom redis_key there is potential for key collision, there is no check.

[ondrejbartas]

When you are using custom redis key you have to know what you are doing and why you are using it. I think in Readme there should be (was) information about redis key uniq need.

And adding some check is not good idea, user can check before by exists? if he is adding new item or updating already existing item

[ondrejbartas]

SORRY FOR DELETING THIS POST MIHU:

Make sense. But there isn't much the user can do, when choosing self generated key. It can create a little bit of mess with aliases too. Consider when two models have same redis_key but different value for alias. The result will be one model saved and two aliases pointing at it, one of them pointing incorrectly.

I have a troubble to figuring out a scenario in which non-unique redis_key would make sense. So for me makes sense, that default behaviour should check for key duplicity (when creating, not when updating).

[ondrejbartas]

redis key is generated from "#{self.name.to_s.underscore.to_sym}:#{key}" + "values of your redis key configuration"

this provides that every model (class) will have its own namespace for storing its items.

[ondrejbartas]

For your updating/creating

you are able to decide (update/create) only for item which was founded/getted by methods find & get. this model know if is saved in redis or not.

Question: what to do when creating and there is already item with same key?

[ghost]

ok... code, is code...

class Article
  include RedisModelExtension
  redis_field :email
  redis_field :login

  redis_key :email, [:email]
  redis_alias :login, [:login]
end

a1 = Article.new(email: 'foo@bar.baz', login: 'foo')
Thread.new do
  a1.save
end

a2 = Article.new(email: 'foo@bar.baz', login: 'bar')
Thread.new do
  a2.save
end

# at this point, there is now way to tell, in which order
# the redis commands will run

So in theory, this could lead to one model saved with two aliases, but one of them stored incorrectly.

[ondrejbartas]

I dont understand why are you using : redis_key :email, [:email]

redis key takes array of field_names

you should write only: redis_key :email or with more fields: redis_key :email, :login

[ondrejbartas]

Ok about theory.

if we put whole save operation (including deletion and creation of aliases) into redis multi. Then there should be saved only one alias and only one item.

You will not be able to tell which one of a1 or a2 it was but I think this will be left to programer to decide what is right or wrong

[ondrejbartas]

BTW

redis_key redis_alias :alias_name,

[ghost]

add redis_key - yeah, that was my mistake for redis_alias. Gosh, I'm loosing it.

IMHO the only reasonable thing what to do when there is collision is raise exception (ActiveRecord does that). There is now way to make it handle itself, if that happens.

In theory, there is no need to put creation of aliases in multi because only one of the concurents will reach that point.

Code could be something like this

if new_record?
  redis.watch key
  if redis.exists key
    redis.unwatch key
    raise Error
  end
  retval = redis.multi do
    redis.hmset key args
  end
  raise Error if retval.nil?
else
   redis.hmset key args
end

create_aliases

[ondrejbartas]

Ok, maybe I am lost.

where is method new_record? or where it decides its new_record?

[ghost]

Sorry for the mistification:o) I've just assumed that it behaves like ActiveRecord. It does that by storing true to instance variable when model is created by new method and storing false when that model is saved. The new_record? is only reader of that variable. I see that in save_destroy.rb is something simillar

       run_callbacks :save do
         unless exists?
           run_callbacks :create do

Which might and might not represent the same thing.