In the DrupalCommerceEntityController::buildQuery() method:

    if (isset($this->entityInfo['locking mode']) && $this->entityInfo['locking mode'] == 'pessimistic') {
      // 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
      // but requires us to start a transaction.
      if (empty($this->controllerTransaction)) {
        $this->controllerTransaction = db_transaction();
      }

      $query->forUpdate();

      // Store the ids of the entities in the lockedEntities array for later
      // tracking, flipped for easier management via unset() below.
      $this->lockedEntities += array_flip($ids);
    }

This also happens when loading entities, DrupalDefaultEntityController::load():

    // Load any remaining entities from the database. This is the case if $ids
    // is set to FALSE (so we load all entities), if there are any ids left to
    // load, if loading a revision, or if $conditions was passed without $ids.
    if ($ids === FALSE || $ids || $revision_id || ($conditions && !$passed_ids)) {
      // Build the query.
      $query = $this->buildQuery($ids, $conditions, $revision_id);
      $queried_entities = $query
        ->execute()
        ->fetchAllAssoc($this->idKey);
    }

Which then directly returns the entities:

    return $entities;

The transaction is never commited because of this: $this->controllerTransaction.

Each time someone loads an entity via a commerce controller, the full page rendering will happen in one unique giant database transaction, with a FOR UPDATE row locking! This may explain some slowness of Commerce.

I experienced it doing heavy import jobs, the first load attempt using any commerce controller will put a transaction right at the beginning and never be released (commit). I was injecting 3000+ line items into 300+ orders, in my case, this doesn't slow things that much because I currently have only one parallel thread on my development box, the horrible side effect is that I cannot watch what's happening for debug purposes while injecting my stuff: I need to be able wether or not I need a transaction, and if I need one, I will place it in my business code myself.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pounard’s picture

Correction: it's only doing this for orders, still happens potentially since the cart is itself an order (I didn't tested it thought).

pounard’s picture

Other correction: the giant transaction will happen only if the order is not saved (which happens most of the case).

pounard’s picture

Other correction: I surely understand why the pessimistic locking serve the Commerce framework here, but by the roleplay of various concurrent modules (rules, migrate, views, etc...) you may end up locking a lock of orders in a lot of threads, pretty much all the time: there is probably a way to avoid this in some cases.

pjcdawkins’s picture

+1

I was surprised to find a "SELECT ... FOR UPDATE" clause when all I wanted to do was view an order (at admin/commerce/orders/%commerce_order).

Because I was running an import on commerce orders at the same time, I got the "lock wait timeout exceeded" error.

But it will slow down a lot of other actions too - anything that loads Commerce entities.

pjcdawkins’s picture

The origin of this bug is #1243306: Implement pessimistic entity locking and enable it for orders.

Basically we need a way to turn OFF "pessimistic locking" when it is not needed. Presumably locking is never needed in read-only situations.

pjcdawkins’s picture

Not sure if this is the right strategy, but with the attached patch, developers could do something like:

/**
 * Load Commerce orders without locking.
 *
 * @param array $order_ids
 *   An array of Commerce order IDs.
 *
 * @return
 *   An array of order objects indexed by order_id.
 */
function custom_quick_load_orders(array $order_ids) {
  $controller = entity_get_controller('commerce_order');
  $controller->preventLocking();
  return $controller->load($order_ids);
}
pjcdawkins’s picture

Status: Active » Needs review
FileSize
1.52 KB

... and here's the patch

pounard’s picture

From what I can remember, you can disable pessimistic locking directly by implementing the hook_entity_info_alter() hook. If I remember well, I did it on the project I was developing when I opened this issue, that's why I never made any patches.

Beyond there is still a conceptual problem, a transaction opened by the business layer should never stall into the darkness of the API, what the business layer opened should always be closed by the business layer. The choise of opening (and therefore closing) or not a transaction should be made in business controllers (i.e. page callbacks) explicitely in very specific occasions, on a per-needed basis; transaction errors should be treated as business errors at the exact very same place. Right now the algorithm is biaised and dangerous because it's implicit and causes problems that remains hidden for people that code on a higher layer: transaction related problems are always very difficult to spot and debug.

pjcdawkins’s picture

I agree. So this bug basically exists because back in 2011 Damien Tournoud asked this question:

Do we want to implement a clean way to load an order without enforcing the lock?

