thumbnail: Only lock the message row, not the Attachment row.

This prevents a deadlock between the thumbnailing worker and message
sending, as follows:

1. A user uploads an image, making Attachment and ImageAttachment
   rows, as well as enqueuing a job in the thumbnailing queue.

2. Message sending starts a transaction, creates the Message row,
   and calls `do_claim_attachments`, which edits the Attachment row
   of the upload (implicitly locking it).

3. The thumbnailing worker starts a transaction, locks the
   ImageAttachment row for its image, thumbnails it, and then
   attempts to `select_for_update()` the message objects (joined to
   the Attachments table) to find the ones which link to the
   attachment in question. This query blocks, since "a locking
   clause without a table list affects all tables used in the
   statement"[^1] and the message-send request already has a write
   lock on the Attachments row in question.

4. The message-send request attempts to re-fetch the ImageAttachment
   row inside the transaction, which tries to pull a lock on it.

5. Deadlock, because the message-send request has the Attachment
   lock, and waits for the ImageAttachment lock; the thumbnailing
   worker has the ImageAttachment lock, and waits for the Attachment
   lock.

We break this deadlock by limiting the
`update_message_rendered_content` `select_for_update` to only take
the lock on the Message table, and not also the Attachments table --
no changes will be made to the Attachments, so no lock is necessary
there. This allows the thumbnailing worker to successfully pull the
empty list of messages (since the message-send request has not
commits its transaction, and thus the Message row is not visible
yet), and release its ImageAttachment lock so that the message-send
request can proceed.

[^1]: https://www.postgresql.org/docs/current/sql-select.html#SQL-FOR-UPDATE-SHARE

(cherry picked from commit 3cbbf2307b)
This commit is contained in:
Alex Vandiver 2024-10-04 15:31:25 +00:00 committed by Tim Abbott
parent 8dca9af3d0
commit 57518a2059
1 changed files with 1 additions and 1 deletions

View File

@ -162,7 +162,7 @@ def update_message_rendered_content(
message_class.objects.filter( # type: ignore[attr-defined] # TODO: ? message_class.objects.filter( # type: ignore[attr-defined] # TODO: ?
realm_id=realm_id, attachment__path_id=path_id realm_id=realm_id, attachment__path_id=path_id
) )
.select_for_update() .select_for_update(of=("self",))
.order_by("id") .order_by("id")
) )
for message in messages_with_image: for message in messages_with_image: