php-pm / php-pm-httpkernel

HttpKernel adapter for use of Symfony and Laravel frameworks with PHP-PM
MIT License
246 stars 72 forks source link

Reset doctrine object manager at the end of the request if closed #121

Closed dnna closed 6 years ago

dnna commented 6 years ago

Putting this up for discussion. This prevents the "Entity manager is closed" error from propagating to future requests. Using this together with https://github.com/facile-it/doctrine-mysql-come-back on my project I have not had any database related issues in the past few weeks.

andig commented 6 years ago

As per https://github.com/php-pm/php-pm/issues/391 it might also make sense to check if the entity manager‘s db connection is still open? Unfortunately that also brings the burden of error handling into our turf? I‘m a little unsure if this isn‘t really application responsibility?

dnna commented 6 years ago

Calling resetManager already forces a reconnect AFAIK, so it should also prevent cases where the connection was closed from propagating to future requests.

That said, this PR does not aim to solve the connection timeout problem, because connection timeouts will still happen at random times and throw exceptions that are hard for the user to debug during the request. It just aims to improve general stability by preventing any database errors from rendering the worker unusable until restarted.

dnna commented 6 years ago

The best practice for handling connection timeouts in my opinion is to use a connection wrapper that auto-reconnects on errors (like the one I mentioned above). That's probably an application responsibility (unless we hijack the container and inject our own connection wrapper) but maybe we should mention the issue as part of the documentation so users don't have to dig through several issues and PRs to find the solution?

andig commented 6 years ago

Calling resetManager already forces a reconnect

Yeah- the question is if isOpen checks for closed connection. In my non-symfony code I'm checking for connection, too. However I've got hold of the credentials which isn't the case here.

dnna commented 6 years ago

I'm not sure if isOpen checks for a closed connection but it shouldn't matter - if the connection is closed it will crash in the next request and then the entity manager will be closed :)