Can not run a sequence of commands if the EntityManager is closed

[pablonunez]

Hi, we found a problem running several commands from a web controller, because if an exception is raises, the next command can not execute queries to database.

We did a fork (https://github.com/UEGMobile/tactician-doctrine/) to resolve the problem, and I think it could be interesting if we can PR. But before that I want to know your opinions and possible conflicts. We can allow people to configure this functionality with a parameter or we can "duplicate" the middleware with this behavior.

This is the code we add to TransactionMiddleware:

if(!$this->entityManager->isOpen()){
    // if the entity manager is closed in a previous command, reset the entity manager
    $this->entityManager = $this->entityManager->create(
        $this->entityManager->getConnection(), $this->entityManager->getConfiguration());
}

[rosstuck]

Hi @pablonunez, thanks for using this package and also for such a clear issue report. :)

I've done some thinking about this in the last couple days. As far as I know, the best practice is to ditch the EntityManager after closing it and create a new one (see http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/transactions-and-concurrency.html#exception-handling). I'm assuming the ->create() method was added specifically for this use case rather than having to create your own factory interface, but I'd like to confirm it with someone on the Doctrine team before moving ahead here.

I can see it being a bit tricky because while we might have a new entity manager in the middleware, I don't know if any repositories used in the handlers will be updated to use this new instance. How is that working right now in your app, if you don't mind me asking? :)

As a second note (because this is something I see a lot): if you're sending several commands back-to-back from a single controller action and the success of your action depends on most of them completing, it might be better to compose those actions together inside your domain, rather than a series of commands. For example, a domain service that runs all of the key actions together by calling domain objects directly or maybe using League\Pipeline. If you're chaining several commands together because you're looking for a robust construction, then maybe a Saga/ProcessManager implementation would be a better fit? I'm not trying to divert the PR you've raised, but for many of the command chaining scenarios I see, entering/exiting the application service boundary is just overhead or sometimes composing at a less ideal level. 😄

[pablonunez]

Hi @rosstuck,

In my opinion, close de EntityManager in the middleware when there is an error is a very good approach. When there is an not controlled database error, rollback and close the EM is very reasonable. There are some reason why the EM should be opened again, but where and when is the question.

My team am me discuss a solution before made the pull request, if we should inject the EM and manage it with the logic of the handler (open, close, flush,...), but we think that the handler do not have to care about a not controlled database error. It's not part of their responsibility. Maybe, it's should be very good to have a second shared EM used only for read operation...then, if something goes wrong, at least you can read data. What do you think about that?

Also, we thought to configure the EM as not share service (https://symfony.com/doc/current/service_container/shared.html) for each handler, but this is very inefficient. We do not like this approach. We think that share the EM and check it if it's close before start a command is a better solution.

About you second note, I agree with you, launch several command from a controller should not be the normal use. But we launch event with "Recorded Domain Events" (https://github.com/borNfreee/tactician-domain-events/graphs/contributors), and those events launch new commands in the bus. That why we need to reopen the EM if there is an error in some command. I should know that there is an error in that event, but depending of the importance of the event, I should decide if continue or not, and at least I should be able to do some readonly queries to complete the response of the action.

So, at the end, configure the middleware to reopen the EM is needed in some case. It should be an optional configuration.

[mateuszsip]

Take a look at ManagerRegistry, it was designed for that:

BTW one of the reasons why we created the registry was to deal with cases when a flush fails. After a failed flush, the entity/document is no longer useable since it is now in an undefined state, so it is closed to prevent harm. However this can be problematic for code following. So the registry makes it possible to get a fresh copy.

AFAIK requires https://github.com/symfony/proxy-manager-bridge that integrates https://github.com/Ocramius/ProxyManager to work (makes entity manager a lazy service), and this is optional dependency - otherwise throws :(

Maybe it should be introduced as a separate middleware?

[mateuszsip]

cc @rosstuck @pablonunez

in my case we dispatch commands in consumers and some are allowed to fail (duplicated message or expected error, etc) so specific domain exceptions are catched in consumers where em is already closed.

Additional middleware would be awesome, I can try implement it.

[rosstuck]

tl;dr Let's split rollback off into a separate middleware, not close.

I think ultimately there's two different workflows at play here:

• Workflow A: injecting the ManagerRegistry around or recovering from certain errors to reuse or recreate the EntityManager.
• Workflow B: other folks are injecting only the repository.

The current setup (rollback + close) favors Workflow B because there is no recreating an EntityManager for them. Those repositories they injected don't know about the new EM and are dead in the water (short of some more recent Symfony proxying trickery but this isn't a Symfony specific package). So, closing the EM is tidier and more inline with how they'd normally work.

However, this issue (and #20) tries to add support for Workflow A. The current suggestion (at least in the PR) is to split this into two middleware that can be wrapped discretely. I like that idea and would likely do that were this a brand new package, aside from the middlewares must be in a specific order (close must wrap rollback).

Unfortunately, splitting the close() off into its own middleware adds a bit of a BC break. Close is sometimes used to force ending a connection or expected to free up memory. Explaining this in a changelog note is a bit subtle and I don't think it would be immediately obvious to everyone.

Therefore, my suggestion would be we split rollback off into a new "RollbackOnly" middleware which does exactly that. The current middleware would keep doing what it does, no BC break and folks could opt into the RollbackOnly one when they need it. That also means there's no ordering issues where close can be called before rollback.

If someone wants to change #20 and update the docs (gh-pages branch on the main Tactician repo) then this could move ahead. I'm not particularly planning on picking this one up myself but if someone would like to get it merge ready, I'm happy to include it. :)

[rosstuck]

Closed by #21 :)