Skip to:
Content

BuddyPress.org

Opened 3 years ago

Last modified 2 days ago

#8544 assigned enhancement

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

Reported by: petersmithrestless's profile petersmithrestless Owned by: espellcaste's profile espellcaste
Milestone: 15.0.0 Priority: low
Severity: normal Version: 1.0
Component: Messages Keywords: has-patch needs-unit-tests
Cc:

Description

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.

Attachments (1)

8544.patch (2.7 KB) - added by petersmithrestless 3 years ago.
Patch to ensure thread_id is generated in a single db transaction while inserting a new message

Download all attachments as: .zip

Change History (15)

#1 @imath
3 years ago

  • Component changed from Core to Messages
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Awaiting Contributions
  • Version changed from 9.0.0 to 1.0

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
3 years ago

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

#2 @petersmithrestless
3 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Contributions to Awaiting Review

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.

#3 @imath
3 years ago

  • Milestone changed from Awaiting Review to 10.0.0

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.

#4 @espellcaste
3 years ago

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

#5 @imath
3 years ago

  • Milestone changed from 10.0.0 to Up Next

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

#6 @imath
22 months ago

  • Milestone changed from Up Next to 12.0.0

#7 @imath
14 months ago

  • Milestone changed from 12.0.0 to Up Next

#8 @imath
11 months ago

  • Milestone changed from Up Next to 14.0.0

#9 @espellcaste
9 months ago

  • Milestone changed from 14.0.0 to Up Next

#10 @imath
4 months ago

  • Milestone changed from Up Next to 15.0.0

#11 @espellcaste
3 months ago

  • Keywords needs-unit-tests needs-refresh added
  • Owner set to espellcaste
  • Priority changed from normal to low
  • Status changed from new to assigned

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

#12 @espellcaste
4 days ago

  • Milestone changed from 15.0.0 to Under Consideration

@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?

This ticket was mentioned in PR #399 on buddypress/buddypress by renatonascalves.


4 days ago
#13

  • Keywords needs-refresh removed

#14 @espellcaste
4 days ago

  • Milestone changed from Under Consideration to 15.0.0
  • Type changed from defect (bug) to enhancement

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/

Last edited 2 days ago by espellcaste (previous) (diff)
Note: See TracTickets for help on using tickets.