[RFC] - Add support for tracking what throwable closed the entity manager

[petrosz]

#SymfonyHackDay

I would like some feedback on this before I go on and also try to write some tests.

Problem I am trying to solve:
When we run long process, usually in workers consuming a queue of tasks, we might get an exception. That exception is not thrown immediately but it is added in a result object. This object will then be persisted in Mysql in a task_note table.

Interesting to mention that we use multiple database servers so a process might access 3 database servers during the same execution.

When the exception is not closing the entity manager, everything is ok.

When the entity manager is closed, the insert to task_note fails. Other inserts/updates before that, using the other entity managers, work fine.

So when this happens, our mysql task_note doesn't have the error. We then have to go to new relic. What we see there is only "Entity Manager closed" and the stacktrace points to the save($taskNote) which is basically not the place where the error occurred.

Then it is very time consuming to travel up the whole process until you find a mysql operation that might have failed. There is no trace of it.

It's important to note here that just throwing the exception and killing everything is not a good choice for us because we want the worker to keep working if the exception is not DB related.

in order to mitigate this we have to do this:

 try {
            $this->setNewRelicCustomParameter("Task", $task->getId());

            // do the job

            $daemonActivity->setResultFromDaemonActivityResultEnum($daemonActivityResult);
            $this->getDaemonActivityRepository()->save($daemonActivity);
            $this->outputMessage("Finished executing");
        } catch (DBALException $databaseException) {
            // we throw the exception here so at least new relic can catch it with correct stacktrace before we hit an
            // entity manager closed error. Unfortunately, some times the error here is again "entity manager closed" because 
            // of a nested try / catch
            throw $databaseException;
        } catch (Exception $exception) {
            // this will mark the task as failed, store the exception as a note it in the db and continue running
            $this->reactToExceptionWhileExecuting($task, $exception);
        }

Solution I am proposing

• store the exception that caused the entity manager to close
• once we throw the "Entity manager closed" exception, also attach the previous throwable

Examples

• how we deal with a non DB exception in the task

2. an "Entity manager closed" exception before my change

3. after this change

[petrosz]

I also added a test for the specific change I made.

[alcaeus]

I really like the idea, and I was wondering if we could somehow get rid of the setClosingThrowable method. I'll defer to @derrabus for a more detailed review.

[alcaeus]

I'd honestly prefer this kind of logic:

Suggested change

	$this->em->close();
	if ($this->em instanceof EntityManager) {
	$this->em->setClosingThrowable($e);
	}
	$this->em->close($e);

I'm aware that having an EntityManagerInterface may not allow us to do this, but it would be nicer than exposing a public setter to inject the closing throwable.

[petrosz]

I also played a bit with this approach, but I feel that changing the contract of close() is worse than adding the setter.

EntityInterface method is

/**
     * Closes the EntityManager. All entities that are currently managed
     * by this EntityManager become detached. The EntityManager may no longer
     * be used after it is closed.
     *
     * @return void
     */
    public function close();

do we want to give a parameter to a function that by the interface definition doesn't accept one? Won't we have static analysers complain about possible polymorphism (and also be right about it)?

If the team prefers the optional parameter, I can just switch the logic with the same results.

[greg0ire]

If the parameter is optional, there won't be any violation: https://3v4l.org/6MsTY and SA tools will be fine with it: https://phpstan.org/r/b7f1a19d-b90d-423c-8a67-99f278f2a624

The only issue will be a breaking change for classes extending EM, but we could work around that with func_get_args() and remove func_get_args in 3.0

[SenseException]

Did we do such changes in a patch release in the past?

[greg0ire]

Oh right, I forgot to mention the target branch. Let's retarget to 2.18.x

[petrosz]

@greg0ire Thanks for the clarification and the proof from stan. It's something I wouldn't like in our codebase that is why I chose the setter. I will make the refactor, test on 2.18 and send it back.

[greg0ire]

You will be able to feel better when making things right on 3.0.x :)

[petrosz]

also found a related issue #4505

[github-actions]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

[github-actions]

This pull request was closed due to inactivity.