(https://drupal.org/node/1243306#comment-4840424)
... and he received no response. The answer should have been: yes, we do.

My patch above is a workaround.

rszrama’s picture

pjcdawkins’s picture

@rszrama. Thanks

So actually the patch seems unnecessary, and all we need to do in custom code is this:

/**
 * Load a Commerce order without locking.
 */
function custom_quick_load_order(array $order_id) {
  $order = commerce_order_load($order_id);
  entity_get_controller('commerce_order')->resetCache(array($order_id));
  return $order;
}

Right?

rszrama’s picture

Component: Commerce » Developer experience

Basically, but I wouldn't be opposed to adding a utility function to Commerce that does that if we can think up a name for it. It's come up a few times now, and adding such a function would be both an aid to developers and a natural place to add documentation.

pjcdawkins’s picture

commerce_order_load_read_only()
commerce_order_load_multiple_read_only()

Perhaps? Those functions could even mark the order entity object as read only (e.g. $order->read_only), and the controller could then prevent saving. With clear comments to show that those should only be used where you're certain that no writing is needed.

pjcdawkins’s picture

Perhaps you meant something like

function commerce_order_unlock_multiple(array $order_ids) {
  entity_get_controller('commerce_order')->resetCache($order_ids);
}

(I notice that DrupalCommerceEntityController::resetCache() actually wants an array so #11 was wrong - corrrected)

pounard’s picture

I deeply think that the real solution is to provide an helper for the developer to lock explicitely when he needs to, and never lock implicitely.

Well documented, developers will follow it, especially if Commerce code itself does that.

Any solution based on using the resetCache() module will create more performance problems than it will solve real life problems.

pjcdawkins’s picture

Priority: Normal » Major
Status: Needs review » Needs work

OK, I'm not sure of the way forward here. Since this appears to be one of the main performance issues in a Commerce site of mine, bumping to major.

pjcdawkins’s picture

And just to be clear, I completely agree with #15. Entities should be explicitly loaded "for update" only when they need to be.

The Commerce Order UI menu paths user/%user/orders/%commerce_order and admin/commerce/orders/%commerce_order/view should definitely not use pessimistic locking.

pjcdawkins’s picture

a.ross’s picture

Agree with #15. We're now running into the problem that our order list won't load at all because of this. Disabling the pessimistic locking feature solves it, and in fact makes the page lightning fast. I've had to limit the order page to just 2(!) items to sort of make it work for now. Looks like I'll have to disable pessimistic locking altogether until this issue is fixed.

a.ross’s picture

FileSize
6.58 KB

Here is a patch that implements the preventLocking() method for Views. There are probably more places that this needs to be fixed, but I believe Views is the single most important instance.

a.ross’s picture

Status: Needs work » Needs review

By the way, would it make sense to add an api function like commerce_entity_load_readonly(), which does this? Or perhaps commerce_entity_load() with an extra readonly parameter?

a.ross’s picture

Issue summary: View changes

Fixing PHP enclosing tags

shaisachs’s picture

FileSize
2.18 KB

WRT #21 - it seems to me that the default behavior for commerce_entity_load and friends should be readonly, and that pessimistic locking during entity load should be an explicit declaration on the part of the developer - not the reverse. Given that _save and _delete already pessimistically lock, it seems counter-intuitive for _load to grab a pessimistic lock.

I believe that the lock held by commerce_order lock is a particularly serious problem, since commerce_order_load is invoked by menu routing for urls like checkout/$order_id/$page. If the checkout page in question takes a long time to load, the database server can crash as other incoming requests lock up while the locking page churns away. In conditions of high load, that is a very plausible and problematic scenario.

The enclosed patch addresses this issue, at the most local level which seems to make sense to me - i.e. during commerce_order load time - but I also think that a slightly broader patch, affecting say all commerce entities during load time, would make sense as well.

pjcdawkins’s picture

I like the approach and patch in #22 (although personally I wouldn't bother with the method_exists() call)

pjcdawkins’s picture

Status: Needs review » Needs work

For some reason the patch in #22 doesn't prevent locking (for me) when loading orders via Views.

bojanz’s picture

Title: Wrong database transaction handling: stalling transactions » Provide a way to load an order without locking

Closed #1514618: Developer documentation: Loading order entities without considering locking can cause problems and #1320062: SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded as duplicates.

I had to disable order locking completely on marketplace.commerceguys.com because of this.
I spoke to Damien and we agreed that we should have a way to load an order without locking, and we should use that for all views (admin / customer).

EDIT: Here's a more ambitious attempt to handle this without hacks: #2240427: Improve commerce entities locking.

Anybody’s picture

That's great news!! Thanks :)

smokris’s picture

Status: Needs work » Closed (duplicate)

It seems like the plan on #2240427: Improve commerce entities locking is the way forward, so I'm tentatively marking this issue duplicate.

xurizaemon’s picture