The changes made to the order on the onNotify method are not applied on the onReturn method

Marking this as RBTC, #49 is still working on Drupal Core 9.5.3 + Commerce 8.x-2.33

This still needs work as the comments from comment #23 were left unadressed. A new public method was added to the order storage but it hasn't been added to the interface.

Also can't remember why we didn't use dependency injection in the PaymentCheckoutController and... yeah, not really sure how we can provide a failing test so we can safely commit this....

I've used #23 for 3 weeks without any incidents on production. We had several duplicate orders a day before the path.

Has anyone tested whether the problem can be fixed by setting the database isolation level in settings.php?

database isolation levels only apply when within a transaction. The problem here is that two separate requests load an order entity and then save them. Loading an entity happens outside of the transaction, that's why we add locking here.

Re #53, I guess you meant the comments in #27? I replied to the concerns about locking, there are like a dozen of positive reports here as well to confirm that this works, also in regards to not being able to have a test for this. I think it would be possible to start the lock within the test, and then request a page that attempts to load and save the order which should then fail.

> Using a static cache of locked orders works well only if we're attempting to load the order from within the same request.

The static cache is to check if the current request has the lock, it needs and must only work in the current request.

A bit of DI on the controller is possible, but specifically the route match must not be injected, as documented. And yes, I guess an OrderStorageInterface can be introduced.

I'll see if I can find some time for this.

The interface and DI added.

Those fails are unrelated I think?

Added a test, was quite tricky, the problem is of course that the test controller will wait to update, that's the whole point. So what I did instead is set a short timeout, let the controller wait, in the meantime change and save the order, which causes the lock to be freed up and then the test controller can make it's change.

interdiff is slightly off, forgot to add a few changes to the previous commit.

Ah. I did change the presave method to use the existing throw-or-log setting and tested that as well. I think that makes sense, because if we don't throw an exception here, it will happen for the order that used the lock API.

@Berdir: Thanks for working on the changes, the test failures are indeed unrelated... The product layout builder tests are randomly failing...
I'm wondering under which circumstance the lock could not be acquired and in this case... Should the code expect that? Because right now there is no try catch around the calls to the

loadForUpdate()

method even though we document that there are cases where we could return NULL.

The return null is modeled after the regular load method, if you pass an invalid order id it will return NULL. The @return doesn't go into details why, but I think load() doesn't either. If the lock can't be acquired, it will fail with an exception, that's documented on the interface.

It could be caught, but there's nothing you can do at that point, it's not possible to proceed. It's also very unlikely I think.

+++ b/modules/payment/src/Controller/PaymentCheckoutController.php
@@ -84,6 +96,18 @@ class PaymentCheckoutController implements ContainerInjectionInterface {
     $order = $route_match->getParameter('commerce_order');
     $step_id = $route_match->getParameter('step');
     $this->validateStepId($step_id, $order);
+
+    // Reload the order and mark it for updating, redirecting to step below
+    // will save it and free the lock. This must be done before the checkout
+    // flow plugin is initiated to make sure that it has the reloaded order
+    // object. Additionally, the checkout flow plugin gets the order from
+    // the route match object, so update the order there as well with. The
+    // passed in route match object is created on-demand in
+    // \Drupal\Core\Controller\ArgumentResolver\RouteMatchValueResolver and is
+    // not the same object as the current route match service.
...
+    \Drupal::routeMatch()->getParameters()->set('commerce_order', $order);
+

Shouldn't we move this code further up? And not get the order from the route match to start with? Or alternatively, skip specyfing the route parameter type? So the param converter doesn't even attempt to load the order?

Not getting it from route match seems out of scope for this, I think that's not easy and would require API changes (passing it in explicitly). It's not this controller that is "the problem", it's the other place.

It could be moved a few lines up directly below getting the $order, but that's a difference of two lines and IMHO it makes sense to do the validate step first.

Fixed minor phpcs violations and committed the patch from #59. @Berdir: Thank you very much for this :).
Decided to trust the community on this, somehow curious about the potential performance impact though... (I personally haven't applied this to any of the projects I'm working on since I'm currently not experiencing this and the main projects I'm currently involved in are Headless).

It probably makes sense for the JSON API payment resources defined by Commerce API to also call loadForUpdate().

Awesome, thanks, one less patch to worry about!

I fear it's the opposite for me, only working on non-decoupled sites. Someone who uses that and has problems will need to look into that. It quite possibly is less of an issue there and it's mostly also specific to some payment gateways. Typically affects those offsite gateways that redirect back to the site and also have a callback. I guess JSON API also means fewer kernel shutdown shenanigans, which increases the chance of race conditions.

Any reason for reopening the issue? Just to go back to my performance concern, would there be a way to not check whether a lock is available on presave? I don't think so right?

Just to clarify, referring to the following line which is being called even whenever the order wasn't "locked".
!$this->lockBackend->lockMayBeAvailable($this->getLockId($order->id()

Perhaps the impact of this would be minimal for installations using the non DB lock backend, but could be non negligible for the ones using the DB lock backend.

Sorry, the status change was just a stale/refreshed browser window.

The lock check should be negligible, it's a single simple database query. The real performance cost here is more subtitle. The loadForUpdate() method does a loadUnchanged() to make sure we really get the current order entity directly from the database, so we bypasss the entity static and persistent cache. And actually saving does that again then (as already before this was committed). There might be some room for improvements there to statically cache the unchanged Entity, but that could also have side effects and we need to be very careful with that when an entity is saved multiple times on the same request for example.

I don't think statically caching the unchanged entity is a good idea either. I don't think there is anything to change atm... Unless we find bugs in the future ofc. Loading the unchanged entity is the right thing to do from loadForUpdate() for the reasons you mentioned (i.e making sure we really get the current order entity).

I guess there might be an easier way to optimize that, thinking about it. Entity save allows to preset the original entity, so we could do $order->original = clone $order; and it wouldn't load it a second time.

FYI we tracked down our OrderVersionMismatchException problem to some custom logic that was loading every single anonymous order object, and didn't filter for completed orders, so it ended up loading every anonymous cart too... and there were a LOT of them. And because of how the OrderStorage system works it was then trigger a save() operation. On every. Single. Anon. Cart.

Automatically closed - issue fixed for 2 weeks with no activity.

Hi DamienMcKenna,
What was the logic you added to fix this issue?

We changed our custom logic to exit when the visitor was anonymous, so it never processed on anonymous carts/orders.