Confirmed ALLOW_DEADLOCK_RERUN combined with Entity#save

[PaulB]

Affected version

2.2.8

Several calls to AbstractAdapter#executeTransaction in the UserAlert repository pass the ALLOW_DEADLOCK_RERUN option, but that option can't be combined with entities unless forceSet is set on the affected entities. Any attempt to re-run the closure in response to a deadlock will result in a LogicException, thereby masking the underlying deadlock and making it difficult to debug. This is especially painful given how difficult it is to debug deadlocks in the first place.

There are safe uses of ALLOW_DEADLOCK_RERUN elsewhere in the vanilla XenForo codebase, such as in the Forum repository.

Note that although this is related to https://xenforo.com/community/threads/logicexception-attempted-to-set-alerts_unviewed.188223/, it's not the cause of that issue, and fixing this bug will not fix that bug; it will just change the error message.

 

[Xon]

Most of the calls inside explicit transactions for User Alerts can be easily rewritten into direct update statements which avoid transaction read/modify/write cycle.

This would reduce deadlocks, and also lock-times

 

[Jeremy P]

I've haven't dived too deep into everything but, looking at the outstanding issues as a whole, it seems like we'll need to:

• Run SELECT user_id FROM xf_user WHERE user_id = ? FOR UPDATE first to make lock orders more consistent
• Replace entity updates with update queries to avoid race conditions (and just call Entity::setAsSaved after)

 

[Xon]

I've haven't dived too deep into everything but, looking at the outstanding issues as a whole, it seems like we'll need to:

• Run SELECT user_id FROM xf_user WHERE user_id = ? FOR UPDATE first to make lock orders more consistent
• Replace entity updates with update queries to avoid race conditions (and just call Entity::setAsSaved after)

Something like how I've implemented markAlertIdsAsReadAndViewed in my Alert Improvements add-on could be used as inspiration.

Basically decompose into the following steps;

• If in a transaction:
  • Use for update on the xf_user record to hold a lock
• Update xf_user_alert.view_date (or read_date), and capture the changed rows counts.
• Update xf_user record (if needed!) with an update statement with the previous changed row counts.
  • If not needed, refresh the alert counts?
• Update in-memory objects via Entity::setAsSaved (this may include alert entities).

All the steps can be done without any explicit transactions and is very resistant to deadlocking.

 

[Xon]

Sadly there are a few functions, not just the reported one which need updating to have the same pattern and lock ordering:

• markUserAlertsViewed
• markUserAlertsRead
• markUserAlertViewed
• markUserAlertRead
• markSpecificUserAlertsRead
• markUserAlertsReadForContent
• markUserAlertUnread

Implementing these in my Alert Improvements add-on basically stopped deadlocks from the alert feature.

 

[Jeremy P]

Yeah, I was looking at making those structural changes to the alert repo as a whole.

Something like how I've implemented markAlertIdsAsReadAndViewed in my Alert Improvements add-on could be used as inspiration.

That's helpful, thanks.