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 | Owned by: | 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)
Change History (15)
#1
@
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
@
3 years ago
Patch to ensure thread_id is generated in a single db transaction while inserting a new message
#2
@
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
@
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
@
3 years ago
Is it possible to have unit tests to replicate this issue or to show it works as desired?
#5
@
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.
#11
@
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
@
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
Trac ticket: https://buddypress.trac.wordpress.org/ticket/8544
#14
@
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.
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.