Duplicate positions and lost items

[andyw8]

We're using acts_as_list in an Ajax UI to allow users to re-order items in a list by dragging and dropping items.

If the user drags and drops a few items in quick succession, this can lead to items being lost and duplicated in the list, e.g. "Item A, Item B, Item C" becomes "Item A, Item C, Item C".

It seems the gem doesn't cope well with overlapping operations, and there is no locking. There doesn't seem to be any behaviour to protect against two items having the same position value.

I have some ideas about how to approach this problem but I wanted to get some guidance first.

Any thoughts?

[swanandp]

Hi @andyw8, which version of the gem are you using? This was definitely an issue in some older versions, but it has been fixed since then.

[andyw8]

We're on 1.15. I haven't tried 1.20 yet due to a separate dependency issue that I need to resolve first.

However, I looked through the code and latest commits and didn't see anything related to this kind of issue. (I noticed that a similar gem, acts_as_restful_list, makes uses of Rails' optimistic locking).

BTW, the lack of tags for this repo makes it difficult to tell what was added in each release.

[swanandp]

@andyw8 Lack of tags, yes. I need to fix that. Let me know if you still have this issue.

[swanandp]

@andyw8 I think this might be fixed with the latest commit. Do check.

[andyw8]

I'm actually no longer involved in the project which used this. But thanks for looking into it!

[dre-hh]

You can reproduce this easily

Given 3 Items with position from 1 to 3

i1 = Item.last 
i2 = Item.last

i1.insert_at 1
i2.insert_at 1

Will result in

  Item id: 1 pos: 3
  Item id: 2 pos: 3
  Item id: 3 pos: 1

This simulates the scenario where 2 users would open a list and both assign 1 as the position to the last item concurrently. If their request will be handled by separate rails processes (e.g on passenger) or threads (e.g on puma) this is one possible outcome

Performed sql operations 2x:

   UPDATE `items` SET position = (position + 1) WHERE (1 = 1 AND position >= 1 AND position < 3)
   BEGIN
      UPDATE `itmes` SET `position` = 1, `updated_at` = '2014-12-08 12:43:49' WHERE `catalogs`.`id` = 4
     SELECT COUNT(*) FROM `items`  WHERE (1 = 1 AND position = 1)
   COMMIT

[fschwahn]

This issue still happens - we have a frontend where several records for a model can be created at once, which results in separate queries to the create action of the respective controller. Some records have the same position when the queries overlap. This could also occur in environments were lots of users create records concurrently.

I just looked quickly at the code, and it seems there is no locking in place in add_to_list_bottom which is called on creation.

[danielross]

Hi this issue still happens.

The following test in shared_list.rb in test_insert_at would fail.

last1 = ListMixin.order(:pos).last
last2 = ListMixin.order(:pos).last
last1.insert_at(1)
last2.insert_at(1)
assert_equal [1, 2, 3, 4, 5], ListMixin.where(parent_id: 20).order('pos').map(&:pos)

The insert_at_position method would need to reload_position or something to fix it.

[brendon]

Hi @danielross, thank you for your pull request. Do you think there's scope for locking the record when reloading it in order to ensure there isn't a race condition where two processes are doing something at the same time?

[danielross]

Hi @brendon
Yes I think so and actually reload_position won't fix it properly. It is probably better to wrap the content of insert_at_position in with_lock

[brendon]

Thanks @danielross, would you be keen to add a test that simulates the race condition, then we can work on a fix :)

[brendon]

For reference: #195 is working on this issue.

[fschwahn]

This issue is still not solved for me, using version 0.8.1. This happens for me when creating records, not when updating records.

I think the problem is that this line https://github.com/swanandp/acts_as_list/blob/16cc8af5583040a54ee92257c568ee560856c7fa/lib/acts_as_list/active_record/acts/list.rb#L351 is outside of the with_lock-block. I cannot say if this is an oversight or on purpose, but it seems sensible to require a lock also when creating a record, not only on update?

[brendon]

That's a strange one :) I'm not sure how locking the new record would fix that.

In the current case, locking the record that's about to be inserted at a new position (update) should hold off other insertions that have anything to do with this record by acts_as_list (i.e. those records in the same scope). This is probably more of a beneficial side effect?

