Improve commerce entities locking

I don't think that the option of throwing exceptions when saving a non locked order is really necessary even useful at all. In the end, you can still reload the order locking it into the save method, actually it will be exactly the same as if you'd loaded it locking it in the first place. The only thing it changes is that you'll lock the order a tiny shorter amount of time. I don't see use cases where it is useful to proceed this way, do you have any?

New patch with minor syntax fix.

I'm not sure about #4. As far as I can tell, the point of locking is to ensure the order and its related entities are the same as they were at the moment you decided whether you want to make a change. If that's correct, locking an unlocked order right before saving it would be too late.

@#5 Right, indeed, another thread could attempt a save between this thread's load and save operations.

For consistency, the locking is pretty much not optional. We really want to trigger an exception if you are trying to save an unlocked order instead of letting people with unknown bugs that will cause trouble at the worse possible moment (in production during high traffic).

I like the look of the test results. Now we just need to fix those :)

Is there any workaround till it is evolving cos my sites are collapsing. Would the cron run be solution for this?

@mibfire: try the patch in https://drupal.org/node/1608294#comment-8000455 and see if that works for you. Clear all caches after applying the patch, then it should basically disable locking for all views. We're using it in production out of sheer necessity. Others have disable locking altogether, but I'm not sure how to do it and how big of a chance there is of side-effects.

Thx ur reply a.ross

I tryed this patch https://drupal.org/node/1514618 but didnt solve all cases. What is the difference between these patches? Should i use both of them?

Basically what my patch does is prevent locking only for Views, as that is the highest impact scenario. If you have problems elsewhere you should probably alter the entity info to disable pessimistic locking entirely. I think I have an idea how to do it, but maybe someone else here can help you with a tested piece of code.

Hopefully the refactoring will be done sooner rather than later, so we won't have to be juggling patches anymore.

Thx, but should i use both patches or just yours?

Please follow Bojanz' advice. The patch here fails tests and it has been clearly mentioned that you shouldn't use it. Like I said, if you still experience problems using my patch in the other issue, then disable locking altogether.

