-
Notifications
You must be signed in to change notification settings - Fork 153
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
Race conditions #184
Comments
Setting |
I'm battling the same issues as well here. One solution is to have a "deferred unique together" constraint, which isn't supported by Django, but can be run in a custom postgres migration like this: class Migration(migrations.Migration):
dependencies = [
('orders', '0003_auto_20190115_1322'),
]
operations = [
migrations.RunSQL(
f"ALTER TABLE orders_truckplacement ADD CONSTRAINT position_date_truck_unique_together UNIQUE (execution_date, position, truck_id) DEFERRABLE INITIALLY DEFERRED",
reverse_sql = "ALTER TABLE orders_truckplacement DROP CONSTRAINT position_date_truck_unique_together"
)
] However, I'm still able to provoke deadlocks with |
Alright, so here's what I've found out so far. Test case # In my project I use multiple schemas, there's some extra code in there for that in the actual test-case, but I think the same principle applies.
class Test(APITransactionTestCase):
def test_deadlock_simple_case(self):
today = timezone.now().date().isoformat()
truck = mommy.make(Truck)
# Create some orders here. The orders aren't particularly relevant, but what's
# relevant is that each of them has a "Placement" which is an OrdredModel
# that's ordered with regards to the fields "execution_date" and "truck"
# - the 'position is the order field'
existing_orders = [
mommy.make_recipe('orders.call_order', placement__execution_date=today, placement__position=i,
placement__truck=truck) for i in range(0, 5)]
# Here we randomly take some existing_orders and switch them around. This will provide a deadlock.
# This decorator runs the function is several threads. The implementation is not important but it's here: https://gist.github.com/GeeWee/bc5da4f982af4c221aa1d0b8a1a3944e
@run_concurrently(5, existing_orders)
def switch_position_around(order):
with transaction.atomic():
position = random.randint(0, 5)
order.placement.to(position)
connection.close()
switch_position_around() Error message
I've logged the SQL and it looks like this (some things omitted) [555] LOG: statement: BEGIN
[553] LOG: statement: UPDATE "orders_truckplacement" SET "position" = ("orders_truckplacement"."position" + 1) WHERE ("orders_truckplacement"."execution_date" = '2019-02-22'::date AND "orders_truckplacement"."truck_id" = 1 AND "orders_truckplacement"."position" >= 0 AND "orders_truckplacement"."position" < 1)
[556] LOG: statement: BEGIN
[552] LOG: statement: UPDATE "orders_truckplacement" SET "position" = ("orders_truckplacement"."position" - 1) WHERE ("orders_truckplacement"."execution_date" = '2019-02-22'::date AND "orders_truckplacement"."truck_id" = 1 AND "orders_truckplacement"."position" > 0 AND "orders_truckplacement"."position" <= 5)
[555] LOG: statement: UPDATE "orders_truckplacement" SET "position" = ("orders_truckplacement"."position" + 1) WHERE ("orders_truckplacement"."execution_date" = '2019-02-22'::date AND "orders_truckplacement"."truck_id" = 1 AND "orders_truckplacement"."position" >= 2 AND "orders_truckplacement"."position" < 3)
[556] LOG: statement: UPDATE "orders_truckplacement" SET "position" = ("orders_truckplacement"."position" + 1) WHERE ("orders_truckplacement"."execution_date" = '2019-02-22'::date AND "orders_truckplacement"."truck_id" = 1 AND "orders_truckplacement"."position" >= 2 AND "orders_truckplacement"."position" < 4)
[553] LOG: statement: UPDATE "orders_truckplacement" SET "created" = '2019-02-22T12:18:25.334839+00:00'::timestamptz, "modified" = '2019-02-22T12:18:25.502266+00:00'::timestamptz, "call_order_id" = 2, "automatic_order_id" = NULL, "archived_order_id" = NULL, "execution_date" = '2019-02-22'::date, "truck_id" = 1, "position" = 0 WHERE "orders_truckplacement"."id" = 2
[552] LOG: statement: UPDATE "orders_truckplacement" SET "created" = '2019-02-22T12:18:25.303821+00:00'::timestamptz, "modified" = '2019-02-22T12:18:25.502939+00:00'::timestamptz, "call_order_id" = 1, "automatic_order_id" = NULL, "archived_order_id" = NULL, "execution_date" = '2019-02-22'::date, "truck_id" = 1, "position" = 5 WHERE "orders_truckplacement"."id" = 1
[553] ERROR: deadlock detected
[553] DETAIL: Process 553 waits for ShareLock on transaction 291529; blocked by process 552.
Process 552 waits for ShareLock on transaction 291528; blocked by process 553.
Process 553: UPDATE "orders_truckplacement" SET "created" = '2019-02-22T12:18:25.334839+00:00'::timestamptz, "modified" = '2019-02-22T12:18:25.502266+00:00'::timestamptz, "call_order_id" = 2, "automatic_order_id" = NULL, "archived_order_id" = NULL, "execution_date" = '2019-02-22'::date, "truck_id" = 1, "position" = 0 WHERE "orders_truckplacement"."id" = 2
Process 552: UPDATE "orders_truckplacement" SET "created" = '2019-02-22T12:18:25.303821+00:00'::timestamptz, "modified" = '2019-02-22T12:18:25.502939+00:00'::timestamptz, "call_order_id" = 1, "automatic_order_id" = NULL, "archived_order_id" = NULL, "execution_date" = '2019-02-22'::date, "truck_id" = 1, "position" = 5 WHERE "orders_truckplacement"."id" = 1
[553] HINT: See server log for query details.
[553] CONTEXT: while updating tuple (0,2) in relation "orders_truckplacement"
[553] STATEMENT: UPDATE "orders_truckplacement" SET "created" = '2019-02-22T12:18:25.334839+00:00'::timestamptz, "modified" = '2019-02-22T12:18:25.502266+00:00'::timestamptz, "call_order_id" = 2, "automatic_order_id" = NULL, "archived_order_id" = NULL, "execution_date" = '2019-02-22'::date, "truck_id" = 1, "position" = 0 WHERE "orders_truckplacement"."id" = 2 I've also been able to provoke the following deadlock:
It seems the issue is that django-ordered-model doesn't lock the table properly for updates. I think the correct way to do it is to call SELECT FOR UPDATE with a deterministic order. Preferably we'd do it in a subquery like this/this Now we'd obviously prefer not to dive into raw SQL as we'll lose database portability. Django comes with Calling select_for_update and forcing an evaluation of the queryset fixes the deadlock issues: # New implementation of get_ordering_queryset
def get_ordering_queryset(self, qs=None):
qs = qs or self._get_class_for_ordering_queryset().objects.all()
order_with_respect_to = self.order_with_respect_to
if order_with_respect_to:
order_values = self._get_order_with_respect_to()
qs = qs.filter(**dict(order_values))
#### BELOW HERE IS NEW
qs = qs.select_for_update()
len(qs) # Force evaluation of qs
return qs However we still do end up with potential race-conditions as I'm able to get some conflicting position keys, but I'm not quite sure why. I can reproduce it with the same test, but it takes much longer. |
I've managed to reproduce it. I've produced a more detailed output below, but the gist is this: We try to move two things around at once. Order 1 and 2. qs.filter(**{self.order_field_name + '__lt': getattr(self, self.order_field_name),
self.order_field_name + '__gte': order})\ It does so by using the "stale" value, and so it ends up with duplicate values. Detailed SQL log
|
I've seemingly solved the issues of the race conditions in the The filtering in the subquery = self._get_class_for_ordering_queryset().objects.filter(pk=self.pk).values(self.order_field_name)
qs = qs.filter(**{self.order_field_name + '__lt': Subquery(subquery),
self.order_field_name + '__gte': order}) If you'd take these changes, I'd be willing to put in a PR. I'm not a database expert, and can only test on Postgres, but I think these solutions are correct. |
I've added a PR to fix most of these issues in #190 |
+1 |
Is there an issue with the given PR? I find it discouraging that the maintainers haven't merged it nor commented on this issue after almost 2 years. |
@haplo Hi, there is no issue other than no time to work on this. If you are up to it you could pick up that PR, rebase it and I will review it. |
@imomaliev Fair enough! I will fork the project, rebase #190 and test the integration with my project. Hopefully we will be able to get this merged and released. |
I've accepted the merge, thanks! |
Can someone with a live deployment of django-ordered-model try out the branch? I'm not going to be able to test it. |
I just ran into this issue in a real life situation. Two instances ended up having the same |
@jasperoosthoek Could you try with @GeeWee's solution? |
I've just set up a test case for our situation with threading to simulate concurrent requests made by our UI. I hope I'll be able to test out the branch with that test, as I was able to pin point the cause to the For now I'm looking at edit: turns out that the approach is basically the same - locking the involved rows and refreshing the instance to produce the correct |
This is extremely hard to actually reproduce... I'm sorry. Basically the idea is that multiple requests can arrive at the same time (and they will do because of how the frontend is coded!) which kicks off a number of threads that all start with the same view on the data. django-ordered-model then performs some UPDATE queries based on that view of the data and finally a save call to the object itself to set the order. That last action often corrects a lot of things, but the UPDATE query causes mayhem. See also django-ordered-model/django-ordered-model#184.
This is extremely hard to actually reproduce... I'm sorry. Basically the idea is that multiple requests can arrive at the same time (and they will do because of how the frontend is coded!) which kicks off a number of threads that all start with the same view on the data. django-ordered-model then performs some UPDATE queries based on that view of the data and finally a save call to the object itself to set the order. That last action often corrects a lot of things, but the UPDATE query causes mayhem. See also django-ordered-model/django-ordered-model#184.
This is extremely hard to actually reproduce... I'm sorry. Basically the idea is that multiple requests can arrive at the same time (and they will do because of how the frontend is coded!) which kicks off a number of threads that all start with the same view on the data. django-ordered-model then performs some UPDATE queries based on that view of the data and finally a save call to the object itself to set the order. That last action often corrects a lot of things, but the UPDATE query causes mayhem. See also django-ordered-model/django-ordered-model#184.
This is extremely hard to actually reproduce... I'm sorry. Basically the idea is that multiple requests can arrive at the same time (and they will do because of how the frontend is coded!) which kicks off a number of threads that all start with the same view on the data. django-ordered-model then performs some UPDATE queries based on that view of the data and finally a save call to the object itself to set the order. That last action often corrects a lot of things, but the UPDATE query causes mayhem. See also django-ordered-model/django-ordered-model#184.
This is extremely hard to actually reproduce... I'm sorry. Basically the idea is that multiple requests can arrive at the same time (and they will do because of how the frontend is coded!) which kicks off a number of threads that all start with the same view on the data. django-ordered-model then performs some UPDATE queries based on that view of the data and finally a save call to the object itself to set the order. That last action often corrects a lot of things, but the UPDATE query causes mayhem. See also django-ordered-model/django-ordered-model#184.
Hello, what's the status on this? I'm looking to adopt this package for my application but I'm a little concerned about the possible race condition issue. |
This app is quite susceptible to race conditions. If two people try to move the same two objects to the top at the same time, they could both end up with the same order value. This is worse than it sounds because you can't reliably disentangle the objects - all the movement methods assume that the order is consistent and will happily swap one of the entangled objects with another, so that now that one is entangled. Moving objects to the top or bottom of the order has the same problem for the same reason.
There is a strong argument this is the application's responsibility to sort out, rather than the model. However even in that case, the admin arrows are susceptible and need to be fixed.
The text was updated successfully, but these errors were encountered: