Developer documentation: Loading order entities without considering locking can cause problems [#1514618]

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

commerce_order_load
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:

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

Patch attached.

Comment	File	Size	Author
#2	1514618-commerce-order-load-readonly-v3.patch	2.64 KB	davidwhthomas
#1	1514618-commerce-order-load-readonly.patch	2.52 KB	davidwhthomas
#1	1514618-commerce-order-load-readonly.patch	2.58 KB	davidwhthomas
	commerce-order-load-readonly.patch	2.19 KB	davidwhthomas

Comments

Comment #0.0

davidwhthomas commented 4 April 2012 at 08:24

Issue summary:

View changes

minor edit

Comment #0.1

davidwhthomas commented 4 April 2012 at 08:27

Issue summary:

View changes

Clarity

Comment #0.2

davidwhthomas commented 4 April 2012 at 08:28

Issue summary:

View changes

clarity

Comment #0.3

davidwhthomas commented 4 April 2012 at 08:29

Issue summary:

View changes

clarity

Comment #0.4

davidwhthomas commented 4 April 2012 at 08:30

Issue summary:

View changes

syntax highlighting

Comment #0.5

davidwhthomas commented 4 April 2012 at 08:31

Issue summary:

View changes

more info

Comment #0.6

davidwhthomas commented 4 April 2012 at 08:32

Issue summary:

View changes

formatting

Comment #1

davidwhthomas commented 4 April 2012 at 08:50

Status	File	Size
new	1514618-commerce-order-load-readonly.patch	2.58 KB
new	1514618-commerce-order-load-readonly.patch	2.52 KB

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.

Comment #1.0

davidwhthomas commented 4 April 2012 at 08:50

Issue summary:

View changes

formatting

Comment #1.1

davidwhthomas commented 4 April 2012 at 10:06

Issue summary:

View changes

text

Comment #2

davidwhthomas commented 4 April 2012 at 10:24

Status	File	Size
new	1514618-commerce-order-load-readonly-v3.patch	2.64 KB

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:

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

Thanks,

Comment #3

rszrama commented 24 April 2012 at 19:56

Issue tags:

+1.3 review

Tagging.

Comment #4

helior commented 26 April 2012 at 04:15

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.

Comment #5

damien tournoud commented 5 May 2012 at 11:44

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:

entity_get_controller('commerce_order')->resetCache(array($order->order_id));

Comment #6

davidwhthomas commented 5 May 2012 at 23:30

Status:

Postponed (maintainer needs more info)

» Fixed

Hi Damien, thanks for the reply.

The issue we were having was with two cases:

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.
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

Comment #7

19 May 2012 at 23:30

Status:

Fixed

» Closed (fixed)

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

Comment #8

johnennew commented 2 July 2012 at 10:12

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();

Comment #9

rszrama commented 16 November 2012 at 03:27

Status:

Active

» Closed (fixed)

That looks good to me based on the comments above.

Comment #10

aidanlis commented 29 January 2013 at 22:24

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.

Comment #11

rszrama commented 29 January 2013 at 23:06

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

Where would you recommend this be documented?

Comment #12

aidanlis commented 6 February 2013 at 00:37

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?

Comment #13

rszrama commented 6 February 2013 at 04:19

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.

Comment #14

aidanlis commented 10 February 2013 at 19:52

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.

Comment #15

thim commented 24 July 2013 at 15:33

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

Comment #16

aidanlis commented 31 July 2013 at 13:05

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.

Comment #17

pjcdawkins commented 25 September 2013 at 08:30

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.

Comment #18

ardnet commented 6 October 2013 at 20:51

Status:

Active

» Needs review

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

Comment #19

ardnet commented 6 October 2013 at 21:04

Status:

Needs review

» Active

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

Comment #19.0

ardnet commented 6 October 2013 at 21:04

Issue summary:

View changes

text

Comment #20

bojanz commented 14 April 2014 at 07:54

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.

Comment #21

BITANUBE commented 10 June 2016 at 19:52

Lot of thanks to #5 Damien Tournoud !

Works like a charm !

:-)

Comment #22

thirstysix commented 4 April 2017 at 09:23

Same issue..