DrupalCommerceEntityController allows for entities to define a locking mode.

The default method when loading any commerce orders is "pessimistic"

Note, the comment in commerce.controller.inc : "In pessimistic locking mode, we issue the load query with a FOR UPDATE clause. This will block all other load queries to the loaded objects"

From the API docs:
FOR UPDATE prevents the rows retrieved by the SELECT statement from being modified or deleted by other transactions until the current transaction ends. Other transactions that attempt UPDATE, DELETE, or SELECT FOR UPDATE of these rows will be blocked until the current transaction ends.

This locking mode can result in lock wait timeout errors, e.g: when the same order is loaded twice, and perhaps modified, in a page load.

The error looks like:

https://.../admin/commerce/orders/599/payment
Message    PDOException: SQLSTATE[HY000]: General error: 1205 Lock wait timeout
exceeded; try restarting transaction: SELECT revision.order_number AS
order_number, revision.revision_id AS revision_id, revision.revision_uid AS
revision_uid, revision.mail AS mail, revision.status AS status, revision.log AS
log, revision.revision_timestamp AS revision_timestamp,
revision.revision_hostname AS revision_hostname, revision.data AS data,
base.order_id AS order_id, base.type AS type, base.uid AS uid, base.created AS
created, base.changed AS changed, base.hostname AS hostname FROM
{commerce_order} base INNER JOIN {commerce_order_revision} revision ON
revision.revision_id = base.revision_id WHERE (base.order_id IN
(:db_condition_placeholder_0)) FOR UPDATE; Array (
[:db_condition_placeholder_0] => 599 ) in DrupalDefaultEntityController->load()
(line 196 of /var/www/.../includes/entity.inc).

It's not necessary to lock like this, especially when you only need to read the data on an order, not update it.

It's a problem in systems with multiple users loading and saving orders, such as managing online subscriptions with Commerce, (the author's current project)

The attached patch provides a solution to this issue by allowing developers to pass a "readonly" flag to either

  1. commerce_order_load
  2. commerce_order_load_multiple

DrupalCommerceEntityController then checks this readonly condition prior to setting the locking $query->forUpdate(); option.

Old method:

<?php
/**
 * Loads an order by ID.
 */
function commerce_order_load($order_id) {
 
$orders = commerce_order_load_multiple(array($order_id), array());
  return
$orders ? reset($orders) : FALSE;
}
?>

New method:

The patch adds a single parameter to commerce_order_load, an array of conditions. This has the added benefit that instead of just passing an empty array to commerce_order_load_multiple, a developer can actually specify additional load conditions, e.g order status

<?php
/**
 * Loads an order by ID.
 */
function commerce_order_load($order_id, $conditions = array()) {
 
$orders = commerce_order_load_multiple(array($order_id), $conditions);
  return
$orders ? reset($orders) : FALSE;
}
?>

Example order load using readonly method:

  1. commerce_order_load(1, array('readonly' => TRUE))
  2. commerce_order_load_multiple(array(1,2), array('readonly' => TRUE))

Patch attached.

Files: 
CommentFileSizeAuthor
#2 1514618-commerce-order-load-readonly-v3.patch2.64 KBdavidwhthomas
PASSED: [[SimpleTest]]: [MySQL] 3,553 pass(es).
[ View ]
#1 1514618-commerce-order-load-readonly.patch2.52 KBdavidwhthomas
PASSED: [[SimpleTest]]: [MySQL] 3,553 pass(es).
[ View ]
#1 1514618-commerce-order-load-readonly.patch2.58 KBdavidwhthomas
PASSED: [[SimpleTest]]: [MySQL] 3,553 pass(es).
[ View ]
commerce-order-load-readonly.patch2.19 KBdavidwhthomas
PASSED: [[SimpleTest]]: [MySQL] 3,557 pass(es).
[ View ]

Comments

davidwhthomas’s picture

Issue summary:View changes

minor edit

davidwhthomas’s picture

Issue summary:View changes

Clarity

davidwhthomas’s picture

Issue summary:View changes

clarity

davidwhthomas’s picture

Issue summary:View changes

clarity

davidwhthomas’s picture

Issue summary:View changes

syntax highlighting

davidwhthomas’s picture

Issue summary:View changes

more info

davidwhthomas’s picture

Issue summary:View changes

formatting

davidwhthomas’s picture

StatusFileSize
new2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 3,553 pass(es).
[ View ]
new2.52 KB
PASSED: [[SimpleTest]]: [MySQL] 3,553 pass(es).
[ View ]

Here's a slightly simplified version of the patch

Edit: Sorry, accidentally attached two files and can't delete the first one.

The second patch is the recommended one.

davidwhthomas’s picture

Issue summary:View changes

formatting

davidwhthomas’s picture

Issue summary:View changes

text

davidwhthomas’s picture

StatusFileSize
new2.64 KB
PASSED: [[SimpleTest]]: [MySQL] 3,553 pass(es).
[ View ]

OK, one more update to this.

Here's an adjusted version of the patch that uses 'lock' instead of 'readonly' in the condition.

The default is to allow locking, but by setting 'lock' => FALSE a developer can disable locking for that query.

I think that terminology makes more sense here.

Note, the new load condition

Example order load using 'lock' => false method:

  1. commerce_order_load(1, array('lock' => FALSE))
  2. commerce_order_load_multiple(array(1, 2), array('lock' => FALSE))

Thanks,

DT

rszrama’s picture

Issue tags:+1.3 review

Tagging.

helior’s picture

Status:Needs review» Needs work

