Implement pessimistic entity locking and enable it for orders [#1243306]

Here is the drill: our whole processing stack for orders is particularly sensitive to race conditions. During the checkout process, especially, it is not unlikely for the same order to be processed twice. The two common cause for this are:

(1) People clicking on a button twice
(2) Payment gateways or other external services wanting to affect the status of the order at the same time as the customer

The only solution for this is to implement locking. This locking needs to be:

pessimistic: the order needs to be locked way upfront, as soon as it is loaded, and only unlocked when finally saved.
unconditional: because of the richness of the Drupal pipeline, there is no way to know upfront that an order is not going to be modified (and saved) down the road.

We made sure that core supports this type of use case #1185780: Make transactions more flexible and useful (which is in 7.x and will be released with 7.8). Now it is time for this to actually happen.

Comment	File	Size	Author
#1	1243306-commerce-locking.patch	8.54 KB	damien tournoud

Comments

Comment #1

damien tournoud commented 8 August 2011 at 22:51

Status:

Active

» Needs review

Status	File	Size
new	1243306-commerce-locking.patch	8.54 KB

Here is probably the most minimal patch for this. It's not the end of the road, but it's better then nothing, and I would like to put that in front of everyone as soon as possible.

Here are some of the things we need to figure out:

When should we decide to release the lock? As soon as possible or do we just keep it until the end of the request?
If we decide to release the lock as soon as possible, should we make sure that no order is saved if there is no active lock on it? I already know that in some parts of the codebase we save the order twice, and as a consequence we release the lock too early in that case.
Do we want to implement a clean way to load an order without enforcing the lock (as a consequence this order will not be save-able, but it might make tests cleaner.

This passes the test suite.

Comment #2

rszrama commented 10 August 2011 at 01:03

With this patch applied, I can generate a PDOException using the following code at /devel/php:

  global $user;

  // Add two products to the cart.
  foreach (array(1, 2) as $product_id) {
    $product = commerce_product_load($product_id);
    $line_item = commerce_product_line_item_new($product, 2);
    commerce_cart_product_add($user->uid, $line_item);
  }

  // Attempt to remove the line items both at once.
  $order = commerce_cart_order_load($user->uid);

  foreach (entity_metadata_wrapper('commerce_order', $order)->commerce_line_items as $delta => $line_item_wrapper) {
    $order = commerce_cart_order_product_line_item_delete($order, $line_item_wrapper->line_item_id->value());
    // break;
  }

  return 'Well, did it work?';

The exception message is: There is already an active transaction in db_transaction() (line 2479 of /includes/database/database.inc).

I've done what I can to try and debug this, but I'm not having any success.

Comment #3

rszrama commented 10 August 2011 at 16:28

Status:

Needs review

» Fixed

Ok, the error I was getting is already solved in Drupal 7.x-dev, so as soon as 7.8 or later is released, there won't be any issue. I'm going to go ahead and commit this patch, and if anyone runs into the db_transaction() error I mentioned above, they can just update to 7.x-dev for the fix.

Comment #4

carn1x commented 15 August 2011 at 03:01

they can just update to 7.x-dev for the fix.

Do you mean update to Commerce dev or Core dev?

Comment #5

rszrama commented 15 August 2011 at 14:05

Core dev, b/c it's a bug in Drupal itself not Commerce.

Comment #6

carn1x commented 16 August 2011 at 09:00

Sorry to get a little off-topic, are you aware of a clean method to take Drupal core stable to dev, and then back again once the next stable is released? Is this even a good idea or should I just wait until 7.8? I've floated the question in a few places, nothing but tumbleweed! Thanks :)

Comment #7

rszrama commented 16 August 2011 at 14:58

I suppose it really depends on what kind of changes have gone into core. The only danger is if you were to update to a version that made a schema change but then a later patch undid the schema change or something. You can review the changelog before updating to dev, but I didn't experience any problems. Buyer beware. ; )

And if it's too much risk, we'll have a 7.8 the first Wednesday of September. They're doing predictable point releases now.

Comment #8

j0rd commented 17 August 2011 at 10:48

I just upgraded to latest -dev and not I'm getting this error with devel module enabled. I get this error when attempting payment with authorize.net

Additional uncaught exception thrown while handling exception.

Original

PDOException: There is already an active transaction in db_transaction() (line 2479 of /home/www/powerengineercentral.ca/htdocs/includes/database/database.inc).

Additional

PDOException: There is already an active transaction in variable_set() (line 783 of /home/www/powerengineercentral.ca/htdocs/includes/bootstrap.inc).

Uncaught exception thrown in session handler.

PDOException: There is already an active transaction in _drupal_session_write() (line 203 of /home/www/powerengineercentral.ca/htdocs/includes/session.inc).

This is a blocker for my site, as I can't process any transactions now.

Additionally even with this error, it appears my hook_order_paid_in_full which I would assume if we're using transaction locking, should not happen.

Digging into it.

Update: Disabled devel and it doesn't appear to happen after I tried again. I did get another interesting bug related to PHP errors on checkout which needs to be addressed. http://drupal.org/node/1251840