Skip to:
Content

BuddyPress.org

Opened 3 years ago

Last modified 5 weeks ago

#8544 new defect (bug)

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:
Milestone: Up Next Priority: normal
Severity: normal Version: 1.0
Component: Messages Keywords: has-patch
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 (10)

#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
2 years ago

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

#5 @imath
2 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
15 months ago

  • Milestone changed from Up Next to 12.0.0

#7 @imath
7 months ago

  • Milestone changed from 12.0.0 to Up Next

#8 @imath
4 months ago

  • Milestone changed from Up Next to 14.0.0

#9 @espellcaste
5 weeks ago

  • Milestone changed from 14.0.0 to Up Next
Note: See TracTickets for help on using tickets.