Skip to content

Commit 9f6a012

Browse files
viktymviktym
committed
ACP2E-2055: Duplicate orders with same Quote Id at same time with few time difference
1 parent b446650 commit 9f6a012

File tree

8 files changed

+292
-142
lines changed

8 files changed

+292
-142
lines changed

‎app/code/Magento/Quote/Model/CartLockedException.php‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\Quote\Model;
9+
10+
use Magento\Framework\Exception\StateException;
11+
12+
/**
13+
* Thrown when the cart is locked for processing.
14+
*/
15+
class CartLockedException extends StateException
16+
{
17+
18+
}

‎app/code/Magento/Quote/Model/CartMutex.php‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\Quote\Model;
9+
10+
use Magento\Framework\Lock\LockManagerInterface;
11+
use Psr\Log\LoggerInterface;
12+
13+
/**
14+
* @inheritDoc
15+
*/
16+
class CartMutex implements CartMutexInterface
17+
{
18+
/**
19+
* @var LockManagerInterface
20+
*/
21+
private $lockManager;
22+
23+
/**
24+
* @var LoggerInterface
25+
*/
26+
private $logger;
27+
28+
/**
29+
* @param LockManagerInterface $lockManager
30+
* @param LoggerInterface $logger
31+
*/
32+
public function __construct(
33+
LockManagerInterface $lockManager,
34+
LoggerInterface $logger
35+
) {
36+
$this->lockManager = $lockManager;
37+
$this->logger = $logger;
38+
}
39+
40+
/**
41+
* @inheritDoc
42+
*/
43+
public function execute(int $id, callable $callable, array $args = [])
44+
{
45+
$lockName = 'cart_lock_' . $id;
46+
47+
if (!$this->lockManager->lock($lockName, 0)) {
48+
$this->logger->critical(
49+
'The cart is locked for processing, the request has been aborted. Quote ID: ' . $id
50+
);
51+
throw new CartLockedException(
52+
__('The cart is locked for processing. Please try again later.')
53+
);
54+
}
55+
56+
try {
57+
$result = $callable(...$args);
58+
} finally {
59+
$this->lockManager->unlock($lockName);
60+
}
61+
62+
return $result;
63+
}
64+
}

‎app/code/Magento/Quote/Model/CartMutexInterface.php‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\Quote\Model;
9+
10+
/**
11+
* Intended to prevent race conditions during quote processing by concurrent requests.
12+
*/
13+
interface CartMutexInterface
14+
{
15+
/**
16+
* Acquires a lock for quote, executes callable and releases the lock after.
17+
*
18+
* @param int $id
19+
* @param callable $callable
20+
* @param array $args
21+
* @return mixed
22+
* @throws CartLockedException
23+
*/
24+
public function execute(int $id, callable $callable, array $args = []);
25+
}

‎app/code/Magento/Quote/Model/QuoteManagement.php‎

