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

Rails concurrency bug on save #13234

Closed
daveroberts opened this Issue Dec 8, 2013 · 7 comments

Comments

Projects
None yet
5 participants
@daveroberts
Copy link

daveroberts commented Dec 8, 2013

There is an error with validating and saving a Rails ActiveRecord object on multi-threaded servers like Puma.

When you call .save() on an ActiveRecord model in Rails, by default, it will check to see if the object is valid, and if so, save the object.

The source for this logic is here: http://bit.ly/1eZH6zt

It looks like this:

def save(options={})
  perform_validations(options) ? super : false
end

The problem is that the call to perform_validations and super are not thread safe, and if operations are performed between their execution, the answer to perform_validations can change, leading to an incorrect database state.

It's easiest to describe this bug with an example: Imagine you have a Rails app where you add people, and the same person can't be added twice to the DB. If you try to add the same person twice, asynchronously, both calls can succeed:

# Synchronous model

p1 = Person.new(name: 'Jon', age: 10)
p1.save
# perform_validations is called on Jon, age 10, returns true
# super is called on Jon, age 10, record is saved

p2 = Person.new(name: 'Jon', age: 20)
p2.save
# perform_validations is called on Jon, age 20, returns false because DB already contains a Jon
# super is never called
# Asynchronous model

p1 = Person.new(name: 'Jon', age: 10)
p2 = Person.new(name: 'Jon', age: 20)
# call both p1.save and p2.save asynchronously
# perform_validations is called on Jon, age 10, returns true
# perform_validations is called on Jon, age 20, returns true
# super is called on Jon, age 10, record is saved
# super is called on Jon, age 20, record is saved

Since this bug involves multiple request threads, I don't know how to write a unit test to reproduce this problem, but here are the steps I used: I created a project using Rails 4.0, with a single model, Person, with two attributes: name (string) and age (int). So far, everything was generated by Rails scaffolding. The only change I made was to the the Person model to validate the uniqueness of the name attribute. I changed the database to Postgres and the web server to Puma.

Given this application, it is possible to add two people with the same name.

The key is to have the requests hit the server at the same time, so that request 1 and request 2 both validate before either record is saved. To guarantee the thread execution order, I added a sleep between the perform_validations call and the super call. I modified the ActiveRecord code above to the following:

def save(options={})
  if perform_validations(options)
    sleep 10
    super
  else
    false
  end
end

In my web app, I added Jon age 10 in one web browser tab, and then, less than ten seconds later I added Jon age 20 in another browser tab. Both requests return true for perform_validations, and both records will be saved to the database.

Since the perform_validations and write to db are both performed in the same Rails api method, .save(), I believe that its the responsibility of Rails to guarantee that the database doesn't change between perform_validations and write to database, and not that of the application programmer. I further realize that the chance for this type of error is low, but it's non-zero.

@arkiver

This comment has been minimized.

Copy link
Contributor

arkiver commented Dec 8, 2013

I recently faced the same issue. I was using Unicorn, Rails 3.11. I added validates_uniqueness_of but it let duplicate entries get created. I had to add a unique index on my db. Thanks @daveroberts

@why-el

This comment has been minimized.

Copy link
Contributor

why-el commented Dec 8, 2013

This is not a bug but documented and inherent behavior of validates_uniqueness_of.

@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Dec 8, 2013

@daveroberts this isn't a thread safety issue, more of a general concurrency issue and it's not limited to threaded servers - it can happen with any server setup. As @arkiver points out, the only way to handle this properly is at the database layer with a unique constraint on the column. What you then do is to rescue ActiveRecord::RecordNotUnique errors and handle them in a way that's suitable for your application.

@pixeltrix pixeltrix closed this Dec 8, 2013

@daveroberts

This comment has been minimized.

Copy link

daveroberts commented Dec 8, 2013

Thanks for the quick turn around guys.

A database validation will work in this case, but the problem extends beyond unique constraints and into validations that are unique to a Rails application that can't be enforced on the DB level. It appears that Rails is content to allow for optomistic concurrency control, but this still allows for invalid data to be leaked into an application.

@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Dec 9, 2013

@daveroberts with the possible exception of validates_presence_of :association and validates_associated all of the other validations are constrained by the attribute values currently in memory, so aren't susceptible to similar flaws. If you're talking about the thread safety of the perform_validations method, we don't support sharing of Active Record instances across threads.

For most web applications optimistic locking is sufficient, however pessimistic locking is available if you need it.

@csmuc

This comment has been minimized.

Copy link
Contributor

csmuc commented Mar 4, 2016

For the record, this ticket is featured in the thesis "Coordination Avoidance in Distributed Databases" from Peter Bailis: http://www.bailis.org/papers/bailis-thesis.pdf - Chapter 6.5 "Quantifying Integrity Violations in Rails"

@daveroberts

This comment has been minimized.

Copy link

daveroberts commented Mar 4, 2016

Very cool, thanks for posting!

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