Cart runs in a deadlock on multiple calls

[ddanninger]

Hi guys,

i have two calls running on every application load parallel

/shop-api/me and /shop-api/carts/%TOKEN%

everything works greats. But as soon as I apply a coupon they conflict with each other. One of those will always result in a 500 Internal Server error

Error on either one of them: An exception occurred while executing 'UPDATE sylius_order SET items_total = ?, adjustments_total = ?, total = ?, updated_at = ? WHERE id = ?' with params [86626564, -40000000, 46626564, \"2020-08-10 21:34:10\", 298]:\n\nSQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction

Any help or guidance would be appreciated

[mamazu]

Hello, thanks for reporting this bug. The problem occours if the cart is viewed and recalculated at the same time.

In both cases it looks like you are just viewing it but actually behind the scenes it gets recalculated every time there is an authenticated request for this user (eg. the /me endpoint). This is a known problem. One solution would be to remove the cart recalculation logic entirely but this might men that you will get an outdated cart.

[ddanninger]

@mamazu thank you for your fast response, really appreciate that.

in my case, it seems like the cart rules are changing constantly on every request without even changing something on the cart is that how it's supposed to be because of checking the expiration dates on promotions?

Do you think if I would create a custom action merging the /shop-api/me & /shop-api/carts/%TOKEN% it would less conflict? Or any other ideas on how I could apply a quick fix?

[mamazu]

Yes, the cart is getting recalculated no matter what. It could also be that product prices have changed or taxation and so on.

Well what you could do is add the cart to the /me endpoint. It would muddy the waters on what endpoints are for what but this would be the easiest fix. On the other hand you could also disable the cart recalculation logic, this would require some more digging but in the end it's the cleaner solution.

[diimpp]

It's not just /me endpoint, we're having deadlocks in checkout and whatever places just from concurrent requests.

Another problem is when deadlock doesn't actually happen, but order totals are incorrect, like shipping price is not included in order total or some phantom price adjustments exists.

The only solution, that I can think of is to introduce business transaction wrap for the whole orderProcessing. For example with pessimistic lock via https://symfony.com/doc/4.4/components/lock.html

[mamazu]

Yeah, this is probably a thing for Sylius core though as we don't change that much in the order processing logic.

[ddanninger]

The only solution, that I can think of is to introduce business transaction wrap for the whole orderProcessing. For example with pessimistic lock via https://symfony.com/doc/4.4/components/lock.html

@diimpp great tip! Seems that this is a way how to fix it.

As a reference for others who step into this problem:

There are two things that i did/changed:

1. for every request to the shop-api i previously passed over the Authorization Header / JWT Token ... which will trigger the order processor on EVERY call. -> i changed that so it will be passed only to those requests that really require the token. Which improved the deadlocks already but still those were thrown from time to time especially with promotions applied.
2. I extended the CompositeOrderProcessor: ( be aware the code is just for testing now and should be improved )

public function process(OrderInterface $order): void
{
    // LockFactory -> https://symfony.com/doc/4.4/components/lock.html
    $lock = $this->factory->createLock('sylius.order.processor', 60);

    if (!$lock->acquire()) {
        return;
    }

    try {
        foreach ($this->orderProcessors as $orderProcessor) {
            $orderProcessor->process($order);
        }
    } catch (\Exception $e) {
        //
    } finally {
        $lock->release();
    }
}

It stills requires more testing, but so far it works great for me.

Thank you @diimpp @mamazu to guide me through an initial quick fix =)

[diimpp]

Great to know, that it helped. :)

A tip on implementation, try a decoration for CompositeOrderProccesor instead of replacement. For example LockingOrderProcessor, that will handle lock and call underling CompositeOrderProcessor, like locking OrderInventoryOperator is done in core sylius.

In case if lock cannot be acquired, I would suggest to throw exception, that can be converted into http 409 - Conflict. For example via kernel.exception listeners.

[bindermuehle]

@diimpp can you share your implementation details? I am trying out both implementation options with either doctrine based optimistic locking or the external locking and I am still running into concurrency issues.

[diimpp]

@bindermuehle it's pretty much the same as snippet above, only you need to have unique lock id

$lock = $this->factory->createLock(sprintf('sylius.order.processor.%s', $order->getId()), 60);

and if lock acquire failed, then you need to throw exception and convert it to http 409 error code, not return void as in the snippet.

I will try to put together a PR over the week.

P.S. If this doesn't help and you're getting deadlocks for authorized users, then you need to solve this issue as well https://github.com/Sylius/ShopApiPlugin/issues/681

[bindermuehle]

hmm now I know what the problem with my solution was. Instead of sending an exception directly I was using an exponential backoff to retry to lock. This didn't work because the transaction of the delayed request was already started and it read the old values. When you cancel the request that doesn't happen. The only other problem I see with this approach is that you will there is a small delta between commiting of the transaction and releasing of the lock which will allow for transactions to be started in between and trigger the same problem again.

[diimpp]

@mamazu please re-open, it's still actual. Right ticket to close is https://github.com/Sylius/ShopApiPlugin/issues/681 :)

[mamazu]

Done. Thanks for the reminder.