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

Keystore.put uses database FOR UPDATE without a transaction #1019

Open
project-concerto opened this issue Aug 31, 2021 · 1 comment
Open

Comments

@project-concerto
Copy link

Keystore.put calls find_or_create_key_for_update, which in turns use self.lock(true) that issues a FOR UPDATE SQL query. FOR UPDATE is only useful in transactions, but Keystore.put does not use any transactions. Thus, the put operation loses its atomicity.

This only happens when the database being used is neither MySQL or SQLite. But I think it is still an issue worth fixing.

def self.put(key, value)
validate_input_key(key)
if Keystore.connection.adapter_name == "SQLite"
Keystore.connection.execute("INSERT OR REPLACE INTO " <<
"#{Keystore.table_name} (`key`, `value`) VALUES " <<
"(#{q(key)}, #{q(value)})")
elsif Keystore.connection.adapter_name =~ /Mysql/
Keystore.connection.execute("INSERT INTO #{Keystore.table_name} (" +
"`key`, `value`) VALUES (#{q(key)}, #{q(value)}) ON DUPLICATE KEY " +
"UPDATE `value` = #{q(value)}")
else
kv = self.find_or_create_key_for_update(key, value)
kv.value = value
kv.save!
end
true
end

def self.find_or_create_key_for_update(key, init = nil)
loop do
found = self.lock(true).find_by(:key => key)
return found if found
begin
self.create! do |kv|
kv.key = key
kv.value = init
kv.save!
end
rescue ActiveRecord::RecordNotUnique
nil
end
end
end

@pushcx
Copy link
Member

pushcx commented Aug 31, 2021

Thanks for recognizing and pointing it out, seems worth an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants