-
Notifications
You must be signed in to change notification settings - Fork 559
Lock Sliding Sync connections when inserting lazy members, to prevent repeated deadlocks. #19826
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
base: develop
Are you sure you want to change the base?
Changes from 3 commits
0d1c509
b1c313e
57a4d7c
4374b7b
6ba291b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Lock Sliding Sync connections when inserting lazy members, to prevent repeated deadlocks. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -204,6 +204,32 @@ def persist_per_connection_state_txn( | |
| raise SlidingSyncUnknownPosition() | ||
|
|
||
| (connection_key,) = row | ||
|
|
||
| if isinstance(self.database_engine, PostgresEngine): | ||
| # Lock the sliding sync connection row for update upfront, | ||
| # to prevent deadlocks between concurrent transactions | ||
| # (which can retry again and again without making progress). | ||
| # | ||
| # (We don't need to explicitly lock in the other branch, | ||
| # where we re-create the connection, as that implies a lock | ||
| # anyway) | ||
| # | ||
| # Specifically, the statements seen to deadlock against | ||
| # each other were | ||
| # `INSERT INTO sliding_sync_connection_lazy_members` | ||
| # with conflicting tuples on | ||
| # "sliding_sync_connection_lazy_members_idx" UNIQUE, btree | ||
| # (connection_key, room_id, user_id) | ||
| # https://www.postgresql.org/docs/current/explicit-locking.html#LOCKING-ROWS | ||
| txn.execute( | ||
| """ | ||
| SELECT 1 | ||
| FROM sliding_sync_connections | ||
| WHERE connection_key = ? | ||
| FOR NO KEY UPDATE | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't suppose we can optionally add this to the query above if its postgres? Or does it not work for more complex select statements?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's on a different table. I'm not really seeing a sensible way to rejig this otherwise; in the other branch of the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The select above joins on
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahhhh right I wasn't aware of |
||
| """, | ||
| (connection_key,), | ||
| ) | ||
| else: | ||
| # We're restarting the connection, so we clear the previous existing data we | ||
| # used to track it. We do this here to ensure that if we get lots of | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own reference, can someone explain the real life situations that causes this?
Someone is sending multiple concurrent requests with the same
connection_key?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would appear so, I guess? There were multiple users involved.
I suppose it's possible that some of these were retries after a connection dropped/timed out/... or something like that.