Lines changed: 36 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,6 @@
5353
*/
5454
class QuoteManagement implements CartManagementInterface, ResetAfterRequestInterface
5555
{
56-
private const LOCK_PREFIX = 'PLACE_ORDER_';
57-
5856
/**
5957
* @var EventManager
6058
*/
@@ -155,11 +153,6 @@ class QuoteManagement implements CartManagementInterface, ResetAfterRequestInter
155153
*/
156154
protected $quoteFactory;
157155

158-
/**
159-
* @var LockManagerInterface
160-
*/
161-
private $lockManager;
162-
163156
/**
164157
* @var QuoteIdMaskFactory
165158
*/
@@ -185,6 +178,11 @@ class QuoteManagement implements CartManagementInterface, ResetAfterRequestInter
185178
*/
186179
private $remoteAddress;
187180

181+
/**
182+
* @var CartMutexInterface
183+
*/
184+
private $cartMutex;
185+
188186
/**
189187
* @param EventManager $eventManager
190188
* @param SubmitQuoteValidator $submitQuoteValidator
@@ -209,9 +207,11 @@ class QuoteManagement implements CartManagementInterface, ResetAfterRequestInter
209207
* @param QuoteIdMaskFactory|null $quoteIdMaskFactory
210208
* @param AddressRepositoryInterface|null $addressRepository
211209
* @param RequestInterface|null $request
212-
* @param RemoteAddress $remoteAddress
210+
* @param RemoteAddress|null $remoteAddress
213211
* @param LockManagerInterface $lockManager
212+
* @param CartMutexInterface|null $cartMutex
214213
* @SuppressWarnings(PHPMD.ExcessiveParameterList)
214+
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
215215
*/
216216
public function __construct(
217217
EventManager $eventManager,
@@ -238,7 +238,8 @@ public function __construct(
238238
AddressRepositoryInterface $addressRepository = null,
239239
RequestInterface $request = null,
240240
RemoteAddress $remoteAddress = null,
241-
LockManagerInterface $lockManager = null
241+
LockManagerInterface $lockManager = null,
242+
?CartMutexInterface $cartMutex = null
242243
) {
243244
$this->eventManager = $eventManager;
244245
$this->submitQuoteValidator = $submitQuoteValidator;
@@ -268,8 +269,8 @@ public function __construct(
268269
->get(RequestInterface::class);
269270
$this->remoteAddress = $remoteAddress ?: ObjectManager::getInstance()
270271
->get(RemoteAddress::class);
271-
$this->lockManager = $lockManager ?: ObjectManager::getInstance()
272-
->get(LockManagerInterface::class);
272+
$this->cartMutex = $cartMutex
273+
?? ObjectManager::getInstance()->get(CartMutexInterface::class);
273274
}
274275

275276
/**
@@ -395,10 +396,28 @@ protected function createCustomerCart($customerId, $storeId)
395396

396397
/**
397398
* @inheritdoc
399+
*/
400+
public function placeOrder($cartId, PaymentInterface $paymentMethod = null)
401+
{
402+
return $this->cartMutex->execute(
403+
(int)$cartId,
404+
\Closure::fromCallable([$this, 'placeOrderRun']),
405+
[$cartId, $paymentMethod]
406+
);
407+
}
408+
409+
/**
410+
* Places an order for a specified cart.
411+
*
412+
* @param int $cartId The cart ID.
413+
* @param PaymentInterface|null $paymentMethod
414+
* @throws CouldNotSaveException
415+
* @return int Order ID.
398416
* @SuppressWarnings(PHPMD.CyclomaticComplexity)
399417
* @SuppressWarnings(PHPMD.NPathComplexity)
418+
* @SuppressWarnings(PHPMD.UnusedPrivateMethod)
400419
*/
401-
public function placeOrder($cartId, PaymentInterface $paymentMethod = null)
420+
private function placeOrderRun($cartId, PaymentInterface $paymentMethod = null)
402421
{
403422
$quote = $this->quoteRepository->getActive($cartId);
404423
$customer = $quote->getCustomer();
@@ -544,7 +563,11 @@ protected function submitQuote(QuoteEntity $quote, $orderData = [])
544563
$order = $this->orderFactory->create();
545564
$this->submitQuoteValidator->validateQuote($quote);
546565
if (!$quote->getCustomerIsGuest()) {
547-
$this->prepareCustomerQuote($quote);
566+
if ($quote->getCustomerId()) {
567+
$this->_prepareCustomerQuote($quote);
568+
$this->customerManagement->validateAddresses($quote);
569+
}
570+
$this->customerManagement->populateCustomerInfo($quote);
548571
}
549572
$addresses = [];
550573
$quote->reserveOrderId();
@@ -608,12 +631,6 @@ protected function submitQuote(QuoteEntity $quote, $orderData = [])
608631
]
609632
);
610633

611-
$lockedName = self::LOCK_PREFIX . $quote->getId();
612-
if (!$this->lockManager->lock($lockedName, 0)) {
613-
throw new LocalizedException(__(
614-
'A server error stopped your order from being placed. Please try to place your order again.'
615-
));
616-
}
617634
try {
618635
$order = $this->orderManagement->place($order);
619636
$quote->setIsActive(false);
@@ -628,8 +645,6 @@ protected function submitQuote(QuoteEntity $quote, $orderData = [])
628645
} catch (\Exception $e) {
629646
$this->rollbackAddresses($quote, $order, $e);
630647
throw $e;
631-
} finally {
632-
$this->lockManager->unlock($lockedName);
633648
}
634649
return $order;
635650
}
@@ -776,22 +791,4 @@ public function _resetState(): void
776791
{
777792
$this->addressesToSync = [];
778793
}
779-
780-
/**
781-
* Prepare customer quote.
782-
*
783-
* @param Quote $quote
784-
* @return void
785-
* @throws LocalizedException
786-
* @throws NoSuchEntityException
787-
* @throws ValidatorException
788-
*/
789-
private function prepareCustomerQuote(Quote $quote): void
790-
{
791-
if ($quote->getCustomerId()) {
792-
$this->_prepareCustomerQuote($quote);
793-
$this->customerManagement->validateAddresses($quote);
794-
}
795-
$this->customerManagement->populateCustomerInfo($quote);
796-
}
797794
}

