Database Transaction Handling and ResourceModel, incompatible with outside transactions (and generally a mess?)

[ludwig-gramberg]

Today I stumbled upon a weird design choice in Magento which makes using transaction problematic if not impossible:

When using Magentos Repositories, lets say, the CustomerRepository, it is wrapped in an transaction and at the end of that transaction the commit callbacks are being called. The only problem being, that the callbacks are coupled with the resource model and its connections transaction. But what if I want to wrap multiple things in one transaction using the same connection? Well in that case the commit callbacks wont fire because they do so only on commit level 0. Shouldn't all the commit callbacks be coupled to the connection rather than the resource model? Just isn't very (re)usable that way. And digging deeper I found the CallbackPool class which uses static public methods, so anyone can mess with it... it holds the callbacks grouped by the connection instance. But it is only used inside the resource model commit method.

And how does using the repositories and the frameworks transaction object work together? I think not at all...

Preconditions

• mage 2.1.2

Steps to reproduce

wrap the save of a repository inside another transaction which is called on the Magento\Framework\DB\Adapter\AdapterInterface for instance:

<?php
namespace Something;

use Magento\Customer\Model\ResourceModel\CustomerRepository;
use Magento\Framework\App\ResourceConnection;

class Something {

    public function __construct(
        CustomerRepository $customerRepository,
        ResourceConnection $connection
    ) {
        $connection = $connection->getConnection('write');

        $customer = $customerRepository->getById(1);
        $customer->setFirstname('New Firstname');

        $connection->beginTransaction();
        try {

            $customerRepository->save($customer);
            $connection->commit();

            // the callbacks are all still here ...

            $callbacks = CallbackPool::get(spl_object_hash($connection));
            foreach ($callbacks as $callback) {
                call_user_func($callback);
            }

        } catch(\Exception $e) {
            $connection->rollBack();
            throw $e;
        }

    }
}

Expected result

the commit callbacks run

Actual result

the commit callbacks don't run because of: vendor/magento/framework/Model/ResourceModel/AbstractResource.php

public function commit()
    {
        $this->getConnection()->commit();
        /**
         * Process after commit callbacks
         */
        if ($this->getConnection()->getTransactionLevel() === 0) {
            $callbacks = CallbackPool::get(spl_object_hash($this->getConnection()));
            try {
                foreach ($callbacks as $callback) {
                    call_user_func($callback);
                }
            } catch (\Exception $e) {
                throw $e;
            }
        }
        return $this;
    }

[ludwig-gramberg]

also the save methods on the "normal" models are deprecated, so I guess its encouraged to use the repositories? in that case this should be fixed, the callbacks should be moved to the connection object and be run from there.

also i observed that some repositories (like the customer repo) can produce redundant callbacks. this could be optimized by running each only once and skip the redundant callbacks.

[ludwig-gramberg]

unchanged in 2.1.7 unchanged in current master

where is the right place to get this discussion going?

[korostii]

Well, most of the issues here on GitHub don't seem to ever reach past the QA crowd to anyone technical anytime soon. That is, unless they relate to security or checkout. This is a bit sad considering extensive technical questions like yours (with suggested improvements and even direct pointers to questionable source code).

The official response regarding discussions would probably be to use the community forums instead, but that's probably a dead end, as well. You could also try to ping @maksek or @okorshenko from the community engineering team directly.

Alternatively, you could start a Pull Request containing at least a minor improvement on the callbacks front, and it should get some attention in under a month. But I am afraid the only feedback you'll get that way would be just some (extensive, but still) advice and maybe help with writing tests for the suggested improvement. So this is probably a good option only if you're really prepared to invest some personal time in fixing it yourselves.

[ludwig-gramberg]

@korostii thank you for your answer

this all feels a bit disengaging...

fixing this would require someone who understands the framework level very well to be able to estimate the impact a change like this could have. i can even imagine that at the moment some callbacks never fire because of this. changing this could have a huge impact.

I'm invested in magento but magento 2 keeps throwing weird/unnecessary problems in my face and there is such a huge lack in communication between the core developers and the community.

The whole attitude of this project feels like: "yeah, kind of open source but ..."

[korostii]

Sure, no problem. Yep, it is quite frustrating at times.

fixing this would require someone who understands the framework level very well to be able to estimate the impact a change like this could have

Well, from what I know that person might be @maghamed. GitHub seems to notify people when they get mentioned explicitly, so I'd suggest to wait a few days in case someone responds, but I really don't know if this will be a priority for them now that 2.2.0 is at RC state, so I wouldn't keep my hopes up.

Good luck & have a nice day!

[magento-engcom-team]