I dig it, but I think we'll need to write some tests before we can consider committing this to the entity controller.

Damien Tournoud’s picture

Status:Needs work» Postponed (maintainer needs more info)

I think this is more a support request. But in order to help you, can you give us more information about your use case?

It's a problem in systems with multiple users loading and saving orders, such as managing online subscriptions with Commerce, (the author's current project)

When you load an order, it's going to be locked for the duration of the active transaction (that we force open until you either unload it or save it). For transactional requests this is obviously not a problem, but you have to design your background scripts with this in mind.

If you need to unload a given order, use:

<?php
entity_get_controller
('commerce_order')->resetCache(array($order->order_id));
?>
davidwhthomas’s picture

Status:Postponed (maintainer needs more info)» Fixed

Hi Damien, thanks for the reply.

The issue we were having was with two cases:

  1. Completing checkout, the payment gateway would send an IPN alert which needed to load the order but that timed out with the lock wait timeout error.
  2. Cron threads running separately (supervisord) that populated activation and expiry queues while the orders were being edited by administrators on the site.

Fortunately, at this stage we were able to fix both issues:

Issue #1 was solved with the method: entity_get_controller('commerce_order')->resetCache(array($order->order_id)); prior to sending the transaction to the payment gateway.
Issue #2 was solved with code adjustments to not load the whole order during cron queue population, but just the order id.

Therefore, I'm actually happy with the issue at the moment, because through code changes, and the use of the resetCache function we were able to fix the lock wait timeout errors.

It's useful to be able to use that resetCache function which essentially can be used to unlock a readonly order.

Marking as fixed on that basis.

Thanks again, awesome module set.

David

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

ceng’s picture

Category:bug» support
Status:Closed (fixed)» Active

Sorry to reopen but I think it might be worth adding a definitive pattern to the end of this thread to demonstrate how to load a read only commerce order in non-locking mode.

Is the following pattern correct?

// Load the order I want to read from.
$order = commerce_order_load('1234');

// Immediately unload to make the order available to everyone again.
entity_get_controller('commerce_order')->resetCache(array($order->order_id));

// Read data values from $order, for example:
$order_wrapper = entity_metadata_wrapper('commerce_order', $order);
$line_items = $order_wrapper->commerce_line_items->value();

rszrama’s picture

Status:Active» Closed (fixed)

That looks good to me based on the comments above.

aidanlis’s picture

Status:Closed (fixed)» Active

Wait, this is kind of a big deal ... I'm getting these lock timeouts on a daily basis on a heavily trafficked store. The lock flag looks a lot nicer than the resetCache approach, why isn't that patch being applied? Either way this needs to be documented somewhere prominently for third party developers.

rszrama’s picture

Version:7.x-1.2» 7.x-1.x-dev
Status:Active» Postponed (maintainer needs more info)

Where would you recommend this be documented?

aidanlis’s picture

Component:Developer experience» Documentation
Category:support» bug
Status:Postponed (maintainer needs more info)» Active

In the docblock for commerce_order_load would be great:
http://drupalcontrib.org/api/drupal/contributions!commerce!modules!order...

Given how many problems this can cause I think it probably needs to be documented somewhere else, maybe section 11:
http://www.drupalcommerce.org/developer-guide/development-standards

I think given how this is manifesting, e.g. #1320062: SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded, providing an easy way to load an order without it being locked should be a high priority. Is there any work that needs to be done to get the patch included?

rszrama’s picture

Category:bug» task

Make a patch, I guess? Documentation issues shouldn't classified as bug reports, so I've made this a task, but feel free to update the title to a more "tasky" sounding name. If there's a bug to address, we can use that issue you linked in to handle it - no need to double report - and if you think this is just one and the same issue, then let's close this one back out.

aidanlis’s picture

Title:PDOException: Lock wait timeout exceeded; try restarting transaction» Developer documentation: Loading order entities without considering locking can cause problems

Orders should be loaded unlocked if they are being read and not saved, otherwise lock timeouts will occur in high trafficked sites which can destabilise the entire server (due to too many processes waiting for the lock to be released).

commerce_order_load(1, array('lock' => FALSE))
commerce_order_load_multiple(array(1, 2), array('lock' => FALSE))

This should only be documented when the #1320062: SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded patch has been applied.

thim’s picture

Hi Aidanlis,

I have seen several patches passing by in this issue and another issue https://drupal.org/node/1320062#comment-7424886

Which patch is the final patch?
or
Do you suggest to use a simple resetCache like described in https://drupal.org/node/1514618#comment-6185298

aidanlis’s picture

I think in the end it's just easier to use the resetCache method, as no one seems to have much interest in committing this.

The resetCache method isn't ideal: if you're loading 100 orders using commerce_order_load(array(...)) this means they are locked until that function returns and you can unlock them.

pjcdawkins’s picture

A very similar issue, #1608294: Provide a way to load an order without locking, considers this problem a major/DX/bug, not a normal/Documentation/task.

ardnet’s picture

Status:Active» Needs review

commerce-order-load-readonly.patch queued for re-testing.

ardnet’s picture

Status:Needs review» Active

Sorry, for some reason I accidentally clicked on the Re-test button, put this task back as "active" again.

ardnet’s picture

Issue summary:View changes

text

bojanz’s picture

Status:Active» Closed (duplicate)

I'm going to close this issue in favor of #1608294: Provide a way to load an order without locking.
We definitely need to implement a way to load the order read only.
The current behavior is requiring me to disable locking completely on commerceguys sites.