‎app/code/Magento/Quote/Test/Unit/Model/CartMutexTest.php‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\Quote\Test\Unit\Model;
9+
10+
use Magento\Framework\Lock\LockManagerInterface;
11+
use Magento\Quote\Model\CartLockedException;
12+
use Magento\Quote\Model\CartMutex;
13+
use PHPUnit\Framework\MockObject\MockObject;
14+
use PHPUnit\Framework\TestCase;
15+
use Psr\Log\LoggerInterface;
16+
17+
class CartMutexTest extends TestCase
18+
{
19+
/**
20+
* @var LockManagerInterface|MockObject
21+
*/
22+
private $lockManager;
23+
24+
/**
25+
* @var LoggerInterface|MockObject
26+
*/
27+
private $logger;
28+
29+
/**
30+
* @var CartMutex
31+
*/
32+
private $cartMutex;
33+
34+
/**
35+
* @inheritDoc
36+
*/
37+
protected function setUp(): void
38+
{
39+
$this->lockManager = $this->createMock(LockManagerInterface::class);
40+
$this->logger = $this->createMock(LoggerInterface::class);
41+
$this->cartMutex = new CartMutex($this->lockManager, $this->logger);
42+
}
43+
44+
/**
45+
* Tests cart mutex execution with different callables.
46+
*
47+
* @param callable $callable
48+
* @param array $args
49+
* @param mixed $expectedResult
50+
* @return void
51+
* @dataProvider callableDataProvider
52+
*/
53+
public function testSuccessfulExecution(callable $callable, array $args, $expectedResult): void
54+
{
55+
$cartId = 1;
56+
$this->lockManager->expects($this->once())
57+
->method('lock')
58+
->with($this->stringContains((string)$cartId))
59+
->willReturn(true);
60+
$this->lockManager->expects($this->once())
61+
->method('unlock')
62+
->with($this->stringContains((string)$cartId));
63+
64+
$result = $this->cartMutex->execute($cartId, $callable, $args);
65+
66+
$this->assertEquals($expectedResult, $result);
67+
}
68+
69+
/**
70+
* @return array[]
71+
*/
72+
public function callableDataProvider(): array
73+
{
74+
$functionWithArgs = function (int $a, int $b) {
75+
return $a + $b;
76+
};
77+
78+
$functionWithoutArgs = function () {
79+
return 'Function without args';
80+
};
81+
82+
return [
83+
['callable' => $functionWithoutArgs, 'args' => [], 'expectedResult' => 'Function without args'],
84+
['callable' => $functionWithArgs, 'args' => [1,2], 'expectedResult' => 3],
85+
[
86+
'callable' => \Closure::fromCallable([$this, 'privateMethod']),
87+
'args' => ['test'],
88+
'expectedResult' => 'test'
89+
],
90+
];
91+
}
92+
93+
/**
94+
* Tests exception when cart is being processed and locked.
95+
*
96+
* @return void
97+
*/
98+
public function testCartIsLocked(): void
99+
{
100+
$cartId = 1;
101+
$this->lockManager->expects($this->once())
102+
->method('lock')
103+
->with($this->stringContains((string)$cartId))
104+
->willReturn(false);
105+
$this->logger->expects($this->once())
106+
->method('critical')
107+
->with($this->stringContains((string)$cartId));
108+
$this->lockManager->expects($this->never())
109+
->method('unlock');
110+
$this->expectException(CartLockedException::class);
111+
$callable = function () {
112+
};
113+
114+
$this->cartMutex->execute($cartId, $callable);
115+
}
116+
117+
/**
118+
* Private method for data provider.
119+
*
120+
* @param string $var
121+
* @return string
122+
* @SuppressWarnings(PHPMD.UnusedPrivateMethod)
123+
*/
124+
private function privateMethod(string $var)
125+
{
126+
return $var;
127+
}
128+
}

0 commit comments

Comments
 (0)