evolution-events / Artaxerxes

Evolution Events information system
http://www.evolution-events.nl
4 stars 0 forks source link

Prevent inconsistency in concurrent registrations #45

Closed ReneB closed 4 years ago

ReneB commented 6 years ago

There is a slight possibility that registrations may occur concurrently and if those registrations compete for the last spot, they may both get it if we don't properly fix our 💩.

The answer, it seems, is to start a transaction and first SELECT ... FOR UPDATE the event row itself. Any other transactions that read that transaction will ("probably" (see below)) be blocked until the transaction has committed. That means that it kind of acts as a gatekeeper.

However, the documentation, if taken literally, seems to suggest that not only other transactions that attempt to lock the row, but any other queries or transactions are blocked from reading those records. That would mean that pretty much the whole application would grind to a halt - it would no longer be able to render event indices, event pages, et cetera. That would make the app slow down at the exact moment when people are using it.

We should test if this is really how things work using the strategy outlined in the accepted answer of this StackOverflow question, but using "normal" SELECT statements (without the FOR UPDATE) on the second connection.

matthijskooijman commented 6 years ago

Here is a source on implementing atomicity in django, including select for update: https://medium.com/@hakibenita/how-to-manage-concurrency-in-django-models-b240fed4ee2 and the django reference: https://docs.djangoproject.com/en/dev/ref/models/querysets/

ReneB commented 6 years ago

The documentation seems to be incorrect.

In Window 1:

mysql> BEGIN;
Query OK, 0 rows affected (0.00 sec)

mysql> SELECT * FROM notification WHERE `date` >= '2011-05-03' FOR UPDATE;
+------+------------+----------------+
| id   | date       | text           |
+------+------------+----------------+
|    3 | 2011-05-03 | Notification 3 |
|    4 | 2011-05-04 | Notification 4 |
|    5 | 2011-05-05 | Notification 5 |
+------+------------+----------------+
3 rows in set (0.01 sec)

In Window 2:

mysql> SELECT * FROM notification WHERE `date` = '2011-05-02'
    -> ;
+------+------------+----------------+
| id   | date       | text           |
+------+------------+----------------+
|    2 | 2011-05-02 | Notification 2 |
+------+------------+----------------+
1 row in set (0.00 sec)

mysql> SELECT * FROM notification WHERE `date` = '2011-05-02' FOR UPDATE;

Only the last query blocks.

ReneB commented 6 years ago

To strengthen the conclusion a little:

Window 1:

mysql> BEGIN;
Query OK, 0 rows affected (0.00 sec)

mysql> SELECT * FROM notification WHERE `id` = 1  FOR UPDATE;
+------+------------+----------------+
| id   | date       | text           |
+------+------------+----------------+
|    1 | 2011-05-01 | Notification 1 |
+------+------------+----------------+
1 row in set (0.00 sec)

Window 2:

mysql> SELECT * FROM notification WHERE `id` = 1;
+------+------------+----------------+
| id   | date       | text           |
+------+------------+----------------+
|    1 | 2011-05-01 | Notification 1 |
+------+------------+----------------+
1 row in set (0.00 sec)

mysql> SELECT * FROM notification WHERE `id` = 1 FOR UPDATE;
ERROR 1205 (HY000): Lock wait timeout exceeded; try restarting transaction
ReneB commented 6 years ago

For more information on SELECT ... FOR UPDATE in Django: https://docs.djangoproject.com/en/2.0/ref/models/querysets/

matthijskooijman commented 6 years ago

This might also be relevant: https://docs.djangoproject.com/en/2.1/ref/models/expressions/#f-expressions

That shows how to let the database do an UPDATE ... SET x = x + 1 query, which might be useful in this regard.

matthijskooijman commented 4 years ago

I considered that it might be useful to, instead of locking the entire event, locking only the options that have limited availability (e.g. the "Player" or "NPC" option). Then, you could potentially process a Player registration at the same time as an NPC registration.

However, when a single registration influences multiple limited options (e.g. "Player" and "Female god"), both must be locked, and this opens up the possibility of the dining philosophers problem. When the row locks are taken one by one, this could lead to deadlock, or when they are taken all at the same time, that could lead to starvation (though with the limited number of requests we'll have, that is probably not a big problem).

I tested this on Mariadb, it seems it does one-by-one locking. I had three rows, locked row 2 first, then in a different window row 1,2 which blocks, then in a different window row 1, which also blocks, suggesting the second window already has 1 locked while waiting for 2.

One way to work around this is to lock rows in a specific order (e.g. sorted by id), which should guarantee no deadlock happens. However, it seems that mariadb locks whatever rows are available, not keeping a specific order. I tested this by doing the same test as above, but with select * from test where id IN (2, 1) FOR UPDATE;, hoping that it would wait for row 2 before locking 1, but that did not happen.

Also, the exact semantics of this locking is not really well-defined in Mariadb documentation, so it is probably safer not to rely on such details.

So, it is probably best to simply lock the entire event whenever finalizing a registration. Locking just one row should be fairly well-defined.