Problem

There is a problem in the function commerce_marketplace_cart_order_load_multiple in commerce_marketplace_cart.module where the last order_group id is found with SELECT order_group FROM commerce_order ORDER BY order_group DESC LIMIT 1.

If two users add a product in the cart at the same time, they might get the same order_group.

Proposed resolution

Create a new table order_group_ids with a primary key, auto-increment, id column.
Instead of retrieving the order_group id and increment it, insert a new row in the new table order_group_ids and use LAST_INSERT_ID() and the equivalent in PostgreSQL to retrieve the id.

MySQL documentation on LAST_INSERT_ID() :

The ID that was generated is maintained in the server on a per-connection basis. This means that the value returned by the function to a given client is the first AUTO_INCREMENT value generated for most recent statement affecting an AUTO_INCREMENT column by that client. This value cannot be affected by other clients, even if they generate AUTO_INCREMENT values of their own. This behavior ensures that each client can retrieve its own ID without concern for the activity of other clients, and without the need for locks or transactions.

Remaining tasks

This is a quick fix that works, but should really be optimized.

For example :

- add a column orders in the table order_group_ids to store the serialized orders for this order_group
- if (!empty($orders)) : add the order to the column
- else : change db_query with db_insert to insert and retrieve the id at the same time

Data model changes

Table order_group_ids created.

CommentFileSizeAuthor
#9 commerce_marketplace-fix_race_condition-2850718-9.patch2.88 KBjordan.gillet
#8 commerce_marketplace-fix_race_condition-2850718-8.patch2.88 KBjordan.gillet
#7 commerce_marketplace-fix_race_condition-2850718-7.patch.patch2.87 KBjordan.gillet
#2 commerce_marketplace-fix-race-condition-2850718.patch3.19 KBjordan.gillet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jordan.gillet created an issue. See original summary.

jordan.gillet’s picture

jordan.gillet CreditAttribution: jordan.gillet commented
FileSize
commerce_marketplace-fix-race-condition-2850718.patch3.19 KB

Here is the patch.

Thanks for the review.

jordan.gillet’s picture

jordan.gillet CreditAttribution: jordan.gillet commented
Assigned: jordan.gillet » Unassigned
Status: Needs work » Needs review
Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Status: Needs review » Needs work

Hello,

Thanks for the patch.

As I don't know the Commerce marketplace module, here is a review on styles and good practices.

  1. +++ b/modules/cart/commerce_marketplace_cart.install
@@ -0,0 +1,66 @@
+<?php

    Missing blank line after <?php opening. 

  2. +++ b/modules/cart/commerce_marketplace_cart.install
@@ -0,0 +1,66 @@
+  // If orders already exist, restart the sequence

    Missing ending point. 

  3. +++ b/modules/cart/commerce_marketplace_cart.install
@@ -0,0 +1,66 @@
+ * Implements hook_update().

    For hook_update_N, you should tell what the hook does and not Implements hook_update_N. This comment will be shown in the terminal when using Drush or in the update page to inform on what the update does. 

  4. +++ b/modules/cart/commerce_marketplace_cart.install
@@ -0,0 +1,66 @@
+    // If orders already exist, restart the sequence

    Missing ending point. 

  5. +++ b/modules/cart/commerce_marketplace_cart.install
@@ -0,0 +1,66 @@
+  $last_id = db_select('commerce_order','co')->fields('co', array('order_group'))->orderby('order_group','DESC')->range(0,1)->execute()->fetchField();

    Code would be easier to read if you have this line on multiple lines. 

  6. +++ b/modules/cart/commerce_marketplace_cart.install
@@ -0,0 +1,66 @@
+    $last_id = db_select('commerce_order','co')->fields('co', array('order_group'))->orderby('order_group','DESC')->range(0,1)->execute()->fetchField();

    Same comment for readability. 

  7. +++ b/modules/cart/commerce_marketplace_cart.module
@@ -256,14 +256,14 @@ function commerce_marketplace_cart_order_load_multiple($uid = 0, $conditions = a
+    db_query("INSERT INTO order_group_ids (id) VALUES (default);");
+    return Database::getConnection()->lastInsertId();

    Why not using the db_insert() function?

Also, after posting a patch, you should change the status to "needs review", and maybe remove the assignation the issue.

Your patch should have been named commerce_marketplace-fix_race_condition-2850718-2.patch (missing the comment number and be more precise on underscores).

PS: comment written before comment 3 was posted :)

jordan.gillet’s picture

jordan.gillet CreditAttribution: jordan.gillet commented
Issue summary: View changes
jordan.gillet’s picture

jordan.gillet CreditAttribution: jordan.gillet commented
Issue summary: View changes
jordan.gillet’s picture

jordan.gillet CreditAttribution: jordan.gillet commented
Status: Needs work » Needs review
FileSize
commerce_marketplace-fix_race_condition-2850718-7.patch.patch2.87 KB

Hello,

Thank you for the review.

I have updated the patch with the good practices and some refactoring.
I didn't use the db_insert() function because the keyword default is sent in string and doesn't work in PostgreSQL.

jordan.gillet’s picture

jordan.gillet CreditAttribution: jordan.gillet commented
FileSize
commerce_marketplace-fix_race_condition-2850718-8.patch2.88 KB
jordan.gillet’s picture

jordan.gillet CreditAttribution: jordan.gillet commented
FileSize
commerce_marketplace-fix_race_condition-2850718-9.patch2.88 KB

Fixed space in file name.

farhadhf’s picture

farhadhf CreditAttribution: farhadhf as a volunteer commented
Status: Needs review » Closed (won't fix)

Are you sure this is the right module?! We don't even have a file called commerce_marketplace_cart.module. Actually, this module does not have the directory /modules/cart/ at all! I'm closing this issue.