If your hosting allows you to tweak MySQL configuration, that might help as well.

   protected function buildQuery($ids, $conditions = array(), $revision_id = FALSE) {
     $query = parent::buildQuery($ids, $conditions, $revision_id);
 
-    if (isset($this->entityInfo['locking mode']) && $this->entityInfo['locking mode'] == 'pessimistic') {
+    if ($this->requiresLocking() && !empty($conditions['_lock'])) {

The _lock element should be removed from $conditions before passing to parent::buildQuery(), otherwise you get an SQL exception.

I think the other tasks are just to pass $lock = TRUE on the end of a lot of (30 or so) commerce_order_load() and commerce_cart_order_load() calls.

a.ross, I did never say that i wanna use the patch from this issue(https://drupal.org/node/2240427). I said: There are already two patches here https://drupal.org/node/1514618#comment-5825868 and here(it is yours) https://drupal.org/node/1608294#comment-8000455

So which patches should i use or both?

I didn't notice that you were talking about another patch. My bad.
I can't comment on the other one as I don't know what it does. But it's quite likely that combining the patches won't work. I suggest you solely try my patch before trying anything else.

There's a lot more to do, I'm just taking advantage of the test bot here (to check my local tests are working OK)

It looks like (possibly) all the recent exceptions were coming from commerce_cart_product_add(), which is the only function that calls commerce_cart_order_new() if a cart order doesn't yet exist. It seems the cart order then needs to be re-loaded from the DB so that it can be locked. So, in commerce_cart_product_add():

 // ...
  // If no order existed, create one now.
  if (empty($order)) {
    $order = commerce_cart_order_new($uid);
    // Reload the order from the database, to enable locking.
    $order = commerce_order_load($order->order_id, TRUE);
    $order->data['last_cart_refresh'] = REQUEST_TIME;
  }
 // ...

24: 2240427-24-rework-order-locking.patch queued for re-testing.

Two issues addressed, one obvious, one less so:

1. If we are loading an order with locking, and it has already been loaded unlocked in the same request, then the entity controller's cache needs to be bypassed. So I added to DrupalCommerceEntityController:

  /**
   * Overrides DrupalDefaultEntityController::load().
   */
  public function load($ids = array(), $conditions = array()) {
    // If locking has been required, then bypass the internal cache for any
    // entities that are not already locked.
    if (!empty($conditions['_lock']) && $this->requiresLocking()) {
      foreach (array_diff($ids, $this->lockedEntities) as $id) {
        unset($this->entityCache[$id]);
      }
    }
    elseif (isset($conditions['_lock']) && !$conditions['_lock']) {
      // Remove the pseudo-condition _lock if it is FALSE.
      unset($conditions['_lock']);
    }
    return parent::load($ids, $conditions);
  }

2. It turns out the function commerce_cart_order_product_line_item_delete() always saves the order, via commerce_line_item_delete() then commerce_line_item_delete_references(). So I removed its $skip_save parameter.

I wish the testing system didn't create a new comment for every failed patch...

Leaving 'Needs work' as there will still be some test failures. It seems that up to 11 of them are a direct consequence of these changes - the others appear to be indirect effects - and I'm a bit stuck (partly because running tests locally is taking me 1-2 hours).

I have a site deadline in just a few days. Has any progress been made on this?

My issue is that I'm trying to use the Commerce module on a CDN. As soon as the second server connects to the database the link for "add product" and "create product" disappear as well as billing and shipping addresses cannot be added to orders.

Has anyone experienced these problems? Is there a fix for the database locking?

I want to work on this, does anyone have worked on this after #33 but hasn't posted it here?
tnx

Hi guys,

(I took the latest dev version and applied the patch.)
When trying to save an order at '/admin/commerce/orders/78/edit',
gives the following error. That is the main problem at the moment? Or I did something wrong

Exception: Cannot save unlocked commerce_order entity #78 in DrupalCommerceEntityController->save() (line 222 of C:\xampp\htdocs\coolgift\profiles\commerce_kickstart\modules\contrib\commerce\includes\commerce.controller.inc).

Re. #37: I'm not working on this at the moment and I don't think anyone else is.

Re. #38: the patch is going to need quite a lot of extra work, the problem is there are so many unpredictable ways an order can be updated so it's hard to catch all cases.

Wondering did people implemented latest patch and how does it work for them? Comments?

Yep I did and found an issue, fixed with the attached patch.
Adding a condition in $conditions['_lock'] caused the DrupalDefaultEntityController:load() to load all the line items stored in the db (causing the memory being completely eaten) on each page call because of
if ($ids === FALSE || $ids || $revision_id || (($conditions && !$passed_ids) in (includes/entity.inc:187) (I noticed it only for line item, but I guess all Commerce entities are affected)

I've updated the DrupalCommerceEntityController::load() method by the complete DrupalDefaultEntityController, but adding an extra condition in the if that checks if _lock is the only key in the $conditions array.

We're starting to see lock_wait a lot now as well, and it makes me wonder if this could cause other issues such as line items becoming out of sync and discounts/coupons being erroneously removed from an order? Interested to know what kinds of side effects there are from simply removing locking for order entities. Thanks for the work on this issue - as soon as a patch is determined to be "ready for production" I will test it on our production server.

So far it seems like simply loading a Commerce Order that allows its line items to be refreshed make the system prone to lots of lock_wait, sometimes in the order of 15 seconds.

I'm still digging in, but it seems related to this issue I created earlier: #2384331: Do not look for Checkout panes on non-Checkout pages - see my latest comment .. and it is exacerbated with the new Memcache Stampede Protection (which we need to avoid other issues!).

If you think this patch could help, I'd definitely like to test it.

@nanak: Do you think you could re-roll this patch with --relative?

@bojanz the improvements are huge with the locking, but my number of queries jumped too and made it slower.
So the locking query was eating up 318ms and only called 5 times and after it was 1ms, but there seemed to be more calls at 19 times and over all the those extra transactions added up to a slower page load from anywhere between 100ms to 1s. So I'm seeing slower results kinda of like @torgosPizza. There is also a bit more on the unserializing for some reason...

@nanak would you mind putting an interdiff on there so we can see what you changed from #42 It would really save us on review time when trying to decide if your changes are all ok. If you haven't made one before it's just a diff between the first patch and the second, here's instructions https://www.drupal.org/documentation/git/interdiff

Also I had a large order, 42 line items in the cart, 2 discounts, 5 product variation types

@nanak's patch has a slight improvement but not much, the biggest drawback I see is there are >1000 more calls to DrupalCommerceEntityController::load

Ok using @nanak's patch here's an unscientific performance test.

This is on the cart page with Devel SQL query log (refreshed the page 3 times to warm the caches after swapping patches, probably not needed but did it anyways:P):

No patch:

Executed 364 queries in 212.3 ms.
Page execution time was 3720.5 ms.
Memory used at: devel_boot()=2.9 MB, devel_shutdown()=59.44 MB, PHP peak=60.75 MB.

With @nanak's patch: #44

Executed 395 queries in 225.44 ms.
Page execution time was 3683.17 ms.
Memory used at: devel_boot()=2.9 MB, devel_shutdown()=59.68 MB, PHP peak=60.75 MB.

With @bojanz's patch in #42

Executed 395 queries in 226.07 ms
Page execution time was 3708.6 ms.
Memory used at: devel_boot()=2.9 MB, devel_shutdown()=59.68 MB, PHP peak=60.75 MB.

Looks like there are 15 more DrupalEntityController::load calls that generate SQL queries.
3 extra calls to DrupalCommerceEntityController::buildQuery (but they seem like they just moved around a bit in their execution)
1 extra call to field_sql_storage_field_storage_load

I am bit confused, first you said over 1000, now you say there are 15 and 3 extra calls. What is the conclusion. No difference with performance, but better with deadlocking?

Those were different tests, granted that technique is horrible though the second test is a bit more consistent.

The first one was using blackfire.io and comparing the profiles. The second was just devel query output in the footer.

But with the second was more careful to make sure the results were similar after cache warmed.

In general there were more queries run and no less SELECT ... FOR UPDATE calls but more which I don't think is helping the problem.

I should run xhprofkit on this to get super accurate results, but I'm quite sure I'd see any better results with these patches.

Here's the patch made relative and its interdiff

I have to say I am confused with this :) tempted to apply it to production where order locking happens 2-4 a day but not sure will it bring more problems or help.

It's already on production on my side on a large corpo site, it did not introduced more issues (but do not fix them all of course)

Nanak, can you do your patches relative to the commerce module only ?

You make them relative to a profile install. I have to rewrite all paths before apply.

Thanks.

@enorniel: the patch #54 have been made relative, do not use #44

Can we get an update what's the current status and how we can load an order without locking until there is a final solution? This is really major but things don't seem to move forward. What can we do?

anyone with luck solving this issue? thanks.

Tagging for Sprint to try and at least get some eyes on it.

Using the newest #54 patch I am able to reproduce @joelpittet's results. Using a simple view with 1 order I get the following without and with the patch.

	                   Queries 	Execute Time	 For Update Calls
Without patch	       57	               ~4.9ms	       2
With patch	       72	               ~6.5ms	       9

We also tested the patch on a larger production site of ours and noticed slight speed decreases as well. I'm not sure what the problem is yet, but testing show the new patch is clearly worse.

Ok, more info. Turns out I had missed my views cache clear on that test and the performance is actually identical, except with no more "for update" calls, which is what this patch intends to do so makes sense.

	                   Queries 	Execute Time	 For Update Calls
Without patch	       57	               ~4.9ms	       2
With patch	       57	               ~4.9ms	       0

This mistake actually proved useful, at it turns out that this patch is better if locking is not required, but worse if it is. I did some further investigation and this appears to be the offending piece.

+++ b/includes/commerce.controller.inc
@@ -21,14 +44,157 @@ class DrupalCommerceEntityController extends DrupalDefaultEntityController imple
+    // If locking has been required, then bypass the internal cache for any
+    // entities that are not already locked.
+    if (!empty($conditions['_lock']) && $this->requiresLocking()) {
+      foreach (array_diff($ids, $this->lockedEntities) as $id) {
+        unset($this->entityCache[$id]);
+      }
+    }

The cache gets unset for locking entities, which is not something we previously used to do, so it makes the existing functionality worse and requires more entity loads. I don't understand why this part is here, it doesn't seem like it is needed for this patch as it would be an existing problem with locking. Hopefully someone with more insight can clue me in to what is going on.

I'm wondering if someone is currently testing #54 or if anyone have more details about this issue?

Subscribe

This looks good. There's a handful of coding standard changes which make the patch a bit harder to discern what existing in regards to locking workflow and what was changed, but not horrible.

+++ b/includes/commerce.controller.inc
@@ -21,14 +44,157 @@ class DrupalCommerceEntityController extends DrupalDefaultEntityController imple
+    // If locking has been required, then bypass the internal cache for any
+    // entities that are not already locked.
+    if (!empty($conditions['_lock']) && $this->requiresLocking()) {
+      foreach (array_diff($ids, $this->lockedEntities) as $id) {
+        unset($this->entityCache[$id]);
+      }
+    }

I wonder if this tidbit right here could help mitigate issues we have with orders in AJAX because we're always hitting entity cache. When loading an order that needs a lock we can then bypass.

This looks really promising, but just needs some manual testing and verification.

@mglaman the problem with that code snippet is it doesn't work right though, it bypasses caching way too often and actually makes this patch perform worse instead of better. I haven't got a chance to dissect exactly what is wrong with it though, hopefully this weekend.

Aight, so I got this bastard figured out:

This little bit is at fault here, at first glance it looks fine, but lockedEntities is actually array_flip()'ed when it is set, so this never matches, thus causing us to uncache the entity each time we call load, which on my current test page resulted 10 addition queries each page load.

+      foreach (array_diff($ids, $this->lockedEntities) as $id) {
+        unset($this->entityCache[$id]);
+      }

Updated patch is attached, I have tested it a bunch and it now seems to have the expected effects, speed remains the same but FOR UPDATES are removed in some instances. I think there are further instances we could remove locking, but that is for another issue.

Interdiff as per request

Whao great work! Do you have performance results like in #63?
I'll have a look at it soon on course. At first glance the patch looks good.

Let's get some more feedback on this. We should fix this as soon as ever possible. Thank you very much!!

@anybody yes, performance is the same, I don't have the same testing environment setup, but here are some stats on my current testing setup. I included a bit more detail this time, as you can see all the queries numbers and times remain the same and only the FOR UPDATEs decrease. I also included the edit page to confirm it remains unchanged as it should still be using a FOR UPDATE.

Without Patch			
URL                              # of Queries      Query Time       FOR UPDATEs
admin/commerce/orders                182	           20ms	          1
admin/commerce/orders/1              187	           16ms	          1
admin/commerce/orders/1/edit         139	           16ms	          1
			
With Patch			
URL	                           # of Queries      Query Time     FOR UPDATEs	
admin/commerce/orders                182	           20ms	          0
admin/commerce/orders/1              187	           16ms	          0
admin/commerce/orders/1/edit         139	           16ms	          1

We just had a shippable order take 50 seconds to complete a SQL query. It happened while calculating shipping line items, so I'm not sure if there is something in the Shipping module that is causing a new bottleneck or what. Thoughts?

This patch would only effect orders themselves, it doesn't change anything for line items. Also it only fixes some of the locking, there are still lots of instances where locking occurs, but those should probably be handled by future patches.

Do you happen to know what the query was? I can look into it but it should probably be split off into a separate issue.

+    // Lock by default if the caller didn't indicate a preference.
+    $conditions += array('_lock' => TRUE);

^ You can set this directly to $this->requiresLocking() so that you don't have to recheck that everytime down the stack... something like:

$conditions += array('_lock' => TRUE);
$conditions["_lock"] = $this->requiresLocking() && $conditions["_lock"];

Also, we do control the stack from there, so let's pass this condition as a protected property to ->buildQuery() and ->cacheGet() so that we don't have to hack around $conditions everywhere.

+    // If locking has been required, then bypass the internal cache for any
+    // entities that are not already locked.
+    if (!empty($conditions['_lock']) && $this->requiresLocking()) {
+      foreach (array_diff(array_flip($ids), $this->lockedEntities) as $id) {
+        unset($this->entityCache[$id]);
+      }
+    }

^ I assume this means to use array_diff_key(). Please add an explicit test case for this branch.

+    $entities = array();
+
+    // Revisions are not statically cached, and require a different query to
+    // other conditions, so separate the revision id into its own variable.
+    if ($this->revisionKey && isset($conditions[$this->revisionKey])) {
[... a ton more code...]

^ If we start passing around the lock property as a protected property, we can remove this nearly verbatim copy of the ancestor here, no?

       // Maintain the list of locked entities and release the lock if possible.
-      unset($this->lockedEntities[$entity->{$this->idKey}]);
-      $this->releaseLock();
+      if ($this->requiresLocking()) {
+        unset($this->lockedEntities[$entity->{$this->idKey}]);
+        $this->releaseLock();
+      }

^ Given that ->requiresLocking() can change value between load and save, please just do that unconditionally (i.e. just remove this hunk).

Hey Damien, thanks for all the good feedback!

#1 - With changing to a protected property, I can just include most of this in requriesLocking
#2 - Changed to protected property, I was able to clean things up a LOT because of this
#3 - You are correct sir, also the unset was not correctly using the key, so I fixed that as well. Also added some isCached checking to the tests to confirm the locking is not breaking caching
#4 - Removed 90% of it and replaced with parent::load, only the custom bit at the start is now there
#5 - Removed

I posted an interdiff, but tbh it is probably just easier to look at the new patch since lots was removed.

@smccabe: Thanks, indeed there seems to be some weirdness around orders being saved/loaded any time a line item is added to an order; in this case it was a shipping line item. I'll have to dig in a bit further with XHprof (see my New Relic screengrab below):

I'll update my patch with the latest too just to keep parity (and to continue testing)!

Updated patch with change to use a single transaction object for loading and saving, instead of 2 separate objects. Also separated clearing cache and releasing locks, since you may want to do one and not the other, for example loading an original from the database for comparison.

Turns out resetCache was used in a few tests as an unlock hack, replaced with releaseLocking() function.

Green! That is awesome. Fantastic work, I'll take a look and test asap.

Additional performance testing data.

If everything is running quickly, locking is unlikely to become an issue as the locks will clear out quicker than the page loads and not become an issue. I ran a simple test at 30 concurrency both with and without the patch and times were essentially the same, about 10 trans/sec on my little test server.

To simulate some less than idea circumstances where locking would be more problematic and test the patch, I added a 1sec delay into the page generation during the "locked" phase. Without the patch applied this brings things essentially to a halt, I get 0.04 trans/sec and almost everything ends in a timeout. With the patch enabled I get roughly the same at just under 10 trans/sec, although with an expected ~1 second longer response time.

                                 trans/sec
7.x-1.x - No Delay                 ~10/sec
W/Patch - No Delay                 ~10/sec
7.x-1.x - 1sec Delay               ~0/sec
W/Patch - 1sec Delay               ~10/sec

Testing command for those interested:

siege -c 30 -b -t120s -v --header="Cookie: SESSa84...=wREq..." drupalvm.dev/admin/commerce/orders

I have pushed the new locking code to Production, and so far so good. In Dev I was noticing our load time for Cart page cut in half (1.5 seconds from ~3 seconds) using identical code except one with locking and one without. So I'm cautiously optimistic that we'll see some good improvements on production as well. I'll be watching in New Relic as we push a new product release later today!

@smccabe and I were discussing in IRC, it seems that the commerce_line_item_delete() function does not pass its transactions back to its subsequent load() and save() calls, resulting in potential locked tables. We saw this in a 15 second SELECT ... FOR UPDATE query when a user clicked "Remove" on an item in /cart.

Hoping to see a new version of the patch later today, and once we have it I'll put that up on production as well.

   public function save($order, DatabaseTransaction $transaction = NULL) {
-    if (!isset($transaction)) {
-      $transaction = db_transaction();
+    //we don't really ever want to have 2 transactions, should we really even accept passed in transactions? or at least flush our internal one immediately?
+    if (isset($transaction)) {
+      $this->controllerTransaction = $transaction;
+    }
+
+    //check if we already have a transaction, else create one
+    if(!isset($this->controllerTransaction)){
+      $this->controllerTransaction = db_transaction();
       $started_transaction = TRUE;
     }

^ This is not a good idea. Transactions are nested, and inner transactions are simply SAVEPOINTs. In case this save operation fails, you want only that operation to be rolled-back, not the whole transaction stack. (Plus, the database layer will freak-out if you try to rollback out of order.)

@torgosPizza: I don't understand what you are describing. From the look of it, it seems like *something else* is causing the lock to be held. A single request cannot by definition lock itself. You should track what is locking... on production systems, admin pages and background processes (like advanced queue, etc.) are common culprit.

@Damien Tournoud: It was definitely a SELECT ... FOR UPDATE in the commerce_order table after a customer deleted an item from their cart:

Based on what you're saying in #85 perhaps I'll revert our lock patch for now, but I will admit that the locking fixes have shown performance improvements and (so far) no side effects that I can tell.

Hey @Damien, Yah I spent all last night learning about Drupal's transactions, takes a bit cause they are not really documented. I now know about how it sets an initial transaction, then everything else within that is just save points, when you pass a name it is just for save points, etc. etc. What I was trying to do with that piece of code is make sure both transactions close, so it worked by making sure everything got closed off, but as you pointed out it did so incorrectly. I will take another go at adjusting the patch to do what I intended correctly.

@torgosPizza, I looked into delete line item stuff a bunch last night, it does lock for a much longer time than loads or adds, but @Damien is correct in that a single thread can't conflict with itself since it just uses savepoints within the same transactions. What is somewhat of a problem is that order_load usually starts a transaction early on, so a transactions is held for most of the pageload. By itself though, this is ok, as this should be the only instance of this order being accessed, so the lock should not cause issues. There has to be something else that is also locking the order to conflict, admin views, cron jobs, ajax requests, multiple tabs open etc. The problem presents itself on line_item_delete likely because that holds a lock for longer than some other actions, so it just has a higher chance of conflicting with some other process that is running and holding a lock.

I know one another site we have locking issues on, here are some big admin views and reports that would lock everything up and cause huge deadlocking issues. Do you have anything that runs and would likely be hitting orders that are still in cart status?

Ah okay. That does make sense. I'll wrack my brain to see if I can figure out what else might be causing this.

@Damien, updated patch put up, I just removed that piece as on further inspections no replacement was needed, it succeeded only in messing with the rollback. Also a couple of other small fixes added, easy to see in the interdiff.

@smccabe
1) #78 patch has unfinished transaction related changes at 'modules/order/includes/commerce_order.controller.inc' file.
See '->rollback()' method call which must be called as $this->controllerTransaction->rollback() in your logic.
2) I think this part of the patch could be ommited as new transaction opening will just add 'SAVEPOINT' at DB level.
3) Each commerce entity type controller uses identical transaction behaviour. Just keep it similar.
4) 'commerce_entitycache' module requires to be patched as well to be compatible with your changes. Per every 'includes/commerce_entitycache.*.inc' file:

public function load($ids = array(), $conditions = array()) {
+    unset($conditions['_lock']);
     return EntityCacheControllerHelper::entityCacheLoad($this, $ids, $conditions);
   }

@vadym.kononenko: if commerce_entitycache replaces the commerce_order entity controller, it probably doesn't provide locking, which is *extremely* unsafe. Don't use it.

@vadym.kononenko, patch 78 is outdated and that controller stuff is removed as per #90, transactionController is used for locking and not for rollbacks.

@vadym.kononenko @Damien Tournoud, looking over commerce_entitycache, it calls the buildQuery parent controller function still, so on clean loads locking would still get turned on. The big problem seems to be that since it overrides the load function, it doesn't call this:

    // If locking has been required, then bypass the internal cache for any
    // entities that are not already locked.
    if ($this->requiresLocking()) {
      foreach (array_diff_key(array_flip($ids), $this->lockedEntities) as $id => $value) {
        unset($this->entityCache[$id]);
      }
    }

Which means that if an order gets loaded and then needs to be locked, it will already be cached and not be cleared out and locked and instead the unlocked version from cache will be used. There may be other problems as well, as it replaces a lot of code and I only did a quick scan, but this was the obvious problem that jumped out at me related to this patch.

Additionally, since commerce_entitycache can cache orders across page loads, it seems like it would stomp on locking completely, since commerce's locking has no concept of locking once the database connection has been closed. Commerce will assume it is receiving a locked order when it could be receiving from cache. I would agree with Damien that commerce_entitycache disregards order locking completely.

Damien, correct me if I said anything stupid above plz :)

@smccabe, thank you for your attention and explanation about 'commerce_entitycache'.

As with the previous patches this one works very well for me. I did notice an almost immediate improvement to checkout performance. I'd set this to RTBC but I figure others may want to look at this and test more deeply, but for us - using this in production - the results have been very positive so far.

1. +++ b/modules/order/commerce_order.module
   @@ -774,6 +784,8 @@ function commerce_order_load_by_number($order_number) {
   + * @param $lock
   + *   Whether to lock the loaded order.
   ...
   @@ -782,6 +794,32 @@ function commerce_order_load_multiple($order_ids = array(), $conditions = array(
   

   Was this param to be added missed or is it an accident?

2. +++ b/modules/order/commerce_order.module
   @@ -1472,6 +1510,34 @@ function commerce_order_entity_query_alter($query) {
   +  $previous_result = &drupal_static(__FUNCTION__, NULL);
   

   nit: the NULL second argument is the default, not needed.

3. +++ b/modules/order/commerce_order_ui.module
   @@ -35,6 +35,8 @@ function commerce_order_ui_menu() {
   +    'load arguments' => array(FALSE),
   
   +++ b/modules/payment/commerce_payment_ui.module
   @@ -13,6 +13,7 @@ function commerce_payment_ui_menu() {
   +    'load arguments' => array(TRUE),
   

   Is this an undocumented trick with 'load arguments'? The docs don't seem to mention this is how it works...
   https://api.drupal.org/api/drupal/developer!hooks!core.php/function/hook...

Sorry should have changed the status on #97

#1 - parameter was added, then change to protected property, I forgot to remove comment, fixed
#2 - fixed
#3 - this was done before I started working on the patch, but yes it basically passes that parameter along to the automatic load function coming from the %order option. ctrl+f for "load arguments" here, it is down in the docs a bit https://api.drupal.org/api/drupal/modules!system!system.api.php/function...

Thanks @smccabe re #3 ah that's my bad I didn't know how that magic offset 'load arguments' worked.

Using the siege command from #82 I got the following with a production site.

There are 2200 orders and showing 50 at a time in the view. Turn on order viewing permissions for anonymous so I didn't have to pass in the session header (cheating). xdebug off.

siege -c 30 -b -t120s -v http://eminence.dev/admin/commerce/orders

Without Patch
=============
Transactions:             140 hits
Availability:             100.00 %
Elapsed time:             119.18 secs
Data transferred:         18.05 MB
Response time:            23.29 secs
Transaction rate:         1.17 trans/sec
Throughput:               0.15 MB/sec
Concurrency:              27.36
Successful transactions:  140
Failed transactions:      0
Longest transaction:      35.12
Shortest transaction:     5.42

With Patch
==========
Transactions:             195 hits
Availability:             100.00 %
Elapsed time:             119.60 secs
Data transferred:         25.13 MB
Response time:            16.83 secs
Transaction rate:         1.63 trans/sec
Throughput:               0.21 MB/sec
Concurrency:              27.43
Successful transactions:  195
Failed transactions:      0
Longest transaction:      23.41
Shortest transaction:     10.32

Though maybe not amazing (and I probably have other problems I should look at here). This seems to be getting nearly 1.4 times the transactions in the same amount of time. And I ran this a few times with similar results.

EDIT: FYI those are apache transactions not SQL transactions (because we are talking about transactions in this issue)

Ran patch against CK2 Behat suite: https://travis-ci.org/mglaman/commerce_kickstart/builds/100868406. Green

I'm going to test the latest patch against version 7.11.
I've attached the relevant backport patch.

You can out "do-not-test" in file name so that it doesn't get run by DrupalCI

I've been getting lock timeouts this weekend, may be just me and they seem sporadic but thought I'd mention it. Mostly after I did some updates to rules.

@joelpittet, with or without patch?

With the patch

So far we haven't seen any issues like this in production, but I also have these patches applied: #2303721: make variable_initialize safe when the lock system fails and #1191290: system_cron() should not run hook_flush_caches(), but use a cached version. Might take a look (especially if you use Memcache).

The lock_wait issues we were seeing were mostly happening during the variable_initialize function during bootstrap. It would show up sometimes during PUT requests in Services, and sometimes in the checkout_router, which is what made it hard to track down. I think it was just part of cron/time-based cache rebuilds.

Between those two patches and the order locking patch the most time-consuming calls in our site now on the back-end have been all the external API calls. Right now I'm very happy with our performance!

Just to clarify where this is happening, it only seems to be with one of my menu callbacks, also in dc_cart_ajax which is loaded through authcache ajax too and a custom context order load in panels to check if the cart is empty. So at least 2 of those I'll be able to change to load without locking and see if it helps.

Not removing my RTBC, because this patch doesn't fix all locking, it just gives us the tools to do so, so I've tried to change my custom stuff to unlocked read-only requests.

@joelpittet: Intriguing! I'd love to learn more about your custom code, since that optimization might be something I could use on our site as well.

Oddly enough two days ago we had an order that got stuck at "Checkout: Complete" and did not move into "Completed" (which happens with Rules). Looking at syslog, the only thing I can find is a deadlock error when attempting to get a lock during the INSERT for field_revision_commerce_customer_billing.

Jan 17 15:56:25 10.45.15.248 rifftrax7: https://www.rifftrax.com|1453046185|php|https://www.rifftrax.com/checkout/.../paypal_ec|https://www.rifftrax.com/checkout/.../review|||PDOException: SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction: INSERT INTO {field_revision_commerce_customer_billing} (entity_type, entity_id, revision_id, bundle, delta, language, commerce_customer_billing_profile_id) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6); Array

So far that's the only issue we have had - haven't had any other orders stuck since the beginning of the year, which is definitely a good thing. But obviously I'm curious as to what could've caused the deadlock.

@torgosPizza: are you running in READ COMMITTED transaction isolation mode? This sounds to me like a deadlock that really should not happen.

Most likely, there is something on your site that is causing these concurrency issues (long running background processing? or drush command lines? weird things happening in the admin UI? API callbacks from external systems?). We have had very large Drupal Commerce websites in production for years without seeing this type of issues.

@Damien: You're right, this is probably unrelated. I think there are some performance improvements we can make on the hosting side. Thanks!

Ok this caused me some grief lately so I'm knocking back to NW, sorry @smccabe.

So commerce discount was creating new line items, then trying to read them and the new line item was in entityCache but not in the database yet because it was still in a nested transaction. So when reading from entityCache it would be fine but as soon as something asked for it from the DB it would bail saying 'bundle missing in entity'. This caused me 2-3 days of entity metadata wrapper debugging:(. Thought it was an AJAX related race condition but nope, also thought it was entityCache problem (kinda was because it was made available to entityCache before it was saved).

The scenario: I had a views_handler_field_field::post_execute() loading the order. Which is a views listing in a panel. That started the transaction DrupalCommerceEntityController::buildQuery, and never released it.

I couldn't seem to figure out how or where it should've released completed the transaction, ping me on IRC to discuss further.

I think it may be my version of redis/memcache, I'm using apdqc which is supposed to help deadlocks entirely.
#2657922: Commerce - module incompatibilities

If I add this patch in, get my errored up page again it still fails, when I turn off that that cachebackened it does not fail.

My related patch #2657792: Cart order totals incorrect after updating line item quanity needs to be fixed still I think but it doesn't affect the outcome of the bundle missing fatal error, though it is also brought to light from apdqc's transaction handling.

This is gonna need a bit of comment clean-up and potential refactoring before I commit it, but I'm committing myself to do it. Basically, some docblocks have copypasta errors, and I'm not convinced by the names of the protected properties and methods (e.g. requestLocking, given the fact that we actually default that to TRUE and disable locking per the $conditions array, it makes more sense to talk about releasing locks not locking, etc.).

Otherwise, the patch looks good to me. I don't have time to do the refactor right now but will try to get 'er done and re-tested this weekend.

After apply patch #99 i still get bellow error

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] => 3669 ) in DrupalDefaultEntityController->load() (line 198 of includes/entity.inc). Backtrace:

PDOStatement->execute(Array) database.inc:2171
DatabaseStatementBase->execute(Array, Array) database.inc:683
DatabaseConnection->query('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, Array) select.inc:1265
SelectQuery->execute() entity.inc:198
DrupalDefaultEntityController->load(Array, Array) commerce.controller.inc:148
DrupalCommerceEntityController->load(Array, Array) common.inc:7978
entity_load('commerce_order', Array, Array, ) commerce_order.module:792
commerce_order_load_multiple(Array, Array, ) commerce_order.module:756
commerce_order_load('3669') commerce_cart.module:813
commerce_cart_order_load('174') commerce_cart.module:548
commerce_cart_commerce_product_calculate_sell_price_line_item_alter(Object, NULL, NULL, NULL) module.inc:1157
drupal_alter('commerce_product_calculate_sell_price_line_item', Object) commerce_product_pricing.module:118
commerce_product_calculate_sell_price(Object) commerce_cart.module:2214
commerce_cart_add_to_cart_form(Array, Array, Object, 0, Array) 
call_user_func_array('commerce_cart_add_to_cart_form', Array) form.inc:841
drupal_retrieve_form('commerce_cart_add_to_cart_form_76_80_229', Array) form.inc:350
drupal_build_form('commerce_cart_add_to_cart_form_76_80_229', Array) form.inc:130
drupal_get_form('commerce_cart_add_to_cart_form_76_80_229', Object, 0, Array) commerce_cart.module:2695
commerce_cart_field_attach_view_alter(Array, Array, NULL, NULL) module.inc:1157
drupal_alter('field_attach_view', Array, Array) field.attach.inc:1244
field_attach_view('node', Object, 'teaser', 'fa') node.module:1415
node_build_content(Object, 'teaser', 'fa') node.module:1336
node_view(Object, 'teaser', NULL) node.module:2669
node_view_multiple(Array) node.module:2708
node_page_default() 
call_user_func_array('node_page_default', Array) menu.inc:527
menu_execute_active_handler() index.php:21

Hey @arefen, it won't eliminate all order locking, as commerce still uses pessimistic order locking. All it does is add a method of loading orders without locking and default some instances to non-locking loading. If you have locking views or other types of batch jobs can still cause pretty bad locking. Do you happen to know what is running when you get locking?

Sooooo... the cleanup sort of turned into a refactor of all the set/request/require locking methods to clean up the duplication and hopefully make it a lot cleaner.

Scaled back the refactoring and this patch is just naming and commenting cleanup. With the weird way we're sort of cheating when using conditions, a further refactor isn't possible without creating the possibility of defaulting to unlocked by mistake.

Here's an interdiff between #99 and #124 to see the difference between the patches.

So, two questions:

1. Do we really need the 'load arguments' parameters in the event that it's TRUE? That's the default value of the argument in the loader, so just including it for FALSE should suffice, no?
2. commerce_order_views_pre_execute() kind of concerns me, because it disables locking at the controller level but never re-enables it after the View has finished executing. What if there is an operation on the page after an order View is built that actually should result in a locked order load?

Additionally, I'm not sure certain comments add up. In the docs for $requestSkipLocking it says,

  /**
   * Stores whether a request for skipping locking has been set.
   * If locking has been requested as well it will take preference and the entity
   * load will default to locking
   */
  protected $requestSkipLocking = FALSE;

However, when I look at requireLocking(), I see:

  protected function requireLocking() {
    $enabled = isset($this->entityInfo['locking mode']) && $this->entityInfo['locking mode'] == 'pessimistic';
    $not_skipped = empty($this->requestSkipLocking);

    return $enabled && $not_skipped && $this->requestLocking;
  }

Note that locking will not be required even if pessimistic locking is enabled for an entity type and locking was requested if $requestSkipLocking has been set to TRUE. This seems to contradict the earlier statement that if locking is requested, it will override any attempt to skip locking.

That said, this may just be a documentation problem, not a logic problem. If I get a confirmation one way or the other, I can re-roll. Would still need to address points 1 and 2 above and fix some copypasta like this:

  /**
   * Implements DrupalCommerceEntityControllerInterface::isLocked().
   */
  public function isCached($entity) {
    return isset($this->entityCache[$entity->{$this->idKey}]);
  }

(Note the docblock vs. function name.)

Also running into this issue. I'm definitely entering this conversation late, but I gotta point out that database locking via transactions is a really really terrible idea. If a process dies/is killed/etc the whole table gets locked indefinitely. You should have to explicitly specify when you're going to update an order.

In our case we have a background task that iterates over the list of orders in certain states. It loads an order, runs a buncha rules and calls some external services that might take maybe 1s real time (in a background task), then updates the order (or not) and bails. it's continually fetching new locks, so it can basically block almost anything on the site from showing up.

I can't imagine an architectural decision that would have a worse impact on concurrency

Going to take a look at this. Did some discussion with rszrama and we think this should be split: this patch implements ability to load an order as "read only" without locking, or with locking. Then a follow up to alter the hook menu and such.

This way we can get the implementation and test in.

Here is an updated patch. I fixed up a bunch of the doc blocks. I also expanded the tests and broke the methods up -- to test locking, releasing locks, and caching.

This patch also removes any adjustments to implement this measure. That makes this patch, and issue, an API change to allow loading a non-locked order for read-only purposes. We can then open a follow up to make sure that Views and the menu system do not erroneously lock orders.

+++ b/tests/commerce_base.test
@@ -11,6 +11,8 @@
 
+  protected $profile = 'minimal';
+

Whoopsies.

Looks like we need to implement the load functions.

Adding torgosPizza and joelpittet to credits, thanks for all the testing and performance tests.

Nice, thanks!

@fearlsgroove: We were getting locking errors way more than we should have prior to this patch; but with the patch and with moving to READ-COMMITTED transaction mode in MySQL, the error hasn't been seen in quite some time.

It'd be great if you could help us test the patch and any subsequent follow-ups written to implement an improved locking mechanism to see if your issues go away as well. That sort of data is super valuable to making sure our Commerce sites are as performant as possible.

Ran into an issue with the way things are currently implemented and suggested in the patch.

1) The order is loaded by us locked
2) The cart load fires and refreshes the cart
3) At the end the order is saved by the cart module
4) After order_load our API tells us the order is unlocked

Cart refresh will essentially nullify the lock almost immediately by saving the order.

Disregard #137. Here is a bump of #134 to add a releaseLock($entity); method and a test for cart. It proves that saving an order will remove the lock the order was loaded with.

I posted a bad patch. I'll have an updated one later.

This should be the proper patch.

Updated patch, using un-locked loading for orders in:

commerce_order_form_commerce_customer_customer_profile_delete_form_alter()
and
commerce_order_handler_area_order_total views handler.

Still not able to understand why test as failing with the message:

exception: [Notice] Line 100 of sites/all/modules/commerce/includes/commerce.controller.inc:
Undefined property: CommerceOrderEntityController::$controllerTransaction

There's a new patch so setting to Needs Review.

Improved patch: I defined a custom `CommerceOrderViewsQuery` to allow admins to selectively define if orders should be locked or not during the SQL execution in Views (just use the View -> Advanced -> Query settings).

The patch also extends the `views_handler_field_field` used by the `commerce_price_handler_field_commerce_price` handler to respect the CommerceOrderViewsQuery locking setting.

The attached interdiff is relate to the patch in comment in #2240427-146: Improve commerce entities locking.

Awesome patch! Marking it for testing

Updated patch:

• moved newly added CommerceViewsQuery to commerce module itself as it could be used by sub modules (commerce_order, commerce_product, commerce_line_item).
• Refactored settings added in patch #149
• Using CommerceViewsQuery in Products and LineItem
• Added "$lock" parameter to Product and LineItem `*_load()` ad `*_load_multiple()` functions.

1. +++ b/includes/views/commerce_views_query.inc
   @@ -0,0 +1,115 @@
   +   * Returns the according entity objects for the given query results.
   

   The word 'according' is not appropriate here and is also superfluous.

2. +++ b/includes/views/commerce_views_query.inc
   @@ -0,0 +1,115 @@
   +    // Bail out if the table has not specified the according entity-type.
   

   The word 'according' is not appropriate here and is also superfluous.

3. +++ b/includes/views/commerce_views_query.inc
   @@ -0,0 +1,115 @@
   +   * Mimics the same behaviour of views_plugin_query, but allows admins to define
   

   The word 'same' is not needed here.

@czigor feel free to make those changes with an interdiff. Setting to needs review so Testbot picks it up.

Updated patch with @czigor comments. I also had a chat with him and I fixed few more issues he discovered.
Interdiff against patch in #143

Go testbot, go!

Updated patch, fixed wrong testCartRefreshOrderLock() assertions.

Updated patch, reworked the locking and caching handling functions

Locking fixes (part 2)

Removed latest 2 patches (160 and 162) as over-complicated code was added.

This patch is a followup of #159 tries to add the locking/unlocking feature to the Cart sub-module

Further analyzed where the commerce_order_load() is used.
I spotted some places where the $order is never saved after loading (with locking enabled), I then changed such invocations with a "non-locking" call.

+++ b/modules/cart/commerce_cart.module
@@ -605,22 +606,22 @@ function commerce_cart_user_login(&$edit, $account) {
+        commerce_order_save($order);

The whole commerce_cart_user_login() part seems to me a completely unrelated performance fix. Since the patch is big enough already can we fix this in a separate issue?

@czigor: the commerce_cart_user_login() is slightly related to the performance itself.
The current implementation is querying all the orders (and locking all of them) of a specific user, subsequently the order email is checked, and only a subset of those orders saved. The non-altered orders would stay locked.

The patch fixes that.

Improved commerce_order loading and locking in "Checkout" submodule

Thanks for all the work on this, but I do want to remind that we made #2713193: Implement loading orders without locking so the patch is easier to review.

+++ b/modules/checkout/commerce_checkout.module
@@ -15,6 +15,7 @@ function commerce_checkout_menu() {
+    'load arguments' => array(FALSE),

@@ -23,6 +24,7 @@ function commerce_checkout_menu() {
+    'load arguments' => array(FALSE),

Shouldn't checkout always load a locked order? It's going to be modified.

+++ b/modules/checkout/includes/commerce_checkout.pages.inc
@@ -351,9 +351,6 @@ function commerce_checkout_form_validate($form, &$form_state) {
-  // Load a fresh copy of the order stored in the form.
-  $order = commerce_order_load($form_state['order']->order_id);
-

@@ -368,6 +365,9 @@ function commerce_checkout_form_submit($form, &$form_state) {
+      // Load a fresh copy of the order stored in the form.
+      $order = commerce_order_load($form_state['order']->order_id);
+

Why was this moved? Just to prevent an order load if there is not a next page? Which technically that probably doesn't run (You can't actually submit the last page of checkout.)

@mglaman @thePanz is away, but I can answer:
1. It's enough to load the orders lockingly in the checkout pane's submit callback.
2. Yes, the reason of moving that piece of code was to prevent another locking order load.

We are getting PDOExceptions due to locked orders. It's hard to say where it's exactly coming from so we are trying to convert locking order loads to non-locking ones whenever possible.

Gotcha. Also, have you tried using READ-COMMITTED mode for MySQL? it seems to resolve 90% of the problems.

Shamless plug for PDOExceptions: #2690765: Prevent entity_load_unchanged() from changing orders through order refresh
Also it's blocked on this likely.

Thanks @mglaman, but our MySQL is already using the READ-COMMITTED mode and the error is still thrown.

Just throwing this out there .. database locking to solve this "problem" is nuts. It only applies to the order revision, not line items, payments or any other related objects, and it won't even stop subsequent requests from unintentionally stomping data. Database level locking only makes sense in the context of fetch and update operations, but this is an application level lock. Starting the transaction and the database itself ensure the data for each save is atomic, which seems to be what this naive locking is trying to solve.

Is there a test case where a concurrency issue can occur anywhere? In addition to being (what seems to me to be) conceptually wrong, I don't even think it's solving a real problem that actually occurs, but instead is attempting to circumvent a perceived or potential problem (that isn't in fact valid).

This change just makes the commerce 1.x api even less understandable as a workaround to the workaround that doesn't need to be there in the first place.

I'd propose dropping the database locking FOR UPDATE altogether and replacing it with a proper application level lock that would start as soon as the application code determines the order needs to be updated in an atomic way (with lock_acquire).

I haven't applied the patch yet, but switching MySQL to READ-COMMITTED seemed to fix most of the errors related to lock wait timeouts, at least on the front end of the store (adding products to cart, checking out).

//added to settings.php
$databases['default']['default']['init_commands'] = array(
    'isolation' => "SET SESSION tx_isolation='READ-COMMITTED'",
);

However I'm still having lock wait timeout issues with Views used by Backoffice Orders and Reporting:

• https://www.example.com/admin/commerce/orders
• https://www.example.com/admin/commerce/reports/sales/daily

admin https://www.example.com/admin/commerce/reports [ip address] 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, :db_condition_placeholder_1, :db_condition_placeholder_2, :db_condition_placeholder_3, :db_condition_placeholder_4, :db_condition_placeholder_5, :db_condition_placeholder_6, :db_condition_placeholder_7, :db_condition_placeholder_8, :db_condition_placeholder_9, :db_condition_placeholder_10, :db_condition_placeholder_11, :db_condition_placeholder_12, :db_condition_placeholder_13, :db_condition_placeholder_14, :db_condition_placeholder_15, :db_condition_placeholder_16, :db_condition_placeholder_17, :db_condition_placeholder_18, :db_condition_placeholder_19, :db_condition_placeholder_20, :db_condition_placeholder_21, :db_condition_placeholder_22, :db_condition_placeholder_23, :db_condition_placeholder_24, :db_condition_placeholder_25, :db_condition_placeholder_26, :db_condition_placeholder_27, :db_condition_placeholder_28, :db_condition_placeholder_29))  FOR UPDATE; Array (
    [:db_condition_placeholder_0] => 67961
    [:db_condition_placeholder_1] => 67941
    [:db_condition_placeholder_2] => 68476
    [:db_condition_placeholder_3] => 68661
    [:db_condition_placeholder_4] => 68436
    [:db_condition_placeholder_5] => 69241
    [:db_condition_placeholder_6] => 69401
    [:db_condition_placeholder_7] => 69556
    [:db_condition_placeholder_8] => 69746
    [:db_condition_placeholder_9] => 69981
    [:db_condition_placeholder_10] => 70381
    [:db_condition_placeholder_11] => 70346
    [:db_condition_placeholder_12] => 70806
    [:db_condition_placeholder_13] => 71111
    [:db_condition_placeholder_14] => 71221
    [:db_condition_placeholder_15] => 71311
    [:db_condition_placeholder_16] => 71591
    [:db_condition_placeholder_17] => 71791
    [:db_condition_placeholder_18] => 72061
    [:db_condition_placeholder_19] => 72546
    [:db_condition_placeholder_20] => 72666
    [:db_condition_placeholder_21] => 72866
    [:db_condition_placeholder_22] => 74376
    [:db_condition_placeholder_23] => 74661
    [:db_condition_placeholder_24] => 74946
    [:db_condition_placeholder_25] => 74961
    [:db_condition_placeholder_26] => 75611
    [:db_condition_placeholder_27] => 75776
    [:db_condition_placeholder_28] => 75916
    [:db_condition_placeholder_29] => 76466
)
 in DrupalDefaultEntityController->load() (line 198 of /mnt/www/html/example/docroot/includes/entity.inc). 

Also while the lock wait timeouts are occurring, cron jobs related to loading orders are negatively affected:

Anonymous http://www.example.com/index.php 127.0.0.1 Exception: exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded; try restarting transaction' in /mnt/www/html/example/docroot/includes/database/database.inc:2171
Stack trace:
#0 /mnt/www/html/example/docroot/includes/database/database.inc(2171): PDOStatement->execute(Array)
#1 /mnt/www/html/example/docroot/includes/database/database.inc(683): DatabaseStatementBase->execute(Array, Array)
#2 /mnt/www/html/example/docroot/includes/database/select.inc(1265): DatabaseConnection->query('SELECT revision...', Array, Array)
#3 /mnt/www/html/example/docroot/includes/entity.inc(198): SelectQuery->execute()
#4 /mnt/www/html/example/docroot/includes/common.inc(7963): DrupalDefaultEntityController->load(Array, Array)
#5 /mnt/www/html/example/docroot/sites/all/modules/contrib/commerce/modules/order/commerce_order.module(782): entity_load('commerce_order', Array, Array, true)
#6 /mnt/www/html/example/docroot/sites/all/modules/custom/example_store/example_store.module(3861): commerce_order_load_multiple(Array, Array, true)
#7 [internal function]: _example_store_external_api_error_cron('execute', 'example_store...')
#8 /mnt/www/html/example/docroot/sites/all/modules/contrib/elysia_cron/elysia_cron.module(1264): call_user_func_array('_example_store...', Array)
#9 /mnt/www/html/example/docroot/sites/all/modules/contrib/elysia_cron/elysia_cron.module(1170): elysia_cron_internal_execute_job('example_store...')
#10 /mnt/www/html/example/docroot/sites/all/modules/contrib/elysia_cron/elysia_cron.drush.inc(153): elysia_cron_run_job('example_store...', false, false, false)
#11 [internal function]: drush_elysia_cron_run_wrapper('run', 'example_store...')
#12 /usr/local/drush6/includes/command.inc(362): call_user_func_array('drush_elysia_cr...', Array)
#13 /usr/local/drush6/includes/command.inc(214): _drush_invoke_hooks(Array, Array)
#14 [internal function]: drush_command('run', 'example_store...')
#15 /usr/local/drush6/includes/command.inc(182): call_user_func_array('drush_command', Array)
#16 /usr/local/drush6/drush.php(92): drush_dispatch(Array)
#17 /usr/local/drush6/drush.php(61): _drush_bootstrap_and_dispatch()
#18 /usr/local/drush6/drush.php(16): drush_main()
#19 {main} 

Are these remaining issues addressed by the patch even after switch to READ-COMMITTED mode?

@wesleymusgrove we were always getting the "locking timeout" errors even with the "READ-COMMIT" setting for MySQL.
I personally give up digging into this issue as I totally agree with fearlsgroove (comment #174).

You can use `hook_entity_info_alter()` to disable the pessimistic locking for Commerce entities, of course, you need to be aware that you can have data loss if, as an example, two orders are saved concurrently.

/**
 * Implements hook_entity_info_alter().
 */
function hook_entity_info_alter(&$entity_info) {
  // Altering Commerce entities to NOT use "pessimistic" locking
  $entity_types = ['commerce_order'];
  foreach ($entity_types as $type) {
    $entity_info[$type]['locking mode'] = NULL;
  }
}

@thePanz thanks for the example! Are you doing this in conjunction with your patch from #168 or only disabling pessimistic locking without the patch?

@wesleymusgrove: no patch applied, just the hook.

Just chiming in that Matt, Bojan, and I were discussing this last week and were also wondering about the use of lock_acquire() for application level locking. Didn't even realize that'd been suggested up above as well. Seems like the most worthy thread to pursue.

The attached patch is a follow-up to Matt's patch from comment #143, it adds a isReadOnly() method to the controller used to track the intent.
Thus, if you disable the order locking by altering the entity info array, you'd still be able to track the read-only orders although we're not preventing the orders to be saved.

+++ b/includes/commerce.controller.inc
@@ -181,6 +206,13 @@
+      if (!$this->requestLocking && $ids) {
+        $this->readOnlyEntities += array_flip($ids);

What if it gets loaded for writing? How to we unset this.

The attached patch implements a different approach.
By default, we assume an order is loaded to perform a "write", if that's the case then we try to lock the order unless the lock is explicitly skipped via the requestSkipLocking() method.

We also skip the cart refresh for read-only orders.