Fixed modifyThreadUserPostCount is vulnerable to a race condition

[Xon]

Every so often, the following error is throwing:

Zend_Db_Statement_Mysqli_Exception: Mysqli statement execute error : Duplicate entry '351160-712' for key 'PRIMARY' - library/Zend/Db/Statement/Mysqli.php:214
#0 /var/www/html/library/Zend/Db/Statement.php(297): Zend_Db_Statement_Mysqli->_execute(Array)
#1 /var/www/html/library/Zend/Db/Adapter/Abstract.php(479): Zend_Db_Statement->execute(Array)
#2 /var/www/html/library/Zend/Db/Adapter/Abstract.php(574): Zend_Db_Adapter_Abstract->query('INSERT INTO `xf...', Array)
#3 /var/www/html/library/XenForo/Model/Thread.php(2053): Zend_Db_Adapter_Abstract->insert('xf_thread_user_...', Array)
#4 /var/www/html/library/XenForo/DataWriter/DiscussionMessage/Post.php(105): XenForo_Model_Thread->modifyThreadUserPostCount(351160, 712, 1)
...

modifyThreadUserPostCount does a select then insert or update. While it runs in a transaction, if there is nothing to select, then the row isn't going to be enrolled into the transaction to that row being blocked. The default transaction level does not block new rows! (SERIALIZABLE isn't usable in most MySQL clustered solutions).

Most of the contents of modifyThreadUserPostCount can be replaced with just:

$db->query('
insert into xf_thread_user_post (thread_id, user_id, post_count) values
(?, ?, ?)
on duplicate key update  post_count =  post_count + VALUES(post_count)
', array($threadId, $userId, $modifyValue));

Alternatively, updating the existing code so if the insert fails, an update occurs.

 

[Xon]

I'm fairly sure this is related to an admin on SB (*cough* @Isil`Zha *cough*) who has flood protection disabled, and double-clicking reply to a post or something.

 

[Isil`Zha]

hahaha, yeah I did double-click a post accidentally yesterday (and it displayed the thread as though it were in mobile responsive mode on my desktop... and double clicking is exactly what I attributed it to.)

 

[Mike]

I'm not sure why I wrote the method like this. I think it mostly revolves around returning the new value, which we don't actually use, but since I can't remove that, I'm mostly going to keep it as is and just change to on duplicate key update, though that does mean on this race condition case it could return a slightly incorrect number.

Thanks.