Messages sent at the same time can encounter a race condition where they share the same thread_id

[petersmithrestless]

buddypress/bp-messages/classes/class-bp-messages-message.php
send() method

When sending a new message the thread id is fetched using

SELECT MAX(thread_id) FROM wp_bp_messages_messages

However if two messages are sent at the same time it's possible for both messages to get assigned the same thread_id.

It would be better to keep a separate table of threads with unique ids which are auto-incremented.

When creating a new thread, a row is inserted in to this table and the mysql_insert_id() command will return the correct ID.

[imath]

Hi @petersmithrestless

Thanks for your feedback. I agree it would be better to have a different table. But I believe improving this would be a pretty massive change to deal with. I'd be happy to have a patch about it making sure to take care back compatibility.

[petersmithrestless]

Patch to ensure thread_id is generated in a single db transaction while inserting a new message

[petersmithrestless]

I've created a patch which will ensure that the thread_id is generated at the same time as the message is inserted, in a single transaction.
This will ensure that two messages submitted at the same time cannot end up being assigned the same thread_id.
The generated thread_id is then fetched based on the newly inserted message id.

[imath]

Thanks a lot for your patch! I just gave a quick look and it's very promising. I'll review it asap and we'll work on having this fix available in 10.0.0.

[espellcaste]

Is it possible to have unit tests to replicate this issue or to show it works as desired?

[imath]

Couldn't find the time to review it yet. Sorry about it, I'll try to write unit tests during next major release.

[espellcaste]

I'll look into the patch and confirm if it fixes the issue.

[espellcaste]

@petersmithrestless I'm curious. When you say two messages are inserted at the same time for the thread. Are you talking about two messages from the same user? Or two completely separate users (with different thread ids)?

$this->thread_id = (int) $wpdb->get_var( "SELECT MAX(thread_id) FROM {$bp->messages-table_name_messages}" ) + 1;

Currently, the logic has a + 1 at the end, and it'd not be possible to return the same thread ID. Even if it could, the + 1 would increment it.

But if the $this->thread_id is already present, it'd be used. Which I guess you are saying this $this->thread_id is being set incorrectly to a random user. And that's how two separate users' messages can go to the same thread?

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/8544

[espellcaste]

I was hoping to find a way to unit test or replicate this issue, but it's unlikely. Fair to say the suggested approach/patch is a good one, in general.

By using a subquery, the "fetch" and insert requests are performed by mysql in the same connection. So we don't run the risk of a race condition, and it is, in theory, more performant.

​https://github.com/buddypress/buddypress/pull/399/

[espellcaste]

In 14075:

Messages: Use a mysql subquery to insert the thread_id while inserting a new thread message.

By moving the thread_id insertion into a subquery, we can avoid race conditions where two messages are sent at the same time and both use the same thread_id.

Props imath and petersmithrestless.

Closes ​https://github.com/buddypress/buddypress/pull/399
Fixes #8544