Perhaps we should be locking the whole scope before shuffling things around? I've experimented with this in the past but couldn't get it to work.

If you wouldn't mind, would you be able to create a failing test case (see the current test case for this issue as a start), then investigate locking the scope for that entire method?

[fschwahn]

To be honest I just took a quick glance at the code, and misinterpreted what with_lock is actually doing - I though it was a table level lock instead of a row level lock.

It's very easy to write a failing test, but I had to switch to postgres to do that. As simultaneous writes are AFAIK impossible in sqlite, it's not possible to trigger this behavior with it. If the test suite would support other databases, I could put together a pull request for a failing test (by the way, there are 11 test failures in test_list.rb alone with postgres - maybe worthwhile exploring).

As for a solution, there's a gem for table level locking (https://github.com/norman/fatalistic, it's actually very little code), and I could make my failing test green by updating the callback as follows:

if add_new_at.present?
  before_create do
    self.class.lock do
      send("add_to_list_#{add_new_at}".to_sym)
    end
  end
end

However, I have no idea what impact this has on performance, portability, etc. (Besides, when including this gem a LOT of tests start failing in the test suite, probably because it's overwriting the existing ActiveRecord-method lock instead of defining a new one.).

Maybe this is something which helps people out facing the same problem.

[brendon]

Thanks @fschwahn, that's some very interesting info! Mildly depressing that the suite is failing a lot in postgres! I wonder what it's like in mysql?

Right now it's so simple to get a test environment up and running. Adding those dependencies adds a bit of complexity but it's probably worth it.

As a start, would you like to see if you can get a test suite up and running that tests against sqlite, mysql, and postgres? I've found some notes here:

http://madebynathan.com/2011/12/13/testing-multiple-databases-for-a-rails-app-on-travis-ci/
https://github.com/chiliproject/chiliproject/blob/master/.travis.yml#L7

See the second link (mentioned at the bottom of the first link) as I think they found an even simpler way to do it.

Let me know if I need to modify travis at all, though @swanandp may need to do that mod as I don't think I have full access to the travis setup.

---

Regarding the locking, I think locking the whole table is a bit dangerous given this code is used amongst larger codebases that we don't have any idea about. I think it's probably safer to lock the records that belong to the scope that the record belongs to. In the case of a scope change, either first lock the old scope while the removal takes place, then lock the new scope while the insertion takes place. We could probably add some DB specific locking types that lock for write but not for read also.

In my experimenting, calling .with_lock on a AR scope didn't lock the scope (Actually did nothing). Though I could have been using it wrong. I hope we don't have to loop all the records to lock them...

[fschwahn]

I'll have a look at the travis setup, but I had a brief look at the test failures, and all the failures are because they test database specific details (default order, order of NULLs). These tests need to be adjusted, but there was nothing dramatic.

[brendon]

Oh ok, that's good then. Still, probably a good idea that we're testing against the databases that people actually use :)

[fschwahn]

And about the locking issue:
It's possible to just lock the relevant scope with something like acts_as_list_class.lock.where(scope_condition).to_a (with_lock only operates on individual models) - I briefly tested it and it almost works:
It does not work when several records are inserted when there are no other records yet (ie. on position 1), because there are no rows to lock yet. I fear in that case you need a table level lock - I could be wrong though, I'm far from an expert.

[swanandp]

About Travis, sure, just me know.

[brendon]

@fschwahn, that's some fine sleuthing :) If there are no records yet in that scope then there's no need for a lock because there's nothing to effect unless you're worried about multiple threads adding to the same empty scope. In that case there'd need to be something lower level. Perhaps it's possible to lock that scope as SQL directly (bypassing the Rails stuff). Then I'd assume you could lock the empty scope? Or perhaps I'm thinking of a feature that doesn't actually exist :D

@swanandp, are you able to expand or add me as a travis admin on the project?

[prosanelli]

I am seeing duplicate positions in 0.8.2 and Rails 5.0.0.1.

[brendon]

Hi @prosanelli, are you able to give a bit more information? Under what circumstances is this happening?

[prosanelli]

I am creating records using sidekiq jobs.

[brendon]

So it's probably a locking issue? Is it due to no locking on an empty scope? (i.e. a list with no items yet)