@ludwig-gramberg, thank you for your report. We've created internal ticket(s) MAGETWO-82328 to track progress on the issue.

[Swahjak]

Wow, sad to see that such a critical (framework) issue has been completely ignored since 2018. Especially since the 'preferred' way to use the database is through repositories, which introduce more complex transactions levels. @engcom-Alfa @engcom-Bravo @engcom-Alfa

[Swahjak]

We're currently trying to mitigate this issue by using plugins. Warning: these are very early stage tests. This code is by no means meant for a production environment and we don't know what possible other issues / effects this might have.

use Magento\Framework\DB\Adapter\Pdo\Mysql;
use Magento\Framework\Model\CallbackPool;
use Psr\Log\LoggerInterface;

/**
 * Class MysqlPlugin
 */
class MysqlPlugin
{
    /**
     * @var \Psr\Log\LoggerInterface
     */
    private $logger;

    /**
     * @var array<string,bool>
     */
    private $shouldFlush;

    /**
     * MysqlPlugin constructor.
     *
     * @param \Psr\Log\LoggerInterface $logger
     */
    public function __construct(
        LoggerInterface $logger
    ) {
        $this->logger = $logger;
    }

    /**
     * @param \Magento\Framework\DB\Adapter\Pdo\Mysql $subject
     */
    public function beforeCommit(Mysql $subject): void
    {
        if ($subject->getTransactionLevel() <= 1) {
            return;
        }

        $this->shouldFlush[spl_object_hash($subject)] = true;
    }

    /**
     * @param \Magento\Framework\DB\Adapter\Pdo\Mysql $subject
     * @param \Magento\Framework\DB\Adapter\Pdo\Mysql $result
     * @return \Magento\Framework\DB\Adapter\Pdo\Mysql
     */
    public function afterCommit(Mysql $subject, Mysql $result): Mysql
    {
        $hash = spl_object_hash($subject);

        if (!isset($this->shouldFlush[$hash])) {
            return $result;
        }

        if (0 !== $subject->getTransactionLevel()) {
            return $result;
        }

        unset($this->shouldFlush[$hash]);
        $callbacks = CallbackPool::get($hash);

        try {
            foreach ($callbacks as $callback) {
                call_user_func($callback); // phpcs:ignore
            }
        } catch (\Throwable $e) {
            $this->logger->critical($e);
        }

        return $result;
    }
}

use Magento\Framework\Model\CallbackPool;
use Magento\Framework\Model\ResourceModel\AbstractResource;

/**
 * Class AbstractResourcePlugin
 */
class AbstractResourcePlugin
{
    /**
     * @param \Magento\Framework\Model\ResourceModel\AbstractResource $subject
     * @param \Magento\Framework\Model\ResourceModel\AbstractResource $result
     * @return \Magento\Framework\Model\ResourceModel\AbstractResource
     */
    public function afterCommit(AbstractResource $subject, AbstractResource $result): AbstractResource
    {
        if (0 !== $subject->getConnection()->getTransactionLevel()) {
            return $result;
        }

        CallbackPool::clear(spl_object_hash($subject->getConnection()));

        return $result;
    }
}

    <type name="Magento\Framework\DB\Adapter\Pdo\Mysql">
        <plugin name="ffs_magento" type="Namespace\Patch\Plugin\Framework\DB\Adapter\Pdo\MysqlPlugin" />
    </type>
    <type name="Magento\Framework\Model\ResourceModel\AbstractResource">
        <plugin name="ffs_magento" type="Namespace\Patch\Plugin\Framework\ResourceModel\AbstractResourcePlugin" />
   </type>

[ludwig-gramberg]

the rebuilt it, they rushed it and cut one to many corners and now it has come to this. i'm done with mage2, apart from the framework-level absurdities the frontend is a mess and the pivot to spa wont save it. complexity on mage2 has gone through the roof and I don't see the payoff for my clients.

[ParkYongGyu]

@ludwig-gramberg I am interested in this issue raised. How do you think about the below pattern ?

https://webkul.com/blog/database-transactions-magento2/

[ludwig-gramberg]

@ludwig-gramberg I am interested in this issue raised. How do you think about the below pattern ?

https://webkul.com/blog/database-transactions-magento2/

I checked out entirely of mage2. No longer interested in it at all, sry.

[golaod]

Did magento say anything about it? Are they going to fix it or not? Transactions across multiple entities is a must, because no one wants to introduce event sourcing and have some type of compensation events to happen, so the data stays consistent. Wrapping logical set of entities into a single unit of work is much easier.

Does anyone have a solution for it? Maybe simple git patch dropping that if would be sufficient?