checkout / checkout-magento2-plugin

Checkout.com Magento 2 official extension
MIT License
32 stars 33 forks source link

Module times out with the memory_limit exhausted error when accessing the checkout. #446

Closed samuelbsource closed 3 years ago

samuelbsource commented 3 years ago

Hello, one of my clients that uses this module reported that since updating it to the latest version logged in users can no longer access the checkout. After investigating the logs I've found out that PHP is being killed by the memory limit error, increasing the limit to very large values like 8GB did not fix the issue. After some digging into the code I've managed to pinpoint the source of this error which is the following method: \CheckoutCom\Magento2\Model\Service\TransactionHandlerService::getTransactions

    /**
     * Get the transactions for an order.
     */
    public function getTransactions($orderId, $transactionId = null)
    {
        // Get the list of transactions
        $transactions = $this->transactionSearch->create()
            ->addOrderIdFilter($orderId)
            ->getItems();

        // Filter the transactions
        if ($transactionId && !empty($transactions)) {
            $filteredResult = [];
            foreach ($transactions as $transaction) {
                $condition = $transaction->getTxnId() == $transactionId;
                if ($condition) {
                    $filteredResult[] = $transaction;
                }
            }

            return $filteredResult;
        }

        return $transactions;
    }

This part specifically:

$transactions = $this->transactionSearch->create()
    ->addOrderIdFilter($orderId)
    ->getItems();

$this->transactionSearch has a type of TransactionSearchResultInterfaceFactory The problem is TransactionSearchResultInterface does not have such a method as addOrderIdFilter(), but because it's a DataObject no error is reported. The result of this call is that every single transaction that exists in the database is returned, and this is a big problem if you have millions of transactions in your database. Even if you don't have millions of transactions this code here is a performance bottleneck. TransactionSearchResultInterface is not a very good way of getting the desired data either.

A good way to handle this would be use the $this->transactionRepository variable which is the transaction repository, and search for transactions with Magento\Framework\Api\SearchCriteria.

luke-fenn-cko commented 3 years ago

Hello,

We're currently in the process of addressing this, so thank you for bringing the issue to our attention.

A few questions so we can effectively address this:

  1. What module version was your client on before they upgraded to the latest version (2.3.0)?
  2. Have you tested a version with your suggested code changes, if so did it fix the issue?
  3. Did this affect all logged in users, or was it happening intermittently & did this ever happen with users that weren't logged in?

All the best,

Luke

samuelbsource commented 3 years ago

Hi

  1. Last version was 2.2.7
  2. We have not tested any fix yet
  3. Initially we though that the issue applies to all users, but today we've found that some accounts are unaffected.
iancassidyweb commented 3 years ago

Further information on this issue:

Steps to replicate:

  1. Turn on MYSQL logging
  2. Register for a customer account
  3. Add to cart
  4. Proceed to checkout
  5. Observe the fact that the following query gets executed by checkout.com module

SELECT main_table.* FROM sales_payment_transaction AS main_table

If you're a merchant with millions of previous orders you will have millions of records in the sales_payment_transaction table this will load all existing transactions regardless of if they related to this customer or not.

Screenshot 2021-02-26 at 14 32 40

Screenshot 2021-02-26 at 14 31 22

luke-fenn-cko commented 3 years ago

Thank you both for the info.

@iancassidyweb In your steps to replicate, you say "Register for a customer account", is this only happening for a new account?

@instalab Was there any common theme with the unaffected accounts?

FYI: we are currently removing TransactionSearchResultInterface and replacing it for a more performant variation as suggested.

stephen-gilbert-cko commented 3 years ago

Hi @iancassidyweb @instalab would you be able to get back to us on Luke's questions?