| Project: | Drupal Commerce |
| Version: | 7.x-1.x-dev |
| Component: | Documentation |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
| Issue tags: | 1.3 review |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| commerce-order-load-readonly.patch | 2.19 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 3,553 pass(es). | View details | Re-test |
Comments
#1
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.
#2
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,
DT
#3
Tagging.
#4
I dig it, but I think we'll need to write some tests before we can consider committing this to the entity controller.
#5
I think this is more a support request. But in order to help you, can you give us more information about your use case?
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:
<?phpentity_get_controller('commerce_order')->resetCache(array($order->order_id));
?>
#6
Hi Damien, thanks for the reply.
The issue we were having was with two cases:
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
#7
Automatically closed -- issue fixed for 2 weeks with no activity.
#8
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();
#9
That looks good to me based on the comments above.
#10
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.
#11
Where would you recommend this be documented?
#12
In the docblock for commerce_order_load would be great:
http://drupalcontrib.org/api/drupal/contributions!commerce!modules!order!commerce_order.module/function/commerce_order_load/7
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?
#13
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.
#14
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.