Positions are repeating when creating items concurrently

[ProGM]

I'm using Rails 5.0.1 and acts_as_list v0.8.2 and postgresql.

When creating items in a list concurrently (like in concurrent xhr requests), positions gets repeating.

I tried to replicate in console:

class Paragraph < ApplicationRecord
  belongs_to :article
  acts_as_list scope: :article
end

class Article < ApplicationRecord
  has_many :paragraphs
end

a = Article.create!
p = Array.new(5) { Paragraph.new(article: a) }
t = p.map { |e| Thread.new { e.save! } }
t.map(&:join)
a.reload.pluck(:position) # => [1, 1, 1, 1, 1]

I've temporarily set an unique index for [:article_id, :position] to fail when this happens, but it's really a big problem for me.

[swanandp]

Hmm, this does seem to be a problem, and with threads.

Here is an example app I created, no UI, just specs to recreate the issue: https://github.com/swanandp/aal-position-test-example-app/commits/master

There is a failing test, with threads, and a passing test without threads.

[swanandp]

Have a look at the commits, and try the tests out.

[swanandp]

Since this is a threading and race condition issue, this can be 100% solved by using a lock on article.

https://github.com/swanandp/aal-position-test-example-app/blob/master/spec/models/paragraph_spec.rb#L47

[brendon]

Good investigation @swanandp :) Is there anything we need to do in acts_as_list to help?

[swanandp]

I don't think so, but let's wait to hear back from @ProGM before closing this out.

[ProGM]

@brendon @swanandp I confirm that using a lock on the article solves the issue. 👍

I'm not sure of it, but... shouldn't this be something that the gem could manage itself? I mean, it's pretty common to add elements to a list in a concurrent environment (in my case, I'm using the puma web server, that manages requests using threads).

Otherwise, if this is not something that the gem deals with, you should put a few line about this in the docs or the README, I think :)

[brendon]

I'm out of my depth on that one. @swanandp, what are your thoughts here? We could only lock the scope parent, but sometimes the scope isn't that simple (multiple parents etc...) so I don't think this would be an easy solution.

[swanandp]

I agree @brendon, locking should be left to the application layer, not something acts_as_list should do on its own. Because the context may differ for each app, and multiple parents would make it difficult to choose a pivot for locking.

But it won't hurt to add a few guidelines in the README about this.

@ProGM do you want to take a shot at this? I can edit, correct if needed.

[zharikovpro]

Also, you can add table constraints to raise exception in case of attempt to insert duplicate values. It will act like safety net. And then you will need proper locking implementation to never trigger this, for sure.

[swanandp]

I second that. Always, always use the DB ensure data integrity!

[seanwash]

This thread has been helpful to me as well! I'm working on safeguarding things now, but I have run into this issue where indexes are duplicated and the only way to fix things is to reset them all.

[stepheneb]

This thread was very helpful. Just had this issue with a model with multiple parents -- so there is no obvious way acts_as_list could have prevented problem.

Problem occurred when creating multiple objects as a result of a drag and drop files as part of a bulk import feature invocation.

I solved it by creating list objects in a with_lock block, thanks @swanandp for the tip.

Am using Postgresql. Would be interested in ideas about how to add a constraint disallowing duplicates with a scope to belongs_to model.

[zharikovpro]

@stepheneb you can create compound unique index:

CREATE UNIQUE INDEX position_on_parent_idx ON items (parent_id, position)

[swanandp]

CREATE UNIQUE INDEX position_on_parent_idx ON items (parent_id, position)

The only change I'd suggest over this is to use CONSTRAINTS,

ALTER TABLE items ADD CONSTRAINT INDEX unique_positions_per_parent UNIQUE (parent_id, position)

This allows you to utilise features like on conflict ... update ..., popularly known as "upsert"

[brendon]

Is there anything we can do to solve this issue or should we just close this one?

[Silex]

Just wanted to mention https://github.com/ClosureTree/with_advisory_lock ("best" solution from http://benjit.com/rails/activerecord/2015/04/03/uniqueness-constraint-the-expected-exception)

[brendon]

@Silex, I've had a quick look at that. Looks interesting. Is there anything there that we could apply to the gem itself?

[Silex]

@brendon: yes/no/maybe :smile:

It's basically a mutex implemented by the database, so the data integrity comes with a performance price.

In my case I value data integrity more than performance but some people might disagree.

For this gem, the data update/insertion could be wrapped with something like:

instance.class.with_advisory_lock(format('lock-%s-%s', instance.class.to_s, acts_as_list_scope_value)) do
  update_or_insert()
end

where acts_as_list_scope_value would be the current instance scope that needs locking (e.g the value of parent_id, or nil when there is no scope).

Probably that the current use of with_lock could be removed if we switch to that solution.

EDIT: PR for discussion at #325

[vishnun]

I'm trying to check if my rails app is thread safe or not and I came across this just now. I'm using 0.9.15 version and wanted to know if it still is thread UN-safe? I see that with_lock is used in the gem while inserting and nothing is mentioned in the Readme about adding anything else. So, just want to confirm about thread-safety.

[Silex]

@vishnun: it's thread safe in the sense it won't crash on you, but it's not thread safe in the sense you'll get invalid positions on concurrent inserts, etc.

You can work around this by using a database mutex (see my other post/PR), or you can also enforce constraints on the DB and catch the exception and "retry" or whatever you feel appropriate.