4

The magento ORM layer doesn`t seem to handle concurrent saving of the same object gracefully. When saving a model the ORM issues an update sql query that just tests for the primary key.

Lets say you updated the customer_lastname of an order and save it to the database. The ORM layer will issue a query like this:

UPDATE sales_flat_order SET customer_lastname=`Test` WHERE entity_id=3

If another process (e.g. the paypal ipn controller) changed the same order at the same time (e.g. mark the order as payed), its changes might be overwritten.

This seems to me like a major flaw in the design of the ORM. Is there a reason the ORM layer doesn`t test for changes in the database upon saving?

The ORM layer might issue a query like:

UPDATE sales_flat_order SET customer_lastname=`Test` WHERE entity_id=3
       AND state='pending' AND status='pending' …

If the number of altered rows returned by the db is 1 it is certain that the database had not changed since the model was loaded. If the number of altered rows is 0 then the database was changed by another process after the model was loaded and the new changes were not saved and the save method should fail.

Is there a reason Magento doesn`t already do that (perhaps performance)?

Update

When creating a shipment in magento this happens (not only when creating a shipment, but this mechanism can be found throughout the codebase):

  1. Load the order from the database using SELECT into model object
  2. Create shipment model object from order
    • Loads all order items in model objects
    • gets QtyToShip from item model object (qty_ordered - qty_shipped - qty_refunded - qty_canceled)
    • updates qty_shipped in order_item model object
  3. Save in DB
    • START TRANSACTION
    • save shipment model object in db
    • save order model object in db
    • save order_item model objects in db
    • COMMIT

When two processes try to create a shipment for the same order at the same time and process B is able to finish step 3 outlined above while process A is in step 2 then once process A finishes step 3 the changes made to the order_items by process B are overwritten.

4
  • Because concurrent writes to the same model isn't a problem of the ORM, but a feature for its consumer. Only the application layer can determine whether it's crucial and transaction isolation is or rather should be used whenever it applies. – Melvyn Feb 25 '15 at 8:18
  • So, you are saying that it is the application layers responsibility to use this feature, but I do not think the magento ORM provides this feature. The underlying Zend_Db seems to provide that through SELECT ... FOR UPDATE but the orm doesn`t provide a way to use it. – ochnygosch Feb 25 '15 at 8:40
  • Yes, that's one of many short comings. Using some of the more complex database operations requires one to drill into Zend. Is there a specific issue you ran into that prompted this question? – Melvyn Feb 25 '15 at 9:07
  • When two admins added a shipment at the same time, the order item was shipped twice. Before adding an item to a shipment magento checks qty_shipped in sales_flat_order_item. If there is still an item to be shipped magento adds it to the shipment and updates qty_shipped in sales_flat_order_item. In this case one update was overwritten by the other and the same item appeared on two shipments. – ochnygosch Feb 25 '15 at 9:25
1

If you have a test for this, can you run it with READ COMMITTED isolation level? See the discussion here: https://code.djangoproject.com/ticket/13906

And yes, I'm not convinced either the transaction isolation will fix the issue. But with repeatable reads, you cannot solve the problem period. Both selects within each transaction will not see the updates. So your check for "has this changed and should I rollback" will yield "no" for both transactions.

An undesirable fix is to lock the table for the update as it will stall quote-to-order conversion in the checkout.

Perhaps the real fix is to put UNIQUE(order_item_id) on sales_flat_shipment_item?

6
  • Tomorrow I could simulate a concurrent change of the database. On adding a shipment to an order the qty_shipped field of the corresponding sales_flat_order_item is updated. I could step through the code using xdebug and pause directly before the update of that table and change the value in the database directly. That should simulate an update done by another process (e.g. another admin creating a shipment). I doubt that the transaction isolation level makes a difference, but I will test it. What do you think of this method? – ochnygosch Feb 25 '15 at 21:29
  • WHen using firebug or chrome dev tools, you can copy a request "as curl". If you fire both up on the command line where the first is put in the background, you should have near concurrent requests firing. With "both" I mean, using two different logins, so two different adminhtml cookies and perhaps /key/ part of the URL, but otherwise identical request. – Melvyn Feb 26 '15 at 0:02
  • Thanks for the clarifying update. I see your point. I've dug through the mysql manual and according to dev.mysql.com/doc/refman/5.7/en/innodb-consistent-read.html the repeatable read isolation level doesn`t apply to DML statements. Issuing a plain select statement won't help, but an UPDATE statement that tests for the old values should work. – ochnygosch Feb 26 '15 at 7:22
  • I could not reproduce the malicious behaviour with chrome and curl. It is a one in a million shot to get the timing right. I will try to add my findings to my question. – ochnygosch Feb 26 '15 at 7:23
  • Based on the workflow you describe in the update, it should be relatively easy to fix for the common case. Models have a data and old_data array. An array diff should yield both the field name and old value. Combined with an array of field names on the model that identify the fields to stick into the where clause and indexes on those fields if it helps performance, it should be possible to add the required filters to make process A fail. – Melvyn Feb 26 '15 at 9:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.